-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Containers support was moved higher to enable creating image of CLI apps #37072
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
Conversation
|
@baronfel, we have CheckContainersPackage target checking Microsoft.NET.Build.Containers package presence. Should I move it too? This check is for users using nuget package which supported only web apps. |
| Condition="'$(EnableSdkContainerSupport)' == 'true' AND Exists('$(MSBuildThisFileDirectory)..\..\..\Containers\build\Microsoft.NET.Build.Containers.props')" /> | ||
|
|
||
| <Import Project="$(_ContainersTargetsDir)Microsoft.NET.Build.Containers.targets" | ||
| Condition="'$(EnableSdkContainerSupport)' == 'true' | ||
| AND Exists('$(_ContainersTargetsDir)Microsoft.NET.Build.Containers.targets') | ||
| AND '$(TargetFramework)' != ''" /> |
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.
I think we need to remove EnableSdkContainerSupport from these conditions, and instead put that condition on the PublishContainer target directly - this will enable someone to call -t:PublishContainer at the solution level and have the target be available (but not enabled) for all projects.
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.
Okay, I wanted to avoid importing these files when publishing is not required.
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.
Here's an example of a scenario that should become possible after we move the Container targets into the base SDK:
- myapp.sln
- myapp.lib.csproj
- myapp.web.csproj (references myapp.lib)
- myapp.cli.csproj (references myapp.lib, has manually set
EnableSdkContainerSupporttotrue) - myapp.tests.csproj (references myapp.lib)
We should be able to run dotnet publish -t:PublishContainer on myapp.sln and get containers generated for myapp.web and myapp.cli as a result.
|
@MichalPavlik once we merge this we'll need to backport to release/8.0.2xx. |
|
This looks correct and great to me - I'd like some level of scenario test to show folks that it does work, and then after merge I can look at updating docs and other resources to talk about the property. |
baronfel
left a comment
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.
With the addition of the test showing how it all works, I'm very happy to approve this PR! Thanks for plumbing it through @MichalPavlik!
|
/backport to release/8.0.2xx |
|
Started backporting to release/8.0.2xx: https://github.com/dotnet/sdk/actions/runs/7103894377 |
Fixes dotnet/sdk-container-builds#513