Skip to content

add --progress to wslc build - #41307

Open
ggarzia-MSFT wants to merge 6 commits into
masterfrom
user/ggarzia/build-progress
Open

add --progress to wslc build#41307
ggarzia-MSFT wants to merge 6 commits into
masterfrom
user/ggarzia/build-progress

Conversation

@ggarzia-MSFT

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

Implements wslc build --progress, which was previously defined but commented out in the
argument table. Supports the same modes as docker build --progress: auto, tty, plain,
quiet, and rawjson. auto resolves to tty when progress output is an interactive VT
console and plain otherwise.

No existing behavior changes when --progress is omitted: the resolved default (auto) picks
the same renderer that was previously hard-coded.

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Argument plumbing

  • Enabled the Progress entry in ArgumentDefinitions.h and registered it on wslc build.
  • Added models::ProgressMode (ContainerModel.h) and GetProgressModeFromString /
    ValidateProgressModeFromString. Values are validated up front by Argument::Validate, so an
    invalid mode fails client-side before any build starts.
  • Values are lowercase-only, matching docker and the existing --format option.

Rendering

  • tty — existing in-place renderer: a scrolling log window plus per-entry pull progress that
    updates in place using cursor movement.
  • plain — append-only. Emits no cursor movement, erases, or color, so output is identical
    whether it goes to a console or a redirected stream.
  • quiet — suppresses live progress but still retains log output, so a failed build replays its
    logs on error.
  • rawjson — forwards docker's raw BuildKit progress JSON verbatim. This required a new
    WSLCBuildImageFlagsRawJson flag in WSLCShared.idl so the server bypasses its own parsing and
    formatting.

Known intentional divergence from docker

BuildKit's plain printer (util/progress/progressui/printer.go) appends throttled download status
lines (#5 sha256:... 1.05MB / 31.4MB 0.2s), reprinting a status only after it advances 5% or 5
seconds elapse. This PR omits pull progress entirely in plain instead, since without in-place
updates those lines are mostly noise. This is documented in a comment at the call site. Everything
else about plain output matches: append-only, no escape sequences.

Note for anyone revisiting this: the server's pull-progress message (WSLCSession.cpp) is the only
progress message emitted without a trailing newline, because it was written for in-place
rendering. It would need one before it could be appended.

Validation Steps Performed

Automated tests added

  • 14 parser cases (ParserTestCases.h) covering --progress=value and --progress value forms,
    all five valid modes, case-sensitivity, unrecognized values, an empty value, and a missing value
    at end of input. The Build argument set mirrors ImageBuildCommand::GetArguments() so the
    parser tests exercise the real option set.
  • 5 unit tests (WSLCCLIBuildImageCallbackUnitTests.cpp) driving BuildImageCallback against a
    VT-enabled capture terminal: plain emits no escape sequences, plain output is byte-identical
    on a console and when redirected, quiet emits nothing, and rawjson is not wrapped in cursor
    control. A tty test asserting escape sequences are emitted is included as a control, so the
    plain assertions cannot pass vacuously.
  • 4 E2E tests (WSLCE2EImageBuildTests.cpp): invalid mode is rejected client-side, rawjson
    contains BuildKit "vertexes" objects, plain contains no 0x1b, and quiet emits no progress
    on success.

Negative controls

The plain-mode fix was verified by reintroducing the bug and re-running: both
PlainOnVtConsole_EmitsNoEscapeSequences and PlainMatchesRedirectedOutput failed, while the
tty, quiet, and rawjson tests still passed. Restoring the fix returned all 5 to passing.

This mattered: the E2E plain test passes with and without the fix, because RunWslc redirects
output and so never reaches the VT-console path where the redraw occurred. The unit tests are what
actually cover it.

Manual / build validation

  • Full cmake --build . -- -m clean.
  • clang-format 19.1.5 clean across all files changed on the branch.
  • Test runs: 4/4 *Progress* E2E, 5/5 WSLCCLIBuildImageCallbackUnitTests,
    18/18 WSLCCLIArgumentUnitTests, 27/27 WSLCCLIParserUnitTests.
  • 4/4 pre-existing *IidFile* E2E tests still pass, confirming the rebase onto the argument-caching
    and ReporterTerminal refactors did not regress the neighbouring build options.

Copilot AI lite review requested due to automatic review settings August 10, 2026 22:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements wslc build --progress (matching Docker’s auto|tty|plain|quiet|rawjson) by plumbing a new ProgressMode through argument parsing/validation into BuildImageCallback, and by adding a server-side passthrough path for rawjson output.

Changes:

  • Adds --progress argument registration, ProgressMode, and upfront validation (GetProgressModeFromString / ValidateProgressModeFromString).
  • Updates build rendering behavior in BuildImageCallback (tty in-place vs plain append-only vs quiet suppression vs rawjson passthrough), and resolves auto based on VT-capable stderr.
  • Extends service protocol with WSLCBuildImageFlagsRawJson and adds unit/E2E/parser coverage for the new flag and rendering expectations.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/windows/wslc/WSLCCLIBuildImageCallbackUnitTests.cpp Adds unit tests for progress rendering (plain/tty/quiet/rawjson) under VT vs redirected output.
test/windows/wslc/WSLCCLIArgumentUnitTests.cpp Adds unit tests for progress mode parsing/validation helpers.
test/windows/wslc/ParserTestCases.h Extends parser test sets to include wslc build and --progress cases.
test/windows/wslc/e2e/WSLCE2EImageBuildTests.cpp Adds E2E coverage for invalid progress mode rejection and plain/quiet/rawjson output expectations.
src/windows/wslcsession/WSLCSession.cpp Adds rawjson stderr passthrough path gated by a new build flag.
src/windows/wslc/tasks/ImageTasks.cpp Plumbs --progress into build execution, resolves auto, and sets the rawjson server flag.
src/windows/wslc/services/ContainerModel.h Introduces models::ProgressMode.
src/windows/wslc/services/BuildImageCallback.h Extends callback to accept a progress mode and control in-place vs append-only rendering.
src/windows/wslc/services/BuildImageCallback.cpp Implements mode-specific rendering paths, quiet buffering, and rawjson printing behavior.
src/windows/wslc/commands/ImageBuildCommand.cpp Registers the --progress argument on wslc build.
src/windows/wslc/arguments/SpecParsing.h Declares GetProgressModeFromString.
src/windows/wslc/arguments/SpecParsing.cpp Implements parsing of progress mode strings into ProgressMode.
src/windows/wslc/arguments/ArgumentValidation.h Declares ValidateProgressModeFromString.
src/windows/wslc/arguments/ArgumentValidation.cpp Hooks progress mode validation into Argument::Validate and implements the validator.
src/windows/wslc/arguments/ArgumentDefinitions.h Enables the previously-commented Progress argument definition.
src/windows/service/inc/WSLCShared.idl Adds WSLCBuildImageFlagsRawJson and updates WSLCBuildImageFlagsValid.
localization/strings/en-US/Resources.resw Updates --progress help text to reflect new supported modes/default.
Suppressed comments (1)

src/windows/wslc/services/BuildImageCallback.cpp:140

  • In Plain mode on a real console, writing log chunks that contain '\r' will still move the cursor and can overwrite the current line (even though no VT escape sequences are emitted). That undermines the stated "append-only" behavior for --progress=plain and can make console output differ from redirected output.
    if (m_verbose || !m_renderInPlace)
    {
        // Only major steps are reported here. Unlike docker's plain output, which appends
        // throttled download lines, pull progress is omitted entirely: without in-place
        // updates those lines are mostly noise.
        if (!isPullProgress)
        {
            m_terminal.Info(L"{}", status);
        }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/windows/wslc/services/BuildImageCallback.cpp Outdated
Comment thread test/windows/wslc/ParserTestCases.h Outdated
Copilot AI review requested due to automatic review settings August 11, 2026 00:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/windows/wslc/services/BuildImageCallback.cpp:142

  • In plain mode this path prints status verbatim, which can include carriage returns (\r). \r causes in-place cursor movement on a real console, so --progress=plain is no longer strictly append-only even though the comment describes it that way. Consider sanitizing \r (e.g., replace with \n) when m_mode == ProgressMode::Plain before emitting to the terminal.
        // Only major steps are reported here. Unlike docker's plain output, which appends
        // throttled download lines, pull progress is omitted entirely: without in-place
        // updates those lines are mostly noise.
        if (!isPullProgress)
        {
            m_terminal.Info(L"{}", status);
        }

test/windows/wslc/ParserTestCases.h:94

  • The Build argument set claims to mirror ImageBuildCommand::GetArguments(), but it diverges (it uses ArgType::Label/ArgType::Output instead of BuildLabel/BuildOutput, and it omits ArgType::IidFile). This can make the parser tests drift from the real wslc build option set and miss validation/parse regressions.
            Argument::Create(ArgType::BuildTarget),
            Argument::Create(ArgType::File),
            Argument::Create(ArgType::Label, false, Limit::Unlimited),
            Argument::Create(ArgType::NoCache),
            Argument::Create(ArgType::Output),

src/windows/wslc/arguments/SpecParsing.cpp:683

  • GetProgressModeFromString throws a hard-coded English error message, while similar parsing helpers (e.g., GetFormatTypeFromString) use localized Localization::WSLCCLI_InvalidFormatValueError(...). Since invalid-argument errors are user-facing (and the PR checklist calls out localization), this should be switched to a localized error pattern (e.g., reuse WSLCCLI_InvalidFormatValueError with a supported-values list for progress modes).
        throw ArgumentException(std::format(
            L"Invalid {} value: {} is not a recognized progress type. Supported progress types are: auto, tty, plain, "
            L"quiet, rawjson.",
            argName,
            input));

Copilot AI review requested due to automatic review settings August 11, 2026 01:22
@ggarzia-MSFT
ggarzia-MSFT force-pushed the user/ggarzia/build-progress branch from ab6fd3a to b6b1117 Compare August 11, 2026 01:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/windows/wslc/arguments/SpecParsing.cpp:713

  • GetProgressModeFromString throws a hard-coded English std::format message, unlike the surrounding parsing helpers (e.g., GetPullPolicyFromString / GetInspectJsonIndentFromString) which use Localization::* errors. This makes the CLI error non-localizable and inconsistent with the rest of the argument parsing errors.
        throw ArgumentException(std::format(
            L"Invalid {} value: {} is not a recognized progress type. Supported progress types are: auto, tty, plain, "
            L"quiet, rawjson.",
            argName,
            input));

test/windows/wslc/ParserTestCases.h:88

  • The Build argument set claims to mirror ImageBuildCommand::GetArguments(), but it uses ArgType::Label and ArgType::Output. The actual build command uses ArgType::BuildLabel and ArgType::BuildOutput, which can differ in conversion/validation behavior. This can cause parser tests to drift from the real wslc build option set and miss regressions.
            Argument::Create(ArgType::Label, false, Limit::Unlimited),
            Argument::Create(ArgType::NoCache),
            Argument::Create(ArgType::Output),

ggarzia-MSFT and others added 4 commits August 11, 2026 08:16
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 11, 2026 16:45
@ggarzia-MSFT
ggarzia-MSFT force-pushed the user/ggarzia/build-progress branch from b6b1117 to 843efda Compare August 11, 2026 16:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/windows/wslc/arguments/SpecParsing.cpp:714

  • GetProgressModeFromString throws a hard-coded English error string via std::format, but other CLI validation errors in this file use localized Resources.resw strings (e.g., WSLCCLI_InvalidPullPolicyError). This introduces a new user-facing message that can’t be localized and will behave inconsistently with the rest of the CLI.
        throw ArgumentException(std::format(
            L"Invalid {} value: {} is not a recognized progress type. Supported progress types are: auto, tty, plain, "
            L"quiet, rawjson.",
            argName,
            input));
    }

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 11, 2026 16:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/windows/wslcsession/WSLCSession.cpp:1436

  • In --progress=rawjson mode, rawJsonPassthrough drops any stderr line that isn’t valid JSON (it clears pendingJson when the first byte isn’t '{'), which can hide warnings/errors that docker emits between JSON objects. Since stderr is already read via LineBasedReadHandle, this logic can be simplified to forward each line verbatim (trimming a trailing '\r' if present) and avoids unbounded buffering if parsing never accepts.
    // rawjson mode: forward each complete JSON object docker writes to stderr verbatim (as newline-delimited
    // JSON) to the client, bypassing the parsing/formatting done by captureOutput.
    auto rawJsonPassthrough = [&](const gsl::span<char>& content) {
        pendingJson.append(content.begin(), content.end());

src/windows/wslc/services/BuildImageCallback.cpp:138

  • Plain mode is intended to be append-only, but in the non-in-place path it prints log payloads verbatim, including '\r' characters. A carriage return will still cause in-place overwrites on an interactive console, so plain output can "redraw" despite emitting no VT escape sequences. Consider normalizing '\r' to '\n' (or otherwise stripping it) when mode==Plain before emitting.
    if (m_verbose || !m_renderInPlace)
    {
        // Only major steps are reported here. Unlike docker's plain output, which appends
        // throttled download lines, pull progress is omitted entirely: without in-place
        // updates those lines are mostly noise.

localization/strings/en-US/Resources.resw:3227

  • The progress option help text lists accepted values with commas, but other similar CLI option descriptions in this file use pipe-separated alternatives (e.g. pull policy, secret type). Using a consistent separator makes scanning help output easier and matches existing style.
    <value>Set type of progress output (auto, tty, plain, quiet, rawjson) (default: auto)</value>

@ggarzia-MSFT ggarzia-MSFT changed the title User/ggarzia/build progress add --progress to wslc build Aug 11, 2026
@ggarzia-MSFT
ggarzia-MSFT marked this pull request as ready for review August 11, 2026 18:10
@ggarzia-MSFT
ggarzia-MSFT requested review from a team as code owners August 11, 2026 18:10
Comment thread src/windows/wslcsession/WSLCSession.cpp Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 11, 2026 21:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/windows/wslc/arguments/SpecParsing.cpp:692

  • The PR description says wslc build --progress supports Docker’s rawjson mode, but the client-side parser/validation currently only accepts {auto, tty, plain, quiet}. As written, --progress=rawjson will be rejected client-side (and the localized help/error text will never list it as supported). Either add rawjson end-to-end (enum value, parsing/validation, help text, and the server/client plumbing to actually forward raw BuildKit JSON) or update the PR description to match the implemented set.
models::ProgressMode GetProgressModeFromString(const std::wstring& input, const std::wstring& argName)
{
    static constexpr std::pair<std::wstring_view, models::ProgressMode> c_progressModes[] = {
        {L"auto", models::ProgressMode::Auto},
        {L"tty", models::ProgressMode::Tty},
        {L"plain", models::ProgressMode::Plain},
        {L"quiet", models::ProgressMode::Quiet},
    };

test/windows/wslc/ParserTestCases.h:94

  • The Build argument set claims to mirror ImageBuildCommand::GetArguments(), but it currently diverges (e.g., Label vs BuildLabel, Output vs BuildOutput, and it omits IidFile). This makes the parser tests easier to desync from the real wslc build option set over time.
    case ArgumentSet::Build:
        // Mirrors ImageBuildCommand::GetArguments() so the parser tests exercise the
        // real `wslc build` option set (notably the --progress value option).
        return {
            Argument::Create(ArgType::Path, true), // Required positional (build context path)
            Argument::Create(ArgType::BuildArg, false, Limit::Unlimited),
            Argument::Create(ArgType::BuildPull),
            Argument::Create(ArgType::BuildTarget),
            Argument::Create(ArgType::File),
            Argument::Create(ArgType::Label, false, Limit::Unlimited),
            Argument::Create(ArgType::NoCache),
            Argument::Create(ArgType::Output),
            Argument::Create(ArgType::Progress),
            Argument::Create(ArgType::Secret, false, Limit::Unlimited),
            Argument::Create(ArgType::Tag, false, Limit::Unlimited),
            Argument::Create(ArgType::Verbose),
            Argument::Create(ArgType::Help),
        };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants