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
175 stars 30 forks source link

PublishContainer Appends "dotnet <Target>.dll" to ENTRYPOINT for NativeAOT Builds #559

Closed chrisoverzero closed 1 month ago

chrisoverzero commented 2 months ago

When publishing a natively compiled application:

dotnet publish /t:PublishContainer

…without customizing the entry point or command, the produced image has an Entrypoint which contains extra parameters:

➜ docker inspect -f '{{ .Config.Entrypoint }}' success:latest
[/app/Success dotnet Success.dll]

I'd expect this to be only [/app/Success]. Since this is NativeAOT compiled, the publish process has chosen mcr.microsoft.com/dotnet/nightly/runtime-deps:8.0-jammy-chiseled-aot as the base image. There's no dotnet in this base image, and using the string "dotnet" as an argument doesn't seem meaningful.


SDK Version: 2.0.203 Operating System: macOS Sonoma 14.3 But I'm running these commands in: mcr.microsoft.com/dotnet/nightly/sdk:8.0-jammy-aot

baronfel commented 2 months ago

Oof, good spot.

baronfel commented 2 months ago

You can work around this by overriding the arguments, right? And what version of the SDK/package are you using?

chrisoverzero commented 2 months ago

Since I don't currently have applications which use the command line arguments, it's harmless to me. I only noticed it while trying to diagnose something else.

I'm sorry, that completely slipped my mind. I'm using SDK 8.0.203. (Updated OP.)

chrisoverzero commented 2 months ago

I'm a rank amateur at MSBuild, but I dove into the SDK repo, and I think the issue happens here:

<ItemGroup Label="AppCommand Assignment" Condition="'$(ContainerAppCommandInstruction)' != 'None'">
  <!-- For self-contained, invoke the native executable as a single arg -->
  <ContainerAppCommand Condition="'$(ContainerAppCommand)' == '' and '$(_ContainerIsSelfContained)' == 'true' and !$(_ContainerIsTargetingWindows)" Include="$(ContainerWorkingDirectory)/$(AssemblyName)$(_NativeExecutableExtension)" />
  <ContainerAppCommand Condition="'$(ContainerAppCommand)' == '' and '$(_ContainerIsSelfContained)' == 'true' and $(_ContainerIsTargetingWindows)" Include="$(AssemblyName)$(_NativeExecutableExtension)" />
  <!-- For non self-contained, invoke `dotnet` `app.dll` as separate args -->
  <ContainerAppCommand Condition="'$(ContainerAppCommand)' == ''" Include="dotnet;$(TargetFileName)" />
</ItemGroup>

…because the third ContainerAppCommand is unconditional with respect to self-containedness. I gather that it would be fixed by:

<ContainerAppCommand Condition="'$(ContainerAppCommand)' == '' and '$(_ContainerIsSelfContained)' == 'false'" Include="dotnet;$(TargetFileName)" />

Does that seem right? And if so, would Microsoft accept a PR like this to the SDK repo from outside the org. – that is, from me?

baronfel commented 2 months ago

First off, we would definitely accept a PR! We love PRs in this repo :) The containers work specifically had been highly driven by the community.

I agree with your analysis of where the issue is being introduced, but I'm going to tag in @tmds as well because the details of this are a bit fuzzy. This area was changed in https://github.com/dotnet/sdk/pull/33037, but I think it makes complete sense that we should not use the dotnet form for self-contained applications. We should definitely make sure to cover this scenario with more tests.

tmds commented 2 months ago
<ContainerAppCommand Condition="'$(ContainerAppCommand)' == ''" Include="dotnet;$(TargetFileName)" />

Yes, the problem is here. This was written up as: Condition="'$(ContainerAppCommand)' == ''" won't override an earlier assignment. But instead it adds additional items through Include to the ContainerAppCommand Item.

tmds commented 2 months ago

It regressed in https://github.com/dotnet/sdk/pull/34863.

erikbra commented 1 month ago

Looks like you guys beat me to this by 2 days 😆 - just hit this when trying to migrate from using a Dockerfile to using dotnet publish in my project (see mentioned bug report above).

I've never contributed to this repo before. Have you submitted a PR already, @tmds ? I don't see any in the links here, but you might have either way. Are there tests for the target files here? Could I contribute by writing a failing test, and then fixing?

If you're already on the case, I can just step quietly back into the corner, but please, if I can help in fixing this, could someone please give me a small clue on where to start?

baronfel commented 1 month ago

@erikbra the actual code for the SDK Containers feature does live in dotnet/sdk, as you saw - that repo has a developer guide here that you can use to build the repo.

The code change itself I would expect to be pretty small, and I'd want to see tests that cover the MSBuild logic in the TargetsTests file. The test I linked to show the usage of the older ContainerEntrypoint MSBuild Item, but it should be doable to create matching tests for ContainerAppCommand.

The main branch should be used as the target here, which is currently the branch used for the monthly .NET 9 preview releases. Once we have a PR that is merged we can backport it to the 8.0.400 release in August with a slash-command, and since the bug is kinda bad we'll probably ask .NET leadership to backport it to the other .NET 8 SDKs (8.0.1xx and 8.0.3xx).

erikbra commented 1 month ago

Thanks, @baronfel - I'll see if I can take a shot at it (but knowing myself, don't hold your breath all of you ;) )

erikbra commented 1 month ago

There we go, my attempt. It seems to solve the issue, but I'm a little worried, because the existing test, that tested on ContainerEntryPoint wasn't actually testing anything, due to a bug in the test. The test should possibly be fixed as well, and back-ported to the .NET 7 branch, as we don't set the ContainerEntryPoint anymore on .NET8+. I don't know if you should bother.

Here is the PR in the dotnet/sdk repo: #40792