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

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

Closed
chrisoverzero opened this issue Apr 4, 2024 · 11 comments · Fixed by dotnet/sdk#40792
Labels
bug Something isn't working

Comments

@chrisoverzero
Copy link

chrisoverzero commented Apr 4, 2024

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
Copy link
Member

baronfel commented Apr 4, 2024

Oof, good spot.

@baronfel baronfel added the bug Something isn't working label Apr 4, 2024
@baronfel
Copy link
Member

baronfel commented Apr 4, 2024

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

@chrisoverzero
Copy link
Author

chrisoverzero commented Apr 4, 2024

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
Copy link
Author

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
Copy link
Member

baronfel commented May 3, 2024

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 dotnet/sdk#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
Copy link
Member

tmds commented May 3, 2024

<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
Copy link
Member

tmds commented May 3, 2024

It regressed in dotnet/sdk#34863.

@erikbra
Copy link

erikbra commented May 5, 2024

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
Copy link
Member

baronfel commented May 6, 2024

@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
Copy link

erikbra commented May 7, 2024

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
Copy link

erikbra commented May 9, 2024

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

baronfel pushed a commit to dotnet/sdk that referenced this issue May 15, 2024
… <Target>.dll" to ENTRYPOINT for Self Contained Builds (#40792)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants