dotnet / sdk-container-builds

Libraries and build tooling to create container images from .NET projects using MSBuild
https://learn.microsoft.com/en-us/dotnet/core/docker/publish-as-container
MIT License
176 stars 30 forks source link

`IsPublishable` property is not respected #464

Closed augustfengd closed 9 months ago

augustfengd commented 1 year ago

Hello!

I've got a solution Foo that encompasses two projects, Bar and Baz, where Baz does not have the Microsoft.NET.Build.Containers package installed and is configured with <IsPublishable>false</IsPublishable>.

dotnet publish --os linux --arch x64 /t:PublishContainer -c Release will still try to attempt to build Baz as a container image.

dotnet publish --os linux --arch x64 /t:PublishContainer -c Release
MSBuild version 17.5.0+6f08c67f3 for .NET
  Determining projects to restore...
  Restored /Users/august.feng/learn/fsharp.48/Baz/Baz.fsproj (in 51 ms).
  Restored /Users/august.feng/learn/fsharp.48/Bar/Bar.fsproj (in 79 ms).
  Bar -> /Users/august.feng/learn/fsharp.48/Bar/bin/Release/net7.0/linux-x64/Bar.dll
  Bar -> /Users/august.feng/learn/fsharp.48/Bar/bin/Release/net7.0/linux-x64/publish/
/Users/august.feng/learn/fsharp.48/Baz/Baz.fsproj : error MSB4057: The target "PublishContainer" does not exist in the project.
  Building image 'bar' with tags 1.0.0 on top of base image mcr.microsoft.com:443/dotnet/runtime:7.0
  Pushed container 'bar:1.0.0' to local daemon
baronfel commented 1 year ago

Can you send a binlog or a sample project so I can see more details about the interaction here? Also, is Bar a console project or a Web SDK project, or something else entirely?

augustfengd commented 1 year ago

Hey @baronfel, I've just uploaded a sample project here. As you'll see in the sample, Bar is a console project.

baronfel commented 1 year ago

Awesome, thank you!

baronfel commented 1 year ago

Well the good news is that I wasn't able to repro this on .NET 8 previews. I'll try with the 7.0 series next.

baronfel commented 1 year ago

I also couldn't reproduce with 7.0.305 - what directory are you invoking the publish command from? Bar's project folder? Or a level up at the solution level?

augustfengd commented 1 year ago

Hey, I'm running from the directory at the solution level. Also just confirming the environment, I'm on a Macbook Pro M1 with version 7.0.305 (arm64) and I'm getting the error.

baronfel commented 1 year ago

Ok, this makes more sense to me. When you specify targets at a solution level, those targets are invoked for all projects regardless of if those projects make sense to build that target. Because we don't support PublishProfiles for console apps (yet!) you are forced to specify the target to build.

The fastest way to fix this would be to stop calling publish from the solution directory, and instead publish just the application project that you need. This is my preferred option, as publish from a solution directory has very unclear semantics. You specified an OS and architecture as well - if you had any projects that didn't support that you'd get errors when doing this at the solution level. We really don't encourage doing that :)

Longer term, once we make PublishProfiles work for console apps then this stops being an issue. You can call dotnet publish -p PublishProfile=DefaultContainer and things will just work - the app that doesn't have the package/toggle the SDK-base publishing support won't load the PublishProfile and you'll be all set.

Medium term, we could add an additional Condition to the target, but I think it should be Condition="'$(EnableSdkContainerSupport)' == 'true' and '$(IsPublishable)' == true'", not just the IsPublishable check.

augustfengd commented 1 year ago

Ah! Thanks for your insight on what's to come. We've got a large number of projects that are containerized so we're thinking of having a separate .sln file just for those projects as an immediate solution for now.

Both medium and long-term solution sounds good though.

rainersigwald commented 1 year ago

we're thinking of having a separate .sln file just for those projects as an immediate solution for now.

Consider a solution filter (they're easier to maintain than a whole separate solution).

augustfengd commented 12 months ago

@rainersigwald, big thanks for the solution filter suggestion. It's working out great. :)

baronfel commented 10 months ago

One big downside to adding this constraint is that is blows up publishing for console applications. All users of this tech that made console apps now have to set IsPublishable to true or else their containerization doesn't work anymore. This is wrong - console apps do set/adhere to IsPublishable. What they don't do is import the Web SDK Publish Profiles.

baronfel commented 9 months ago

This one's been added and will be shipped in 8.0.100 RC2!