Improved details for build and pull#40596
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds richer progress reporting to wslc build and wslc pull: pull now shows progress bars with current/total bytes, and build shows per-layer download progress lines that update in place along with colored build-step output.
Changes:
- Plumb
current/totalbyte counts from BuildKit status throughBuildKitStatusJSON,reportProgress, and into the callbacks. - Render a fixed-width progress bar with
FormatBytesoutput inImageProgressCallback, and truncate to console width to avoid wrap artifacts. - In
BuildImageCallback, maintain a per-entrym_pullLinesmap rendered each redraw, and colorize permanent build-step lines in teal.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/windows/wslcsession/WSLCSession.cpp | Extends reportProgress to forward current/total and emits per-entry progress when entry.total > 0. |
| src/windows/wslc/services/ImageProgressCallback.cpp | Replaces percent text with a 30-char progress bar plus formatted byte counts; truncates to console width. |
| src/windows/wslc/services/BuildImageCallback.h | Adds m_pullLines map and <map> include. |
| src/windows/wslc/services/BuildImageCallback.cpp | Stores per-id pull status, redraws periodically, and wraps permanent lines in teal ANSI escapes. |
| src/windows/inc/docker_schema.h | Adds current/total fields to BuildKitStatus deserialization. |
| src/shared/inc/stringshared.h | New FormatBytes helper producing GB/MB/KB/B strings. |
| // accepting any non-empty id, so future or unrelated id usage defaults to permanent. | ||
| const bool isLog = (id != nullptr && std::string_view{id} == "log"); | ||
|
|
||
| // Pull/download progress: update the per-entry map so Redraw can show each entry |
There was a problem hiding this comment.
I'm not sure we are handling this correctly when m_isConsole is false. We should probably skip the whole callback stuff when non in an interactive terminal.
There was a problem hiding this comment.
I think this would break AIs who are scraping wslc.exe build output. Right now we output something like this:
PS C:\cDev\copilot\build-test> cat .\dockerfilesimple.log
Building image from directory: .
[1/2] FROM docker.io/library/ubuntu:24.04@sha256:c4a8d5503dfb2a3eb8ab5f807da5bc69a85730fb49b5cfca2330194ebcc41c7b
[2/2] RUN echo 123
exporting to image
| exporting layers
| writing image sha256:780c43a5ab95735b080898e63db96c3feebdd05591a0a934bc3252fe70201004
| naming to docker.io/library/test
PS C:\cDev\copilot\build-test>
I think this is pretty reasonable and gives the right info on all the steps.
| inline std::wstring FormatBytes(uint64_t bytes) | ||
| { |
There was a problem hiding this comment.
I am wondering if we could move this to wsl::windows::common::string and use the windows StrFormatByteSizeEx. It does have a slightly different oputput. StrFormatByteSizeEx uses 1024-based math with the Windows-conventional labels (KB, MB, GB, TB) — i.e. 1.00 KB means 1024 bytes. The current helper is 1000-based. So switching will change the numbers users see during pulls which would diverge from docker's behavior. However, it does give localized decimal seperator. I could see this going either way.
ALSO, if we are making a helper for this, we should probably get rid of the other FormatBytes method in ContainerTasks.cpp:37 and just use a common helper.
There was a problem hiding this comment.
Opportunity to centralize this, such as moving the one from ContainerTasks into stringshared.h
I'm not sure if Craig's PR is the right place to do that consolidation, but at the very least using the same method for it is appropriate.
There was a problem hiding this comment.
I'm trying to leave this PR as small as possible, so I will refrain for now from doing refactoring changes. If it blocks this PR from going in then I can do it!
| terminator.insert(terminator.begin(), wide.back()); | ||
| wide.pop_back(); | ||
| } | ||
| wide = std::format(L"\033[36m{}\033[0m{}", wide, terminator); |
There was a problem hiding this comment.
Really nit, don't hate me for this. There are a lot of these ANSI escape sequences scattered throught the file, it would be nice to have to have these defined as constants for readability. Either at the top of this file or a common header. (IDK if its used elsewhere).
For example, this format could read like: wide = std::format(L"{}{}{}{}", ansi::Cyan, wide, ansi::ResetColor, terminator);
Then its a lot easier to understand what is happening.
There was a problem hiding this comment.
Actually, I did a brief search with copilot, and alledgedly its also used in WSLCTests.cpp, WSLCE2EHelpers.h, ImageProgressCallback.cpp, BuildImageCallback.cpp. So this could warrant placing in stringshared.h or deps.h. The test files i mentioned use the VT_* constants.
There was a problem hiding this comment.
I was about to make the same comment. I'm not sure this PR should do a lot of refactoring, which could make it a lot bigger than it should be, but at the very least define the VT sequences as constants so the construction is a lot easier to read and so we can do that refactoring more easily later.
There was a problem hiding this comment.
Good suggestion, but in the interest of leaving this PR as small as possible I will leave it in as magic numbers. Can change that though if it's a blocker.
There was a problem hiding this comment.
I am ok with either choice. Definitely, pro not touching the test files here that are unrelated to this PR.
However, Ideally, I would like to see at least local constants defined (or at least a comment), so we know what each of the ansi strings mean without having to do google search. It would really just add a couple of lines at the top of the .cpp file. Then later we can go back and refactor them all into a common header.
Either way, let's make sure we don't forget to do the refactoring that was discussed in the PR. Please create ADO work items for anything that we decide to defer.
dkbennett
left a comment
dkbennett
left a comment
There was a problem hiding this comment.
Echo a lot of the great feedback Kevin had. I like that this PR is relatively small though, and if refactoring things would lead to it growing significantly I would recommend not doing that and having a separate PR for that refactoring. But definitely all for things that make the refactoring easier at the very least.
| inline std::wstring FormatBytes(uint64_t bytes) | ||
| { |
There was a problem hiding this comment.
Opportunity to centralize this, such as moving the one from ContainerTasks into stringshared.h
I'm not sure if Craig's PR is the right place to do that consolidation, but at the very least using the same method for it is appropriate.
| terminator.insert(terminator.begin(), wide.back()); | ||
| wide.pop_back(); | ||
| } | ||
| wide = std::format(L"\033[36m{}\033[0m{}", wide, terminator); |
There was a problem hiding this comment.
I was about to make the same comment. I'm not sure this PR should do a lot of refactoring, which could make it a lot bigger than it should be, but at the very least define the VT sequences as constants so the construction is a lot easier to read and so we can do that refactoring more easily later.
kvega005
left a comment
kvega005
left a comment
There was a problem hiding this comment.
On a successful build, the dim scrolling window gets erased by the final CollapseWindow(), so once the build finishes only the teal step headers ([1/4] FROM …, [2/4] RUN …) remain in scrollback. The replay branch in the destructor only fires when std::uncaught_exceptions() indicates failure.
For comparison, DOCKER_BUILDKIT=0 docker build keeps all RUN stdout in scrollback interleaved with step headers. So things like npm WARN …, pip resolver notes, or a BUILD SUCCESSFUL in 47s line from a tool inside the container aren't recoverable after a successful wslc build without re-running it.
Not sure if that's intentional (the compact view is genuinely nice) — flagging in case it isn't. A couple of options if you do want to address it:
- Drop the
std::uncaught_exceptions()check so the log replays on success too.m_allLinesis already populated and bounded byc_maxAllLinesBytes. - Or add a
--progress=plain/--quiet-style opt-out (could just reuse the existingm_verbosepath).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/windows/wslc/services/BuildImageCallback.cpp:210
- The redraw line budgeting can exceed
c_maxDisplayLineswhenm_pullLines.size()is larger than the max (e.g., many layers):reservedLinesispullCount + pending, but onlycompletedCountis reduced, sodisplayCount(andm_displayedLines) can still grow beyond the intended bound. This can lead to excessive cursor movement/clears and an unexpectedly tall progress UI. Consider capping the number of pull progress lines rendered (e.g., show the most recent N), or folding/scrolling them sodisplayCount <= c_maxDisplayLinesalways holds.
// Determine how many completed lines to show, leaving room for the pending line and pull progress.
const bool showPending = !m_pendingLine.empty();
const SHORT pullCount = static_cast<SHORT>(m_pullLines.size());
SHORT completedCount = static_cast<SHORT>(m_lines.size());
const SHORT reservedLines = (showPending ? 1 : 0) + pullCount;
if (completedCount + reservedLines > c_maxDisplayLines)
{
completedCount = std::max<SHORT>(0, c_maxDisplayLines - reservedLines);
}
const SHORT displayCount = completedCount + reservedLines;
craigloewen-msft
force-pushed
the
user/crloewen/better-build-output
branch
from
df246c5 to
2e644a0
Compare
I'm not seeing this behaviour in docker when I do a build. For example here's an output where I run 'apt update' you can see it compacts it. So I'll leave the compacting behaviour :) 
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/windows/wslc/services/ImageProgressCallback.cpp:138
- Build failure:
visibleWdithis a typo (undefined identifier). This should resize using thevisibleWidthvariable computed above so the line is padded correctly.
// Erase any previously written char on that line.
line.resize(visibleWdith, L' ');
| // Use the visible window width (not the buffer width) to prevent wrapping. | ||
| const auto visibleWidth = std::max<SHORT>(0, info.srWindow.Right - info.srWindow.Left + 1); | ||
|
|
||
| // Truncate to console width to prevent wrapping that would break cursor repositioning. | ||
| if (line.size() > static_cast<size_t>(visibleWidth)) |
There was a problem hiding this comment.
Is this valid? If both places are intending the same behavior, it might be a good idea to consolidate into a helper.
| inline std::wstring FormatBytes(uint64_t bytes) | ||
| { | ||
| constexpr double c_kB = 1000.0; | ||
| constexpr double c_MB = 1000.0 * 1000.0; | ||
| constexpr double c_GB = 1000.0 * 1000.0 * 1000.0; | ||
|
|
There was a problem hiding this comment.
I like the SI / IEC distinction on format bytes. Or, merge the two together with a FormatUnitType enum as a parameter (and default to one).
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
craigloewen-msft
force-pushed
the
user/crloewen/better-build-output
branch
from
2645417 to
742e3b1
Compare
| } | ||
|
|
||
| // Erase any previously written char on that line. | ||
| line.resize(visibleWdith, L' '); |
| // Use the visible window width (not the buffer width) to prevent wrapping. | ||
| const auto visibleWidth = std::max<SHORT>(0, info.srWindow.Right - info.srWindow.Left + 1); | ||
|
|
||
| // Truncate to console width to prevent wrapping that would break cursor repositioning. | ||
| if (line.size() > static_cast<size_t>(visibleWidth)) |
| inline std::wstring FormatBytes(uint64_t bytes) | ||
| { | ||
| constexpr double c_kB = 1000.0; | ||
| constexpr double c_MB = 1000.0 * 1000.0; | ||
| constexpr double c_GB = 1000.0 * 1000.0 * 1000.0; | ||
|
|
||
| if (bytes >= static_cast<uint64_t>(c_GB)) | ||
| { | ||
| return std::format(L"{:.2f} GB", bytes / c_GB); | ||
| } | ||
| else if (bytes >= static_cast<uint64_t>(c_MB)) | ||
| { | ||
| return std::format(L"{:.2f} MB", bytes / c_MB); | ||
| } | ||
| else if (bytes >= static_cast<uint64_t>(c_kB)) | ||
| { | ||
| return std::format(L"{:.2f} KB", bytes / c_kB); | ||
| } | ||
| else | ||
| { | ||
| return std::format(L"{} B", bytes); | ||
| } | ||
| } | ||
|
|
Summary of the Pull Request
Added some small details to
wslc buildandwslc pull.Specifically this just adds a 'total MB' to build and progress bars and total layer sizes to pull.
Pull
Before:
After:
Build
Before:
After:
PR Checklist
Detailed Description of the Pull Request / Additional comments
Scoped it to be as small as possible.
Validation Steps Performed
Tested behaviour before and after.