-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make container app/entrypoint argument defaults only happen on the RID-specific builds, not the outer RID-less build #47424
make container app/entrypoint argument defaults only happen on the RID-specific builds, not the outer RID-less build #47424
Conversation
…D-specific builds, not the outer RID-less build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR refactors the container entrypoint/appcommand logic to compute its parameters on a RID-specific target, allowing for different behavior on Windows versus Linux containers and preventing duplicate configurations.
- Moves the entrypoint/appcommand property out of the outer-level and onto a per-RID target
- Updates tests to reference the newly introduced RID-specific property
Reviewed Changes
File | Description |
---|---|
src/Containers/Microsoft.NET.Build.Containers/KnownStrings.cs | Added a new static property for container execution arguments |
test/Microsoft.NET.Build.Containers.IntegrationTests/TargetsTests.cs | Updated test build invocations to use the new property for RID-specific behavior |
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/Containers/Microsoft.NET.Build.Containers/KnownStrings.cs:30
- The property name '_ComputeContainerExecutionArgs' starts with an underscore, which is inconsistent with the PascalCase naming used for other public static properties. Consider renaming it to 'ComputeContainerExecutionArgs' to maintain consistency.
public static readonly string _ComputeContainerExecutionArgs = nameof(_ComputeContainerExecutionArgs);
I see tests were updated, but flagging if more tests should be written to cover this scenario? |
There are other tests that actually run the publish and assert that args were passed/built, but those tests use the public surface area (PublishContainer) and so are modeling the end-user experience already. |
/backport to release/9.0.3xx |
Started backporting to release/9.0.3xx: https://github.com/dotnet/sdk/actions/runs/13793114191 |
@baronfel backporting to "release/9.0.3xx" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: make container app/entrypoint argument defaults only happen on the RID-specific builds, not the outer RID-less build
Using index info to reconstruct a base tree...
M src/Containers/packaging/build/Microsoft.NET.Build.Containers.targets
Falling back to patching base and 3-way merge...
Auto-merging src/Containers/packaging/build/Microsoft.NET.Build.Containers.targets
CONFLICT (content): Merge conflict in src/Containers/packaging/build/Microsoft.NET.Build.Containers.targets
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 make container app/entrypoint argument defaults only happen on the RID-specific builds, not the outer RID-less build
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
Fixes dotnet/sdk-container-builds#623
Pulls the entrypoint/appcommand calculations out of the outer-level and puts them on a Target that is computed for each RID. This allows for RID-specific commands (useful for Windows containers vs Linux) and prevents the duplication reported in the linked issue.