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

PublishSelfContained should work with /t:Publish #32272

Open
eerhardt opened this issue May 4, 2023 · 14 comments
Open

PublishSelfContained should work with /t:Publish #32272

eerhardt opened this issue May 4, 2023 · 14 comments
Assignees

Comments

@eerhardt
Copy link
Member

eerhardt commented May 4, 2023

Description

When publishing a trimmed app, I am currently getting an error:

Microsoft.NET.ILLink.targets(196,5): error NETSDK1102: Optimizing assemblies for size is not supported for the selected publish configuration. Please ensure that you are publishing a self-contained app. [C:\DotNetTest\SelfContainedTest\SelfContainedTest.csproj]

Even though I'm setting PublishSelfContained=true in my .csproj. I am also setting SelfContained=false because I don't want all of the runtime copied into my build output directory. I have many applications that I'm publishing (test apps in dotnet/aspnetcore) and each application is taking up ~100 MB in the bin directory. This is unnecessary bloat.

Reproduction Steps

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <PublishTrimmed>true</PublishTrimmed>
    <PublishSelfContained>true</PublishSelfContained>
    <RuntimeIdentifier>win-x64</RuntimeIdentifier>

    <!-- Set SelfContained=false to prevent the whole runtime from being copied into the build output directory -->
    <SelfContained>false</SelfContained>
  </PropertyGroup>

</Project>

Running dotnet msbuild /t:Publish on that project will fail. It will also fail just for:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <PublishTrimmed>true</PublishTrimmed>
    <PublishSelfContained>true</PublishSelfContained>
  </PropertyGroup>

</Project>

Expected behavior

I expect both apps above to publish successfully.

Actual behavior

Both of them produce the above error.

Regression?

No response

Known Workarounds

Explicitly specify SelfContained=true, but that causes bloat (will eventually be GBs) in my bin directory.

Configuration

No response

Other information

No response

@ghost ghost added the untriaged Request triage from a team member label May 4, 2023
@ghost
Copy link

ghost commented May 4, 2023

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

When publishing a trimmed app, I am currently getting an error:

Microsoft.NET.ILLink.targets(196,5): error NETSDK1102: Optimizing assemblies for size is not supported for the selected publish configuration. Please ensure that you are publishing a self-contained app. [C:\DotNetTest\SelfContainedTest\SelfContainedTest.csproj]

Even though I'm setting PublishSelfContained=true in my .csproj. I am also setting SelfContained=false because I don't want all of the runtime copied into my build output directory. I have many applications that I'm publishing (test apps in dotnet/aspnetcore) and each application is taking up ~100 MB in the bin directory. This is unnecessary bloat.

Reproduction Steps

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <PublishTrimmed>true</PublishTrimmed>
    <PublishSelfContained>true</PublishSelfContained>
    <RuntimeIdentifier>win-x64</RuntimeIdentifier>

    <!-- Set SelfContained=false to prevent the whole runtime from being copied into the build output directory -->
    <SelfContained>false</SelfContained>
  </PropertyGroup>

</Project>

Running dotnet msbuild /t:Publish on that project will fail. It will also fail just for:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <PublishTrimmed>true</PublishTrimmed>
    <PublishSelfContained>true</PublishSelfContained>
  </PropertyGroup>

</Project>

Expected behavior

I expect both apps above to publish successfully.

Actual behavior

Both of them produce the above error.

Regression?

No response

Known Workarounds

Explicitly specify SelfContained=true, but that causes bloat (will eventually be GBs) in my bin directory.

Configuration

No response

Other information

No response

Author: eerhardt
Assignees: -
Labels:

untriaged, area-Tools-ILLink

Milestone: -

@sbomer
Copy link
Member

sbomer commented May 4, 2023

Looking at the way PublishSelfContained was implemented:

<PropertyGroup Condition="'$(_IsPublishing)' == 'true' and ('$(PublishSelfContained)' == 'true' or '$(PublishSelfContained)' == 'false')">
<SelfContained>$(PublishSelfContained)</SelfContained>
</PropertyGroup>

It looks like it just sets SelfContained based on the value of PublishSelfContained when publishing, and all of the downstream publish logic in the SDK continues to use SelfContained. So ILLink won't be the only tool with this problem.

I think that the intention is for this to be resolved via #30038 (so you won't have to explicitly set SelfContained=false to get the desired behavior). There might be a way to work around this by setting SelfContained conditionally only (maybe based on _IsPublishing.

But I think this is currently by design. cc @nagilson

@nagilson nagilson transferred this issue from dotnet/runtime May 4, 2023
@nagilson nagilson removed the untriaged Request triage from a team member label May 4, 2023
@nagilson nagilson self-assigned this May 4, 2023
@agocke
Copy link
Member

agocke commented May 4, 2023

Seems like there's an open question here: is PublishSelfContained set downstream of SelfContained, or vice versa? If the former, there is a bug here.

@nagilson
Copy link
Member

nagilson commented May 4, 2023

PublishSelfContained does not work with MSBuild publish, you need to do dotnet publish or forward the property _IsPublishing to MSBuild publish.

@sbomer
Copy link
Member

sbomer commented May 5, 2023

It looks like there are larger issues here even if you take the "recommended" path and set _IsPublishing. I don't think there's a way to avoid the self-contained build output today: #32277

@eerhardt
Copy link
Member Author

eerhardt commented May 5, 2023

PublishSelfContained does not work with MSBuild publish

That seems like a mistake.

you need to do dotnet publish

When automating the build of a large repo, it doesn't make sense to jump in and out of MSBuild to dotnet publish. I'm already in MSBuild, and I just want to run the Publish target on a set of projects. For example:

https://github.com/dotnet/runtime/blob/c31fda12adf036670ea5c23bce5f066bfd948ebf/eng/testing/linker/trimmingTests.targets#L132-L134

or forward the property _IsPublishing to MSBuild publish.

If we want users to forward/check/set that property, we should give it an official name without an _ at the front

@eerhardt
Copy link
Member Author

eerhardt commented May 5, 2023

I have many applications that I'm publishing (test apps in dotnet/aspnetcore) and each application is taking up ~100 MB in the bin directory. This is unnecessary bloat.

Just an example of this in dotnet/runtime:

https://github.com/dotnet/runtime/tree/main/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections

This one folder has 20 trimming tests. When I run them (from the root of dotnet/rutnime):

  1. .\build.cmd libs+clr -rc Release
  2. .\dotnet.cmd test .\src\libraries\System.Text.Json\tests\System.Text.Json.Tests\TrimmingTests\

and inspect the folder that contains these test projects: artifacts\bin\trimmingTests\projects\System.Text.Json.TrimmingTests:

image

@nagilson
Copy link
Member

nagilson commented May 5, 2023

That seems like a mistake.

@eerhardt I hear you. We tried very hard to get it to work for MSBuild publish. It also doesn't work inside of VS outside of the context of running a dotnet publish.

There are several reasons we landed here. We could technically create some logic in MSBuild to make it work, but it wouldn't work very well. Adding this to MSBuild would break several key philosophies of MSBuild. One of the problems is that people need to be able to write their own custom publish targets or call the Publish target in their build whenever. We don't know at the very beginning of the build if someone is going to inject a publish target. But we need the _IsPublishing property to be defined by the beginning of the build/publish, as Configuration depends on it as well as properties like PublishSelfContained and PublishRuntimeIdentifier, and for some of these cases any time before the beginning of the build/publish is too late for it to work. @rainersigwald would be able to provide better context.

One of the only ways (if the only) to get around that is to run the build/publish twice, so you can see in the 1st time if publish is ever invoked and commmunicate that to the 2nd run. But that's pretty bad.

If we want users to forward/check/set that property, we should give it an official name without an _ at the front

This is a good suggestion and we could do that. But it sounds like there are problems even if that's used, which is unfortunate. I am trying to think if there is any other solution for this scenario.

@sbomer sbomer changed the title PublishTrimmed should respect PublishSelfContained PublishSelfContained should work with /t:Publish Dec 2, 2023
@sbomer
Copy link
Member

sbomer commented Dec 2, 2023

dotnet/runtime#95496 is another example where PublishSelfContained not working with msbuild /t:Publish causes a bad UX. @eerhardt I took the liberty to rename this issue - feel free to adjust if you disagree with the rename.

@eerhardt
Copy link
Member Author

eerhardt commented Dec 4, 2023

@marcpopMSFT @dsplaisted @baronfel - is there any chance this can be fixed in the near future? I think this behavior is causing a lot of confusion - and extra work debugging failures because of this.

@marcpopMSFT
Copy link
Member

See Noah's comment above. It's particularly challenging to make specific SDK properties work when targets are executed directly through msbuild target invocation. We can have another discussion to see if there are better options but we don't want to maintain that separation between MSBuild and SDK.

CC @nagilson @baronfel @rainersigwald

@eerhardt
Copy link
Member Author

eerhardt commented Dec 5, 2023

I think the minimum we could do is what I suggested in dotnet/runtime#95496 (comment)

Check both SelfContained and PublishSelfContained to ensure ILC gets all the references to assemblies it needs.

This seems like it would fix most (if not all?) of the issue here.

@sbomer
Copy link
Member

sbomer commented Jan 4, 2024

We can have another discussion to see if there are better options

It seems like the core challenge is that dotnet publish must set defaults (like Configuration) early in the build, early enough for the build output to take these into account.

Doing this via <Target Name="Publish" DepondsOnTargets="Build"> seems fundamentally broken, because the Build target doesn't know whether the Publish target is going to run, without relying on hacks like _IsPublishing.

Would it solve some of these issues if the Publish target instead invoked <MSBuild Targets="Build">, passing along whatever defaults it needs?

@Sergio0694
Copy link
Contributor

This is also affecting WinAppSDK, specifically this broke with the .NET 9 SDK. In order to publish something using WinAppSDK (eg. a WinRT component, or a WinUI 3 app) you need msbuild, because WinAppSDK depends on some .targets fluff from Desktop MSBuild, and trying to just do dotnet publish will fail to build. However, trying to do:

msbuild .\Win2DWinRTComponentTest.csproj /restore -t:publish /p:Configuration=Release /p:Platform=x64 /p:RuntimeIdentifier=win-x64

On .NET 9 fails to build with that same error mentioned here (can't optimize for size). Adding SelfContained=true fixes the issue, but this still feels like a regression (and I could see devs not really knowing what to do and how to fix the issue) 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants