Conversation
There was a problem hiding this comment.
Pull request overview
Adds build-system integration to improve developer UX for rebuilding WSLC container images incrementally across MSBuild (C#/C++) and CMake-based consumers.
Changes:
- Introduces MSBuild targets to build/save container images after
Build, with incremental behavior via marker files. - Adds C# CPS up-to-date check inputs/outputs and C++
.tloggeneration for VS Fast Up-to-Date Check. - Adds a CMake module (
wslcConfig.cmake) providingwslc_add_image()to declare image build targets.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| nuget/Microsoft.WSL.Containers/build/Microsoft.WSL.Containers.common.targets | Adds core MSBuild targets for building/saving images with incremental marker outputs and a dependency check for wslc. |
| nuget/Microsoft.WSL.Containers/build/net/Microsoft.WSL.Containers.targets | Adds CPS up-to-date check items intended to track container image sources for .NET SDK projects. |
| nuget/Microsoft.WSL.Containers/build/native/Microsoft.WSL.Containers.targets | Adds .tlog writing/cleanup targets intended to participate in VS Fast Up-to-Date Check for C++ projects. |
| nuget/Microsoft.WSL.Containers/build/cmake/wslcConfig.cmake | Adds CMake helper function to declare container image build targets with incremental rebuild support. |
nuget/Microsoft.WSL.Containers/build/Microsoft.WSL.Containers.common.targets
Outdated
Show resolved
Hide resolved
nuget/Microsoft.WSL.Containers/build/Microsoft.WSL.Containers.common.targets
Show resolved
Hide resolved
nuget/Microsoft.WSL.Containers/build/net/Microsoft.WSL.Containers.targets
Outdated
Show resolved
Hide resolved
nuget/Microsoft.WSL.Containers/build/native/Microsoft.WSL.Containers.targets
Outdated
Show resolved
Hide resolved
nuget/Microsoft.WSL.Containers/build/native/Microsoft.WSL.Containers.targets
Outdated
Show resolved
Hide resolved
nuget/Microsoft.WSL.Containers/build/Microsoft.WSL.Containers.common.targets
Show resolved
Hide resolved
nuget/Microsoft.WSL.Containers/cmake/Microsoft.WSL.ContainersConfig.cmake
Show resolved
Hide resolved
nuget/Microsoft.WSL.Containers/build/Microsoft.WSL.Containers.common.targets
Show resolved
Hide resolved
nuget/Microsoft.WSL.Containers/build/Microsoft.WSL.Containers.common.targets
Outdated
Show resolved
Hide resolved
nuget/Microsoft.WSL.Containers/build/net/Microsoft.WSL.Containers.targets
Outdated
Show resolved
Hide resolved
| add_custom_command( | ||
| OUTPUT "${_marker}" | ||
| COMMAND "${WSLC_CLI_PATH}" image build -t "${_image_ref}" -f "${ARG_DOCKERFILE}" "${ARG_CONTEXT}" | ||
| COMMAND ${CMAKE_COMMAND} -E touch "${_marker}" |
There was a problem hiding this comment.
We don't need to use the flag file for cmake. We can just use the produced files (the image tar file).
There was a problem hiding this comment.
Sure, once we confirmed how to export the image file to build folder, then this line can be removed.
|
|
||
| add_custom_command( | ||
| OUTPUT "${_marker}" | ||
| COMMAND "${WSLC_CLI_PATH}" image build -t "${_image_ref}" -f "${ARG_DOCKERFILE}" "${ARG_CONTEXT}" |
There was a problem hiding this comment.
We need to save the image to the output folder.
TODO: Which compression method to use for the tar file? Is ztsd supported in the wslc session?
There was a problem hiding this comment.
We also need to save the digest file along with the image if we decide to provide a fast path for image id check.
| # find_package(wslc REQUIRED) | ||
| # | ||
| # wslc_add_image( | ||
| # NAME my-server |
There was a problem hiding this comment.
Should we have separate target name and image registry/name:tag here?
Maybe we should just merge the image registry/name and image tag into one image reference parameter.
There was a problem hiding this comment.
Now NAME is specifically used for CMake target names, and the newly added optional parameter IMAGE is used for the image registry/name. If IMAGE is omitted, it will automatically fall back to NAME.
Does this make sense?
| </WslcImage> | ||
| </ItemDefinitionGroup> | ||
|
|
||
| <!-- Auto-detection fallback: if no WslcImage declared, detect container/Dockerfile --> |
There was a problem hiding this comment.
I don't think we should do that. The developer is not required to build their container images here.
There was a problem hiding this comment.
Sure, this is just fallback, will remove it later.
| # Resolve source globs to file lists | ||
| file(GLOB_RECURSE _resolved_sources CONFIGURE_DEPENDS ${ARG_SOURCES}) |
There was a problem hiding this comment.
file(GLOB_RECURSE ...) requires at least one glob expression. Since SOURCES is not validated as required, calling wslc_add_image() without SOURCES will make configuration fail at this line. Handle the empty case (e.g., skip the GLOB when ARG_SOURCES is empty and rely on DOCKERFILE/explicit deps).
| # Resolve source globs to file lists | |
| file(GLOB_RECURSE _resolved_sources CONFIGURE_DEPENDS ${ARG_SOURCES}) | |
| # Resolve source globs to file lists (if any were provided) | |
| if(ARG_SOURCES) | |
| file(GLOB_RECURSE _resolved_sources CONFIGURE_DEPENDS ${ARG_SOURCES}) | |
| else() | |
| set(_resolved_sources) | |
| endif() |
nuget/Microsoft.WSL.Containers/build/net/Microsoft.WSL.Containers.targets
Show resolved
Hide resolved
nuget/Microsoft.WSL.Containers/build/native/Microsoft.WSL.Containers.targets
Outdated
Show resolved
Hide resolved
| # Resolve source globs to file lists | ||
| file(GLOB_RECURSE _resolved_sources CONFIGURE_DEPENDS ${ARG_SOURCES}) | ||
|
|
||
| add_custom_command( | ||
| OUTPUT "${_marker}" | ||
| COMMAND "${WSLC_CLI_PATH}" image build -t "${_image_ref}" -f "${ARG_DOCKERFILE}" "${ARG_CONTEXT}" | ||
| # Uncomment when wslc image save is available: | ||
| # COMMAND ${CMAKE_COMMAND} -E make_directory "${ARG_OUTPUT}" | ||
| # COMMAND "${WSLC_CLI_PATH}" image save -o "${_tar_output}" "${_image_ref}" | ||
| COMMAND ${CMAKE_COMMAND} -E touch "${_marker}" | ||
| DEPENDS ${_resolved_sources} "${ARG_DOCKERFILE}" |
There was a problem hiding this comment.
The custom command invokes wslc ... -f "${ARG_DOCKERFILE}" "${ARG_CONTEXT}" without setting WORKING_DIRECTORY or converting these to absolute paths. With common usage like DOCKERFILE container/Dockerfile and CONTEXT container/, the build tool typically runs commands from the build directory, so these relative paths can fail to resolve. Consider setting WORKING_DIRECTORY to ${CMAKE_CURRENT_SOURCE_DIR} (or the context dir) and/or normalizing ARG_DOCKERFILE/ARG_CONTEXT to absolute paths before use.
| # Resolve source globs to file lists | |
| file(GLOB_RECURSE _resolved_sources CONFIGURE_DEPENDS ${ARG_SOURCES}) | |
| add_custom_command( | |
| OUTPUT "${_marker}" | |
| COMMAND "${WSLC_CLI_PATH}" image build -t "${_image_ref}" -f "${ARG_DOCKERFILE}" "${ARG_CONTEXT}" | |
| # Uncomment when wslc image save is available: | |
| # COMMAND ${CMAKE_COMMAND} -E make_directory "${ARG_OUTPUT}" | |
| # COMMAND "${WSLC_CLI_PATH}" image save -o "${_tar_output}" "${_image_ref}" | |
| COMMAND ${CMAKE_COMMAND} -E touch "${_marker}" | |
| DEPENDS ${_resolved_sources} "${ARG_DOCKERFILE}" | |
| # Normalize paths to be independent of the build directory. | |
| get_filename_component(_dockerfile_path "${ARG_DOCKERFILE}" ABSOLUTE BASE_DIR "${CMAKE_CURRENT_SOURCE_DIR}") | |
| get_filename_component(_context_path "${ARG_CONTEXT}" ABSOLUTE BASE_DIR "${CMAKE_CURRENT_SOURCE_DIR}") | |
| # Resolve source globs to file lists | |
| file(GLOB_RECURSE _resolved_sources CONFIGURE_DEPENDS ${ARG_SOURCES}) | |
| add_custom_command( | |
| OUTPUT "${_marker}" | |
| COMMAND "${WSLC_CLI_PATH}" image build -t "${_image_ref}" -f "${_dockerfile_path}" "${_context_path}" | |
| # Uncomment when wslc image save is available: | |
| # COMMAND ${CMAKE_COMMAND} -E make_directory "${ARG_OUTPUT}" | |
| # COMMAND "${WSLC_CLI_PATH}" image save -o "${_tar_output}" "${_image_ref}" | |
| COMMAND ${CMAKE_COMMAND} -E touch "${_marker}" | |
| DEPENDS ${_resolved_sources} "${_dockerfile_path}" |
| <Target Name="WslcBuildAllImages" | ||
| AfterTargets="Build" | ||
| DependsOnTargets="WslcCheckDependencies" | ||
| Condition="'@(WslcImage)' != ''"> |
There was a problem hiding this comment.
WslcBuildAllImages runs AfterTargets="Build" whenever @(WslcImage) is non-empty. In Visual Studio this will also execute during design-time builds (when $(DesignTimeBuild) is true), which can trigger expensive wslc image build calls and slow the IDE. Add a guard to skip both WslcBuildAllImages (and ideally WslcCheckDependencies) during design-time builds (e.g., require $(DesignTimeBuild) != 'true').
| Inputs="@(_WslcSourceFiles)" | ||
| Outputs="$(IntDir)wslc_$(_WslcName).marker"> | ||
|
|
There was a problem hiding this comment.
The marker file path uses $(_WslcName) (from %(WslcImage.Identity)) directly: $(IntDir)wslc_$(_WslcName).marker. If an image name contains / (common for registry/repo names like ghcr.io/org/image), this becomes a nested path and MakeDir only creates $(IntDir), so the target can fail to create/touch the marker. Consider deriving a file-system-safe marker name (e.g., replace invalid path chars or use a hash) and use that for marker/tlog filenames.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| # set(_tar_output "${ARG_OUTPUT}/${ARG_NAME}.tar") | ||
|
|
||
| # Resolve source globs to file lists | ||
| file(GLOB_RECURSE _resolved_sources CONFIGURE_DEPENDS ${ARG_SOURCES}) |
There was a problem hiding this comment.
SOURCES is optional, but incremental rebuild correctness depends on it. If callers omit SOURCES (as in the second usage example), this glob resolves to an empty list and the build won’t rerun when other context files change. Either require SOURCES or default it to include the full CONTEXT directory contents.
| file(GLOB_RECURSE _resolved_sources CONFIGURE_DEPENDS ${ARG_SOURCES}) | |
| if(ARG_SOURCES) | |
| file(GLOB_RECURSE _resolved_sources CONFIGURE_DEPENDS ${ARG_SOURCES}) | |
| else() | |
| # If SOURCES is not provided, default to the full CONTEXT directory contents | |
| file(GLOB_RECURSE _resolved_sources CONFIGURE_DEPENDS "${ARG_CONTEXT}/*") | |
| endif() |
| add_custom_target(wslc_image_${ARG_NAME} ALL | ||
| DEPENDS "${_marker}" | ||
| ) |
There was a problem hiding this comment.
NAME is used verbatim in the generated custom target name (wslc_image_${ARG_NAME}), so names containing spaces or path separators can make CMake error out during generation. Consider validating NAME against a safe pattern (or normalizing it) and emitting a clear FATAL_ERROR when it isn’t usable as a target name.
| <MSBuild Projects="$(MSBuildProjectFullPath)" | ||
| Targets="_WslcBuildSingleImage" | ||
| Properties="Configuration=$(Configuration);Platform=$(Platform);_WslcName=%(WslcImage.Identity);_WslcTag=%(WslcImage.Tag);_WslcDockerfile=%(WslcImage.Dockerfile);_WslcContext=%(WslcImage.Context);_WslcSourceDir=%(WslcImage.Sources);_WslcOutput=%(WslcImage.OutputDir)" /> | ||
| </Target> |
There was a problem hiding this comment.
%(WslcImage.Sources) can be semicolon-delimited (multi-directory Sources), but here it’s passed through the MSBuild task Properties attribute, which itself is semicolon-delimited. Without escaping, values after the first ; will be treated as separate properties and the extra source dirs will be dropped/break parsing. Escape this value (e.g., via MSBuild::Escape) or pass the source directories via an item list instead of a property string.
…into user/shawn/buildUX
Summary of the Pull Request
DO NOT MERGE, still a draft
Status:
Done
<WslcImage>declaration (all properties)wslc_add_image()functionBlocked (waiting on wslc CLI)
wslc image savenot available)wslc image inspect --formatnot available)Tests passed
Tests not yet done
.nupkg)PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed