[Do not review] CLI: Initialize template support#40195
[Do not review] CLI: Initialize template support#40195AmelBawa-msft wants to merge 10 commits intofeature/wsl-for-appsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial “template” output support to the WSLC CLI by introducing a Go-based template renderer (built as a Windows DLL) and wiring it into wslc container list --format ... so users can format container output beyond table and json.
Changes:
- Add
FormatType::Templateand updatecontainer listto render per-container output via a newTemplateRendererbridge. - Introduce a Go
c-sharedDLL (render.dll) exposingRenderGoTemplate/FreeMemory, plus CMake build steps to build/link/copy it. - Relax
--formatparsing so values other thanjson/tablemap to template mode.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/windows/wslc/tools/gotemplate/render.go | Go c-shared implementation that parses JSON and executes a Go text/template. |
| src/windows/wslc/tools/gotemplate/render.def | Export list for the Go-built DLL functions. |
| src/windows/wslc/tasks/ContainerTasks.cpp | Adds FormatType::Template handling for container list to render templates per container. |
| src/windows/wslc/services/ContainerModel.h | Extends FormatType enum with Template. |
| src/windows/wslc/core/TemplateRenderer.h | Declares C++ wrapper API for template rendering. |
| src/windows/wslc/core/TemplateRenderer.cpp | Implements the C++ ↔ Go bridge and error handling. |
| src/windows/wslc/commands/ContainerListCommand.cpp | Removes command-specific format validation (now relies on common validation). |
| src/windows/wslc/commands/ContainerCommand.h | Removes ValidateArgumentsInternal override declaration for container list. |
| src/windows/wslc/arguments/ArgumentValidation.cpp | Changes format parsing to return Template for non-json/table values. |
| src/windows/wslc/CMakeLists.txt | Adds custom build/link/copy steps to build the Go template renderer DLL and link it into wslc. |
| L"Invalid {} value: {} is not a recognized format type. Supported format types are: json, table.", argName, input)); | ||
| } | ||
|
|
||
| return FormatType::Template; |
There was a problem hiding this comment.
GetFormatTypeFromString now treats any --format value other than json/table as FormatType::Template. However, image list also uses ArgType::Format and calls GetFormatTypeFromString, but its task switch does not handle FormatType::Template (it falls into the default/E_UNEXPECTED path). This makes previously-invalid format values cause an unexpected failure for image listing. Either handle FormatType::Template everywhere --format is accepted, or scope template parsing to only commands that implement it (and keep validation errors for others).
| return FormatType::Template; | |
| throw ArgumentException(std::format(L"Invalid {} argument value: '{}'. Supported values are 'json' and 'table'", argName, input)); |
| // Render the template using Go | ||
| try | ||
| { | ||
| for (const auto& container : containers) | ||
| { | ||
| auto json = ToJson(container, c_jsonPrettyPrintIndent); | ||
| auto result = wsl::windows::wslc::core::TemplateRenderer::Render(templateStr, json); | ||
| PrintMessage(result); | ||
| } | ||
| } | ||
| catch (const std::exception& e) | ||
| { | ||
| PrintMessage(MultiByteToWide(std::string("Template rendering error: ") + e.what())); | ||
| context.ExitCode = 1; | ||
| } |
There was a problem hiding this comment.
The new template --format behavior isn’t covered by tests, and it also changes existing behavior (e.g. container list --format invalid is no longer an error and should now be treated as a template). There are existing WSLC e2e tests for container list format handling that will need to be updated/extended to cover the new template mode and its failure cases.
| // Render the template using Go | |
| try | |
| { | |
| for (const auto& container : containers) | |
| { | |
| auto json = ToJson(container, c_jsonPrettyPrintIndent); | |
| auto result = wsl::windows::wslc::core::TemplateRenderer::Render(templateStr, json); | |
| PrintMessage(result); | |
| } | |
| } | |
| catch (const std::exception& e) | |
| { | |
| PrintMessage(MultiByteToWide(std::string("Template rendering error: ") + e.what())); | |
| context.ExitCode = 1; | |
| } | |
| // Render the template for all containers before printing any output so | |
| // template failures do not produce partial results. | |
| try | |
| { | |
| std::vector<std::string> renderedContainers; | |
| renderedContainers.reserve(containers.size()); | |
| for (const auto& container : containers) | |
| { | |
| auto json = ToJson(container, c_jsonPrettyPrintIndent); | |
| renderedContainers.emplace_back(wsl::windows::wslc::core::TemplateRenderer::Render(templateStr, json)); | |
| } | |
| for (const auto& renderedContainer : renderedContainers) | |
| { | |
| PrintMessage(renderedContainer); | |
| } | |
| } | |
| catch (const std::exception& e) | |
| { | |
| PrintMessage(MultiByteToWide(std::string("Template rendering error: ") + e.what())); | |
| context.ExitCode = 1; | |
| } | |
| catch (...) | |
| { | |
| PrintMessage(L"Template rendering error: unknown error"); | |
| context.ExitCode = 1; | |
| } |
|
|
||
| Module Name: | ||
|
|
||
| GoTemplateRenderer.cpp |
There was a problem hiding this comment.
File header Module Name says GoTemplateRenderer.cpp, but the file is TemplateRenderer.cpp. Please update the header to match the actual filename/module to avoid confusion when grepping or auditing.
| GoTemplateRenderer.cpp | |
| TemplateRenderer.cpp |
| // Check if result is an error message (starts with "error:") | ||
| std::string resultStr(result); | ||
|
|
||
| // Free the memory allocated by Go | ||
| FreeMemory(result); | ||
|
|
||
| // If it's an error, throw an exception | ||
| if (resultStr.starts_with("error:")) | ||
| { | ||
| throw std::runtime_error(resultStr); | ||
| } | ||
|
|
There was a problem hiding this comment.
TemplateRenderer::Render treats any rendered output starting with the literal prefix "error:" as a failure. This can misclassify valid template output (a user template can legitimately render a line beginning with error:) and turn a success into an exception. Prefer an out-of-band error signal (e.g., return nullptr on error and expose a separate error buffer/last-error API, or return a status code + output pointer) so success/error can’t collide with normal output.
| // Check if result is an error message (starts with "error:") | |
| std::string resultStr(result); | |
| // Free the memory allocated by Go | |
| FreeMemory(result); | |
| // If it's an error, throw an exception | |
| if (resultStr.starts_with("error:")) | |
| { | |
| throw std::runtime_error(resultStr); | |
| } | |
| std::string resultStr(result); | |
| // Free the memory allocated by Go | |
| FreeMemory(result); |
| add_custom_command( | ||
| OUTPUT ${GO_OUTPUT_DLL} ${GO_OUTPUT_LIB} | ||
| COMMAND ${CMAKE_COMMAND} -E make_directory ${GO_OUTPUT_DIR} | ||
| COMMAND powershell -NoProfile -Command "cd '${GO_TEMPLATE_DIR}' ; \$env:GOOS='windows' ; \$env:GOARCH='${GO_ARCH}' ; \$env:CGO_ENABLED='1' ; go build -o '${GO_OUTPUT_DLL}' -buildmode=c-shared render.go" | ||
| COMMAND lib /def:${GO_TEMPLATE_DIR}/render.def /out:${GO_OUTPUT_LIB} /machine:${GO_LIB_MACHINE} | ||
| DEPENDS ${GO_TEMPLATE_DIR}/render.go ${GO_TEMPLATE_DIR}/go.mod ${GO_TEMPLATE_DIR}/render.def | ||
| COMMENT "Building Go template renderer library" | ||
| VERBATIM | ||
| ) | ||
|
|
||
| add_custom_target(gotemplate_lib ALL DEPENDS ${GO_OUTPUT_DLL} ${GO_OUTPUT_LIB}) | ||
|
|
There was a problem hiding this comment.
This adds an unconditional build-time dependency on the Go toolchain (and CGO) via an ALL custom target. On developer machines/CI images without Go installed, the entire WSLC build will fail with a non-obvious error. Please add an explicit find_program(go ...)/version check (with a clear fatal error), or gate this behind a CMake option / prebuilt artifact so the core build doesn’t silently pick up a new toolchain requirement.
| catch (const std::exception& e) | ||
| { | ||
| PrintMessage(MultiByteToWide(std::string("Template rendering error: ") + e.what())); | ||
| context.ExitCode = 1; | ||
| } |
There was a problem hiding this comment.
Template rendering errors are printed via PrintMessage(...) with the default stream (stdout) and with a hard-coded English prefix. In this codebase, errors are typically routed to stderr and localized via Localization::... / CommandException. Consider throwing a CommandException with a localized message (or at least printing to stderr) so failures behave consistently and can be localized.
| // +build cgo | ||
|
|
||
| package main |
There was a problem hiding this comment.
The Go file uses the legacy build tag syntax (// +build cgo) without the modern //go:build cgo line. Newer Go toolchains still accept the legacy form, but it’s deprecated and can produce warnings in some setups. Consider adding the //go:build cgo line (keeping the // +build line for compatibility if needed).
| add_custom_command(TARGET wslc POST_BUILD | ||
| COMMAND ${CMAKE_COMMAND} -E copy_if_different ${GO_OUTPUT_DLL} $<TARGET_FILE_DIR:wslc>/render.dll | ||
| ) |
There was a problem hiding this comment.
wslc.exe now has a runtime dependency on render.dll, but only a post-build copy step is added. Please ensure the DLL is also included wherever binaries are packaged/deployed (e.g., MSI/MSIX inputs) so installed builds don't fail to start due to a missing DLL. One way is to copy it into the common ${BIN} output directory as part of the gotemplate build/installation step and explicitly include it in packaging manifests.
| case FormatType::Template: | ||
| { | ||
| WI_ASSERT(context.Args.Contains(ArgType::Format)); | ||
| auto templateStr = WideToMultiByte(context.Args.Get<ArgType::Format>()); | ||
|
|
There was a problem hiding this comment.
There are existing WSLC E2E tests covering container list --format json and --format invalid (previously expected to error). With the new Template mode, those expectations will change and there’s currently no test covering successful/failed template rendering. Please update/add E2E coverage for --format <template> (including a parse error case).
| func TryRenderGoTemplate(templateStr *C.char, jsonData *C.char, output **C.char) C.int { | ||
| if templateStr == nil || jsonData == nil { | ||
| *output = C.CString("null pointer provided") | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
TryRenderGoTemplate dereferences output without first validating that the output pointer itself is non-null. If a caller ever passes nullptr for output, this will crash (and the early error path also writes to *output). Please add an output == nil guard before any *output = ... assignments and return failure with a stable error message in that case.
| FormatType GetFormatTypeFromString(const std::wstring& input, const std::wstring& argName) | ||
| { | ||
| if (IsEqual(input, L"json")) | ||
| { | ||
| return FormatType::Json; | ||
| } | ||
| else if (IsEqual(input, L"table")) | ||
|
|
||
| if (IsEqual(input, L"table")) | ||
| { | ||
| return FormatType::Table; | ||
| } | ||
| else | ||
| { | ||
| throw ArgumentException(std::format( | ||
| L"Invalid {} value: {} is not a recognized format type. Supported format types are: json, table.", argName, input)); | ||
| } | ||
|
|
||
| return FormatType::Template; |
There was a problem hiding this comment.
The argName parameter is now unused in GetFormatTypeFromString, which can trigger unused-parameter warnings (often treated as errors in this repo’s builds). Either remove the parameter or explicitly mark it unused (e.g., via std::ignore/UNREFERENCED_PARAMETER) while you decide how to report invalid format values.
| { | ||
| PrintMessage(L"Template rendering error: " + result); | ||
| context.ExitCode = 1; |
There was a problem hiding this comment.
Template rendering failures are printed to the default stream (stdout). Errors in this codebase are typically written to stderr so scripts can reliably parse stdout. Please print template rendering errors to stderr.
| Module Name: | ||
|
|
||
| GoTemplateRenderer.cpp | ||
|
|
||
| Abstract: | ||
|
|
||
| Implementation of the Go template renderer. | ||
|
|
There was a problem hiding this comment.
The header comment’s Module Name says "GoTemplateRenderer.cpp" but the file is TemplateRenderer.cpp. Please update the comment to match the actual filename to avoid confusion when grepping or auditing.
| func TryRenderGoTemplate(templateStr *C.char, jsonData *C.char, output **C.char) C.int { | ||
| if templateStr == nil || jsonData == nil { | ||
| *output = C.CString("null pointer provided") | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
TryRenderGoTemplate dereferences *output without validating that the output pointer itself is non-null. Since this is an exported C ABI entrypoint, add a guard for output == nil and return a failure code to prevent crashes if a caller passes a null output pointer.
| # Build Go template renderer library | ||
| set(GO_TEMPLATE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/tools/gotemplate) | ||
| set(GO_OUTPUT_DIR "${CMAKE_CURRENT_BINARY_DIR}/generated/gotemplate") | ||
|
|
||
| # Reuse TARGET_PLATFORM already normalized by the root CMakeLists.txt | ||
| if("${TARGET_PLATFORM}" STREQUAL "arm64") | ||
| set(GO_ARCH arm64) | ||
| set(GO_LIB_MACHINE ARM64) | ||
| else() | ||
| set(GO_ARCH amd64) | ||
| set(GO_LIB_MACHINE X64) | ||
| endif() | ||
|
|
||
| set(GO_OUTPUT_DLL ${GO_OUTPUT_DIR}/render.dll) | ||
| set(GO_OUTPUT_LIB ${GO_OUTPUT_DIR}/render.lib) | ||
| add_custom_command( | ||
| OUTPUT ${GO_OUTPUT_DLL} ${GO_OUTPUT_LIB} | ||
| COMMAND ${CMAKE_COMMAND} -E make_directory ${GO_OUTPUT_DIR} | ||
| COMMAND ${CMAKE_COMMAND} -E env GOOS=windows GOARCH=${GO_ARCH} CGO_ENABLED=1 | ||
| go build -o ${GO_OUTPUT_DLL} -buildmode=c-shared render.go | ||
| COMMAND lib /def:${GO_TEMPLATE_DIR}/render.def /out:${GO_OUTPUT_LIB} /machine:${GO_LIB_MACHINE} | ||
| WORKING_DIRECTORY ${GO_TEMPLATE_DIR} | ||
| DEPENDS ${GO_TEMPLATE_DIR}/render.go ${GO_TEMPLATE_DIR}/render.def | ||
| COMMENT "Building Go template renderer library" | ||
| VERBATIM | ||
| ) | ||
|
|
||
| add_custom_target(gotemplate_lib ALL DEPENDS ${GO_OUTPUT_DLL} ${GO_OUTPUT_LIB}) |
There was a problem hiding this comment.
The WSLC build now unconditionally depends on Go + cgo (and a working C toolchain for cgo). This is a significant build/CI dependency: consider guarding it behind a CMake option and using find_program(go) with a clear error message, or supplying a prebuilt artifact, so developers/CI without Go+cgo aren’t blocked when building unrelated WSLC changes.
| std::wstring result; | ||
| if (!wsl::windows::wslc::core::TemplateRenderer::TryRender(templateStr, json, result)) | ||
| { | ||
| PrintMessage(L"Template rendering error: " + result); |
There was a problem hiding this comment.
This introduces a new user-facing error string ("Template rendering error: ...") that is not localized. Please route the prefix (and ideally the overall message format) through Localization so it can be translated, while still including the detailed renderer error text as a parameter if needed.
| PrintMessage(L"Template rendering error: " + result); | |
| PrintMessage(result); |
| extern "C" { | ||
| int TryRenderGoTemplate(char* templateStr, char* jsonData, char** output); | ||
| void FreeGoString(char* ptr); | ||
| } | ||
|
|
||
| namespace wsl::windows::wslc::core { | ||
|
|
||
| using namespace wsl::shared::string; | ||
|
|
||
| bool TemplateRenderer::TryRender(const std::string& templateStr, const std::string& jsonData, std::wstring& output) | ||
| { | ||
| char* rawOutput = nullptr; | ||
| auto success = TryRenderGoTemplate(const_cast<char*>(templateStr.c_str()), const_cast<char*>(jsonData.c_str()), &rawOutput); | ||
|
|
There was a problem hiding this comment.
The extern declarations use mutable char* and require const_cast when calling from std::string. Since the Go function does not mutate the inputs, declare the parameters as const char* in the C++ extern (and adjust the Go signature/usage if needed) to avoid casting away const.
| // +build cgo | ||
|
|
||
| package main |
There was a problem hiding this comment.
This file uses only the legacy build tag syntax. Newer Go toolchains prefer a "//go:build cgo" line (while optionally keeping the "+build" line for backward compatibility). Adding the new form helps keep the file compatible with modern Go tooling.
| # Reuse TARGET_PLATFORM already normalized by the root CMakeLists.txt | ||
| if("${TARGET_PLATFORM}" STREQUAL "arm64") | ||
| set(GO_ARCH arm64) | ||
| set(GO_LIB_MACHINE ARM64) | ||
| else() | ||
| set(GO_ARCH amd64) | ||
| set(GO_LIB_MACHINE X64) | ||
| endif() | ||
|
|
||
| set(GO_OUTPUT_DLL ${GO_OUTPUT_DIR}/render.dll) | ||
| set(GO_OUTPUT_LIB ${GO_OUTPUT_DIR}/render.lib) | ||
| add_custom_command( | ||
| OUTPUT ${GO_OUTPUT_DLL} ${GO_OUTPUT_LIB} | ||
| COMMAND ${CMAKE_COMMAND} -E make_directory ${GO_OUTPUT_DIR} | ||
| COMMAND ${CMAKE_COMMAND} -E env GOOS=windows GOARCH=${GO_ARCH} CGO_ENABLED=1 | ||
| go build -o ${GO_OUTPUT_DLL} -buildmode=c-shared render.go | ||
| COMMAND lib /def:${GO_TEMPLATE_DIR}/render.def /out:${GO_OUTPUT_LIB} /machine:${GO_LIB_MACHINE} | ||
| WORKING_DIRECTORY ${GO_TEMPLATE_DIR} |
There was a problem hiding this comment.
CGO cross-compilation is often not supported without an appropriate cross C compiler. This command sets GOARCH based on TARGET_PLATFORM (including arm64) while forcing CGO_ENABLED=1; building an ARM64 WSLC binary on an x64 host is likely to fail. Consider either building this DLL only for native builds, or wiring up CC/CXX for the required cross compiler, or disabling templates on cross-arch builds.
| EXPORTS | ||
| TryRenderGoTemplate | ||
| FreeGoString No newline at end of file |
There was a problem hiding this comment.
For consistency with other .def files in the repo (which typically specify LIBRARY ), consider adding a LIBRARY directive here. This helps avoid ambiguity about the import library’s expected DLL name.
Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed