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 not do a self-contained build #32277

Open
sbomer opened this issue May 5, 2023 · 13 comments
Open

PublishSelfContained should not do a self-contained build #32277

sbomer opened this issue May 5, 2023 · 13 comments
Milestone

Comments

@sbomer
Copy link
Member

sbomer commented May 5, 2023

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

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PublishSelfContained>true</PublishSelfContained>
  </PropertyGroup>

</Project>

Run dotnet publish.

The output in ./bin/Release/net8.0/linux-x64/publish is self-contained, as expected. What's unexpected is that the build output in /bin/Release/net8.0/linux-x64 is also self-contained. This is true even if I add <SelfContained>false</SelfContained>.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-NetSDK untriaged Request triage from a team member labels May 5, 2023
@nagilson nagilson removed the untriaged Request triage from a team member label May 9, 2023
@nagilson
Copy link
Member

nagilson commented May 9, 2023

Hm, that's a really good point. We're (the SDK and MSBuild team) going to talk about this a bit more tomorrow as there are some things I'm unsure of here

@nagilson nagilson added the needs team triage Requires a full team discussion label May 9, 2023
@nagilson
Copy link
Member

nagilson commented May 10, 2023

To our understanding, the publish output relies on the build output. If we wanted to make the build not SelfContained, then we would have to build a 2nd time in SelfContained for the publish to be SelfContained. (Or re-write publish.)

@marcpopMSFT marcpopMSFT removed the needs team triage Requires a full team discussion label May 10, 2023
@marcpopMSFT marcpopMSFT added this to the Backlog milestone May 10, 2023
@marcpopMSFT
Copy link
Member

The publish requires a build happening first. We could potentially not copy the buld outputs and instead copy from Intermediate output direct to publish but I think the only benefit is a size benefit. CC @dsplaisted if he has other thoughts on this.

@dsplaisted
Copy link
Member

Yes, the self-contained publish depends on a self-contained build. We could probably try to untangle that, but I suspect it would be tricky.

@sbomer
Copy link
Member Author

sbomer commented May 10, 2023

It makes sense that publish depends on having built the user code - but I don't think it makes sense to copy the self-contained dependencies to both the build and publish folders.

The size issue is significant and was the motivation for #32272. I'd argue that there's also a correctness issue - if I specifically set PublishSelfContained, I expect that the build output is not self-contained. But if I do dotnet publish then dotnetn build -c Release, the self-contained build output is left over from running dotnet publish. And I don't think it would be good to fix that by making dotnet build -c Release clean the output folder - those files shouldn't be there in the first place.

@richlander

@richlander
Copy link
Member

@agocke

@ericstj
Copy link
Member

ericstj commented May 11, 2023

Isn't the whole point of PublishSelfContained to be different from SelfContained? The way I imagine this working is that build is not self-contained. Then during publish the app would get a new runtimeconfig, a copy of the runtime pack, specialization of runtimeTargets for the specific runtime - as well as anything else during build that depends on SelfContained.

@agocke
Copy link
Member

agocke commented May 12, 2023

Yeah, the major feature request from my side, and the reason why I wanted to change the behavior of SelfContained to never occur during build, is that self-contained build can be untenable from an output size perspective. Requiring a self-contained build basically requires a minimum output size of the whole FX (~80 MB) for every project. That starts to add up really quickly.

Regardless of what we call the feature, we need a system that prevents a self-contained framework from being copied during build, but produce a self-contained output during publish.

@dsplaisted
Copy link
Member

The way I imagine this working is that build is not self-contained. Then during publish the app would get a new runtimeconfig, a copy of the runtime pack, specialization of runtimeTargets for the specific runtime - as well as anything else during build that depends on SelfContained.

Yes, that is the way it would ideally work, but that's not how it works and it might be hard or risky to try to change it. The behavior we have now with PublishSelfContained is this:

  • If you run dotnet build, you get a framework-dependent build
  • If you run dotnet publish, you get a self-contained publish, and implicitly also get a self-contained build

So when you do a build you don't use up 80MB of space, though when you do a publish you use it twice. By default the implicit self-contained build will be in a different path than the normal build, though if you are using the new artifacts output path format, they would use the same path and overwrite each other. That's something we may want to look at addressing in the new output path format.

As for why it might be hard to fix this, there may be a bunch of different things that depend on whether the build is SelfContained or not, which would have to be changed to support different values for build and publish. For example, whether the runtime framework version uses the latest available patch by default depends on whether the build is self-contained or not. This would probably have to be split up into runtime framework version values, one for build and one for publish. I'm not sure what else there is that might also be impacted.

@nagilson
Copy link
Member

@ericstj

Isn't the whole point of PublishSelfContained to be different from SelfContained?

The difference is in the publish artifacts, not the build artifacts. (This is mildly semantic, and I agree it'd be best if the build also wasn't but see @dsplaisted's and my comment above.)

The main purpose for this was so it could be added to the project file, so you could run dotnet build and have a not-self-contained app for your dev loop, and then run dotnet publish without having to specify SelfContained (note you don't have to specify -C release anymore either ;) )

@sbomer

But if I do dotnet publish then dotnet build -c Release, the self-contained build output is left over from running dotnet publish

That's a good point. I did a test to be check that the output of build is still SelfContained and publish is not and it works: but yeah, the older stuff still remains and I don't think we have a good answer to dealing with that outside changing the build behavior.

@dsplaisted
Copy link
Member

I think we could change the build output folder to be hidden under the intermediate output path when doing a publish in this scenario. It would still take up the same amount of disk space, but it wouldn't pollute your normal output path.

@agocke
Copy link
Member

agocke commented May 16, 2023

The disk size (and copy time) is prohibitive. We really need a way to stop the layout from happening.

@agocke
Copy link
Member

agocke commented May 17, 2023

I should mention, this would be solved completely by my proposal to make SelfContained only occur during Publish.

If we decide to go that route (and take the breaking change), we would change the behavior of PublishSelfContained as well (arguably we could delete PublishSelfContained entirely, as it is the same thing).

As it is, it feels like the PublishSelfContained design is broken. It doesn't achieve what we wanted it to achieve (only create a self-contained layout during Publish) and now fixing it would be another breaking change.

The original proposal of obsoleting build-self-contained is looking even more attractive to me now.

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

7 participants