Add --output flag to wslc build

[ggarzia-MSFT]

This pull request introduces support for the --output option to the image build command, allowing users to specify a build output destination using the Docker Buildx exporter spec (e.g., type=local,dest=path or type=tar,dest=out.tar). The changes include parsing and validation logic for the new option, updates to the service interface and flags, and user-facing descriptions and error messages.

Support for --output option and Buildx exporter spec:

• Added parsing, validation, and formatting functions for the Buildx --output spec in SpecParsing.cpp and SpecParsing.h, including handling for CSV quoting, supported exporter types, and destination resolution. Also added helpers to determine if the output streams to the client or is a directory exporter. [1] [2] [3]
• Updated the ImageBuildCommand to accept the new --output argument with an appropriate user-facing description. [1] [2]

Service and API updates:

• Extended WSLCBuildImageOptions and related IDL definitions to include Output and OutputHandle fields, and added a new flag WSLCBuildImageFlagsOutputIsDirectory to indicate directory exporters. [1] [2]
• Modified the ImageService::Build method signature to accept the new output parameter and included necessary header imports. [1] [2]

User experience improvements:

• Added localized error messages and help text for invalid or missing --output values in Resources.resw. [1] [2]

Summary of the Pull Request

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

Validation Steps Performed

[Copilot]

Pull request overview

This PR extends wslc image build with Docker buildx-style --secret and --output support, plumbing the new options from CLI parsing through the WSLC service boundary into the VM-side build execution. It also refactors spec-style parsing into a dedicated module and standardizes environment variable reads via a shared helper.

Changes:

• Add --secret and --output CLI arguments with parsing/validation, plus new unit/e2e coverage.
• Extend the image build service/IDL structures to carry secrets and output routing (including streaming output back via a handle).
• Refactor spec parsing into SpecParsing.* and introduce ReadEnvironmentVariable() helper usage.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
test/windows/wslc/WSLCCLISecretParserUnitTests.cpp	New unit tests covering --secret spec parsing/validation and value resolution.
test/windows/wslc/WSLCCLIOutputParserUnitTests.cpp	New unit tests defining/validating --output spec parsing and canonical formatting.
test/windows/wslc/WSLCCLIEnvVarParserUnitTests.cpp	Refactor tests to use ScopedEnvVariable for cleaner env var setup/teardown.
test/windows/wslc/e2e/WSLCE2EImageBuildTests.cpp	Add e2e coverage for secrets and output exporters (tar/local/docker/image/cacheonly) with shared contexts.
src/windows/wslcsession/WSLCSession.cpp	VM-side build execution: mount tracking, secret staging into tmpfs, output routing + streaming back to client.
src/windows/wslc/tasks/ImageTasks.cpp	Wire --secret and --output parsing into the ImageService build call.
src/windows/wslc/services/ImageService.h	Add BuildSecret/BuildOutput models and extend ImageService::Build signature.
src/windows/wslc/services/ImageService.cpp	Convert secrets/output to COM options; implement output handle routing and local-export extraction.
src/windows/wslc/core/EnvironmentOptions.cpp	Switch to shared ReadEnvironmentVariable helper.
src/windows/wslc/commands/ImageBuildCommand.cpp	Register --output and --secret arguments for image build.
src/windows/wslc/arguments/SpecParsing.h	New header centralizing spec-style parsers (secret/output/label/filter/etc.).
src/windows/wslc/arguments/SpecParsing.cpp	New implementations for secret/output parsing + moved other spec parsers.
src/windows/wslc/arguments/ArgumentValidation.h	Include SpecParsing.h and remove moved declarations.
src/windows/wslc/arguments/ArgumentValidation.cpp	Delegate secret validation to ParseSecretSpec; remove moved parsing implementations.
src/windows/wslc/arguments/ArgumentDefinitions.h	Add Secret argument definition.
src/windows/service/inc/WSLCShared.idl	Add WSLCBuildImageFlagsOutputIsDirectory and extend valid flags mask.
src/windows/service/inc/wslc.idl	Add WSLCBuildSecret structures and new BuildImageOptions fields for secrets/output streaming.
src/windows/common/wslutil.h	Declare ReadEnvironmentVariable.
src/windows/common/wslutil.cpp	Implement ReadEnvironmentVariable.
localization/strings/en-US/Resources.resw	Add localized strings for secret/output errors and help text.

[dkbennett]

This factoring is fine per additional review.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

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

• ParseOutputSpec() lowercases keys for case-insensitive matching, but it stores non-structural attributes using the original key casing. This makes validation inconsistent: for example type=registry,NAME=repo will be rejected because the required 'name' attribute lookup is case-sensitive. Normalize attribute keys (at least for validation-critical ones like 'name') to keep behavior consistent with the case-insensitive key contract in the comments/tests.

        else
        {
            // Remaining attributes (name, push, compression, annotations, ...) are stored verbatim.
            output.Attributes[kv.Key] = kv.Value;
        }

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

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

• ParseOutputSpec claims keys are matched case-insensitively, but Attributes are stored with the original key casing. This breaks validation like type=registry requiring name=: e.g. "TYPE=registry,NAME=repo/app:tag" will be rejected because the lookup only checks for "name".

        const auto key = toLower(kv.Key);
        if (key == L"type")
        {
            rawType = kv.Value;
            output.Type = toLower(kv.Value);
            hasType = true;
        }
        else if (key == L"dest")
        {
            output.Dest = kv.Value;
            hasDest = true;
        }
        else
        {
            // Remaining attributes (name, push, compression, annotations, ...) are stored verbatim.
            output.Attributes[kv.Key] = kv.Value;
        }

src/windows/wslcsession/WSLCSession.cpp:958

• BuildImage streams exporter output whenever OutputHandle is set, but it doesn't validate that an Output spec was also provided. If a caller sets OutputHandle with Output=null/empty, outputDestPath stays empty and the streamer later runs cat "" (or tar with an empty -C), leading to confusing failures.

    const bool streamOutput = Options->OutputHandle.Type != WSLCHandleTypeUnknown;
    const bool outputIsDirectory = WI_IsFlagSet(Options->Flags, WSLCBuildImageFlagsOutputIsDirectory);
    std::string outputDestPath;

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (3)

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

• The Resources.resw comment for WSLCCLI_BuildOutputArgDescription doesn’t match tools/devops/validate-localization.py rules: only "--output " (with trailing space) will be auto-locked from the string, and extra/incorrect {Locked="..."} markers can fail validation.

  <data name="WSLCCLI_BuildOutputArgDescription" xml:space="preserve">
    <value>Build output destination (docker buildx --output spec; e.g. type=local,dest=path or type=tar,dest=out.tar)</value>
    <comment>{Locked="--output"}{Locked="type=local,dest=path"}{Locked="type=tar,dest=out.tar"}Command line arguments, file names and string inserts should not be translated</comment>
  </data>

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

• ParseSecretSpec splits on ',' using shared string::Split(), which drops empty fields. That means malformed specs like id=s, or id=s,,env=X will be accepted instead of being rejected as an invalid key=value list, since the empty token(s) are never validated.

    for (const auto& part : Split(spec, L','))
    {
        const auto kv = SplitKeyValue(part);
        if (!kv.HadSeparator || kv.Key.empty())

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

• The Resources.resw comment for MessageWslcOutputInvalidSpec doesn’t match tools/devops/validate-localization.py generation rules: this string has '{}' inserts so it must include {FixedPlaceholder="{}"}, and it must lock the exact auto-detected argument token "--output " (with trailing space).

  <data name="MessageWslcOutputInvalidSpec" xml:space="preserve">
    <value>Invalid --output value '{}': {}</value>
    <comment>{Locked="--output "}Command line arguments, file names and string inserts should not be translated</comment>
  </data>

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (3)

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

• ParseSecretSpec() splits the spec using wsl::shared::string::Split(), which drops empty fields. As a result, malformed inputs like id=s,,src=... or a trailing comma won’t be rejected and can be silently accepted. This is inconsistent with ParseOutputSpec(), which explicitly preserves empty fields to reject malformed specs.

    for (const auto& part : Split(spec, L','))

test/windows/wslc/WSLCCLIOutputParserUnitTests.cpp:35

• The header comment says non-structural keys are “stored verbatim in Attributes”, but the implementation normalizes attribute keys to lowercase (matching type/dest normalization). Updating the comment avoids misleading future readers about the contract.

      * Otherwise the spec is a comma separated list of key=value pairs. Keys are matched
        case-insensitively. 'type' and 'dest' populate the struct fields; every other key is
        stored verbatim in Attributes (values may themselves contain '=').

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

• ParseOutputSpec()’s comment says non-structural keys are forwarded “verbatim”, but the function lowercases attribute keys before storing them (and therefore before formatting/forwarding). Either forward keys exactly as provided or (more likely) adjust the comment to reflect the normalization behavior.

    // Mirrors `docker buildx build --output`. A bare token is shorthand for a destination; otherwise
    // the spec is a comma separated list of key=value pairs where 'type'/'dest' are structural and
    // every other key is forwarded verbatim to buildx as an exporter attribute.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/windows/wslc/services/ImageService.cpp:234

• The dest=- validation rejects stdout whenever it’s a FILE_TYPE_CHAR handle, but the user-facing error text says “console”. FILE_TYPE_CHAR includes other character devices (e.g. NUL), and ToCOMInputHandle fails for any FILE_TYPE_CHAR handle. Adjust the error reason to reflect the actual constraint (stdout must be redirected to a file/pipe; character devices aren’t supported), so users don’t get a misleading “console” message when stdout is a different character device.

                    Localization::MessageWslcOutputInvalidSpec(
                        validation::FormatOutputSpec(spec),
                        L"refusing to write build output to the console; redirect stdout to a file or pipe (for example "
                        L"'> out.tar') or pass 'dest=' with a file path"),
                    GetFileType(outputHandle) == FILE_TYPE_CHAR);

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

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

• OutputIsDirectory() only treats tar=false as a directory exporter when the attribute value is exactly "false". buildx treats boolean values case-insensitively and also accepts 0/f, so specs like tar=False would be routed as a file stream on the client/server even though buildx will export a directory (leading to cat() on a directory and a failed/incorrect stream). Use the shared ParseBool helper (Go ParseBool parity) to detect tar=false robustly.

    if (output.Type == L"oci" || output.Type == L"docker")
    {
        // oci/docker default to a single tarball but export an OCI layout directory when tar=false.
        const auto it = output.Attributes.find(L"tar");
        return it != output.Attributes.end() && it->second == L"false";

src/windows/service/inc/WSLCShared.idl:223

• The WSLCBuildImageFlagsOutputIsDirectory comment mentions only type=local, but the code uses this flag for any directory exporter (type=local, and oci/docker when tar=false). Update the comment to match the actual semantics so callers don't misinterpret it.

    WSLCBuildImageFlagsOutputIsDirectory = 8, // OutputHandle receives a tar stream of a directory exporter (type=local); the client extracts it into the destination directory.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

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

• The --output help text includes example exporter specs (e.g. type=local,dest=path) but the Resources.resw comment only locks --output. Other CLI descriptions lock example tokens (e.g. KEY=VALUE, id=NAME) to prevent localization from translating command/spec syntax. Consider locking the example type=... strings here as well so localization validation and translations don't alter the spec format.

    <comment>{Locked="--output "}Command line arguments, file names and string inserts should not be translated</comment>

[dkbennett]

Looks good overall, few small items

[Copilot]

Pull request overview

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

Suppressed comments (2)

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

• The help text for --output includes type=local,dest=path as an example, but the implementation and tests reject type=local (directory exporter) as unsupported. This is misleading and contradicts the documented behavior in this PR; use a supported single-file exporter example instead (e.g., tar or oci).

  <data name="WSLCCLI_BuildOutputArgDescription" xml:space="preserve">
    <value>Build output destination (docker buildx --output spec; e.g. type=local,dest=path or type=tar,dest=out.tar)</value>
    <comment>{Locked="--output "}Command line arguments, file names and string inserts should not be translated</comment>

src/windows/wslcsession/WSLCSession.cpp:1102

• When the server rewrites the buildx --output spec for a mounted destination, it appends dest=<path> without CSV escaping. Because buildx treats the spec as a single CSV record, a destination path containing a comma would be split into multiple fields and the build would fail or mis-route output. Quote the entire dest=... field when the path contains commas (and ideally escape quotes if ever possible).

            {
                dest += '/';
                dest += wsl::shared::string::WideToMultiByte(Options->OutputMountFile);
            }
            outputSpec += std::format(",dest={}", dest);

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/windows/wslcsession/WSLCSession.cpp:1102

• When routing mounted --output, dest is appended via string concatenation (",dest=...") without CSV escaping. A destination filename can legally contain commas/quotes/spaces that would require quoting in buildx's CSV grammar, which would make the generated spec invalid. Consider re-serializing the spec with SplitCsvFields/JoinCsvFields after adding the dest field (same approach as the CLI parser uses).

            outputSpec += std::format(",dest={}", dest);

src/windows/wslcsession/WSLCSession.cpp:1089

• When routing streamed --output, dest is appended via string concatenation (",dest=...") without CSV escaping. If the generated/derived dest ever contains a comma/quote/CR/LF or leading/trailing spaces, this will produce an invalid buildx --output spec and break the build. Re-serialize via SplitCsvFields/JoinCsvFields when adding the new field so quoting rules are applied consistently.

This issue also appears on line 1102 of the same file.

            outputSpec += std::format(",dest={}", outputDestPath);

src/windows/wslcsession/WSLCSession.cpp:69

• IsDirectoryExporterSpec lowercases with std::tolower, which is locale-dependent (e.g., Turkish-I) and can mis-normalize ASCII tokens like "OCI". Since this code is parsing ASCII protocol keys/values, prefer the locale-independent AsciiToLower helper added in stringshared.h.

    const auto toLower = [](std::string_view value) {
        std::string result(value);
        std::transform(
            result.begin(), result.end(), result.begin(), [](unsigned char c) { return static_cast<char>(std::tolower(c)); });
        return result;
    };

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/windows/wslcsession/WSLCSession.cpp:1101

• dest= is appended to the buildx --output spec without CSV escaping. If the destination filename contains a comma/quote/leading-trailing space, buildx will mis-parse the spec. CSV-escape the full dest=... field when adding it.

            auto guestMountPath = mountInVm(Options->OutputMountPath, FALSE);
            std::string dest = guestMountPath;
            if (Options->OutputMountFile != nullptr && Options->OutputMountFile[0] != L'\0')
            {
                dest += '/';
                dest += wsl::shared::string::WideToMultiByte(Options->OutputMountFile);
            }
            outputSpec += std::format(",dest={}", dest);
        }

src/windows/wslcsession/WSLCSession.cpp:1087

• dest= is appended to the buildx --output spec without CSV escaping. If the mounted destination filename contains a comma/quote/leading-trailing space, buildx will mis-parse the spec into extra fields. Use the shared CSV escaping helper when appending the dest= field.

This issue also appears on line 1093 of the same file.

            auto mkdirResult = mkdir.Launch(*m_virtualMachine).WaitAndCaptureOutput(60000UL);
            THROW_HR_IF_MSG(E_FAIL, mkdirResult.Code != 0, "failed to create build output directory");

            outputSpec += std::format(",dest={}", outputDestPath);
        }

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/windows/wslcsession/WSLCSession.cpp:1100

• When mounting a Windows destination for a single-file exporter, dest is appended to the buildx --output spec without CSV escaping. If the destination filename contains a comma (legal on Windows), the resulting spec becomes ambiguous/malformed because buildx parses --output as a single CSV record. Escape the full dest=<path> field using the shared CSV escaping helper so commas/leading spaces round-trip correctly.

            outputSpec += std::format(",dest={}", dest);

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

• The --output help text uses type=local,dest=path as an example, but the implementation explicitly rejects directory exporters (including type=local). This is user-facing and likely to confuse users; update the example(s) to only include supported single-file/stdout outputs.

    <value>Build output destination (docker buildx --output spec; e.g. type=local,dest=path or type=tar,dest=out.tar)</value>