Skip to content
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

Merged

Conversation

baronfel
Copy link
Member

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.

…D-specific builds, not the outer RID-less build
@baronfel baronfel added the Area-Containers Related to dotnet SDK containers functionality label Mar 10, 2025
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Mar 10, 2025
@baronfel baronfel marked this pull request as ready for review March 11, 2025 02:43
@Copilot Copilot bot review requested due to automatic review settings March 11, 2025 02:43
@baronfel baronfel requested a review from a team as a code owner March 11, 2025 02:43

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);
@MattKotsenas
Copy link
Member

I see tests were updated, but flagging if more tests should be written to cover this scenario?

@baronfel
Copy link
Member Author

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.

@baronfel baronfel merged commit 3966b79 into dotnet:main Mar 11, 2025
39 checks passed
@baronfel
Copy link
Member Author

/backport to release/9.0.3xx

Copy link
Contributor

Started backporting to release/9.0.3xx: https://github.com/dotnet/sdk/actions/runs/13793114191

@baronfel baronfel deleted the remove-container-appcommand-from-outer-level branch March 11, 2025 16:24
Copy link
Contributor

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Containers Related to dotnet SDK containers functionality untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-arch container publishing should compute container entrypoint for each RID published
3 participants