-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Introduce PublishSelfContained #28714
Introduce PublishSelfContained #28714
Conversation
Pipeline failure is due to infrastructure.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something, but I think the logic can be simplified a lot to not need to even check if SelfContained
is a global property.
I didn't look at the tests much yet.
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets
Outdated
Show resolved
Hide resolved
@dsplaisted Responded to the feedback, please take a look again when you have availability. Thanks! |
src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildASelfContainedApp.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildASelfContainedApp.cs
Outdated
Show resolved
Hide resolved
src/Tests/dotnet-publish.Tests/GivenDotnetPublishPublishesProjects.cs
Outdated
Show resolved
Hide resolved
src/Tests/dotnet-publish.Tests/GivenDotnetPublishPublishesProjects.cs
Outdated
Show resolved
Hide resolved
src/Tests/dotnet-publish.Tests/GivenDotnetPublishPublishesProjects.cs
Outdated
Show resolved
Hide resolved
src/Tests/dotnet-publish.Tests/GivenDotnetPublishPublishesProjects.cs
Outdated
Show resolved
Hide resolved
@dsplaisted I implemented all of the requested test changes. (Thanks, I should be less sloppy with my tests). EDIT: About the below content, I saw you said we can change where property values.txt is recorded. Ill go ahead and look into that instead. I think the one thing you may object to / the only other thing worth looking at is that I added https://github.com/dotnet/sdk/pull/28714/files#diff-95fe3aebd6a9400190035038d2c2976fae64579c63dc1c5d4d912f7bc53790bbR478 this change to Obvious problems with this solution: |
src/Tests/dotnet-publish.Tests/GivenDotnetPublishPublishesProjects.cs
Outdated
Show resolved
Hide resolved
/// If the output path does not need to be hardcoded, use the overload requesting testRoot instead. | ||
/// </param> | ||
/// <returns></returns> | ||
public Dictionary<string, string> GetPropertyValues(string finalOutputPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative I was thinking of was to not write the file in a RID-specific path. IE update the injected target in TestProject.Create
to something like this:
string injectTargetContents =
$@"<Project>
<Target Name=`WritePropertyValues` BeforeTargets=`AfterBuild`>
<ItemGroup>
{propertiesElements}
</ItemGroup>
<WriteLinesToFile
File=`$(BaseIntermediateOutputPath)\$(Configuration)\$(TargetFramework)\PropertyValues.txt`
Lines=`@(LinesToWrite)`
Overwrite=`true`
Encoding=`Unicode`
/>
</Target>
</Project>";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. The PublishRuntimeIdentifier
tests need this too and I added it there. I think I should retarget both of these to 7.0.2xx. And then merge the publish RID PR. Then merge this with 7.0.2xx and change the test.
Co-authored-by: Daniel Plaisted <[email protected]>
…oject so changes from publishrid can be used instead
0bed169
to
a6e5ce0
Compare
…-publish-self-contained
The SDK has logic to specify SelfContained for PublishAot automatically. Except this doesn't apply when we're using msbuild to run the `Publish` target (`_IsPublishing` is not set): https://github.com/dotnet/sdk/blob/75184c7e763197fa2971954aa5baf70ffd188799/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L64-L83 This can lead to a bad failure mode because SelfContained is the thing that ensure ILC gets all the references to assemblies it needs. It will appear to work most of the time even without SelfContained (because ILCompiler packages carries a framework with it), but it will fail for things like ASP.NET. Enable back the logic that just errors out for this. If someone does `msbuild /t:publish` they'll get an error saying they need to also specify SelfContained instead of some weird failure mode. There was some discussion about this aspect in dotnet/sdk#28714.
…95496) The SDK has logic to specify SelfContained for PublishAot automatically. Except this doesn't apply when we're using msbuild to run the `Publish` target (`_IsPublishing` is not set): https://github.com/dotnet/sdk/blob/75184c7e763197fa2971954aa5baf70ffd188799/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L64-L83 This can lead to a bad failure mode because SelfContained is the thing that ensure ILC gets all the references to assemblies it needs. It will appear to work most of the time even without SelfContained (because ILCompiler packages carries a framework with it), but it will fail for things like ASP.NET. Enable back the logic that just errors out for this. If someone does `msbuild /t:publish` they'll get an error saying they need to also specify SelfContained instead of some weird failure mode. There was some discussion about this aspect in dotnet/sdk#28714.
Summary
Adds PublishSelfContained (PSC):
to publish self-contained by default but not build self-contained by default. You can of course also forward the property.
Examples
dotnet publish --sc=false -p:PublishSelfContained=true
-> Not Self Containeddotnet publish /p:SelfContained=false -p:PublishSelfContained=true
-> Not Self Containeddotnet publish -p:PublishSelfContained=true
-> Self Containeddotnet publish -p:PublishSelfContained=false
-> Not Self ContainedIn a project file:
dotnet publish
-> SelfContaineddotnet publish
-> Not SelfContaineddotnet build -p:PublishSelfContained=true
-> Not Self ContainedLong Form Explanation of changes
If you do
dotnet publish --no-sc -p:PublishSelfContained=true
,--no-sc
will win. Also true ifPublishSelfContained
is in the properties. Also true for other variants of--no-sc
like/p:SelfContained=false
. I got it to work the other way wherePSC
would win but we'd really, really prefer not to do it like that because it could break a lot of stuff.If you do
dotnet publish
and<SelfContained>false</SC>
in the project file butPublishSelfContained
istrue
in the project file or in the CLI, thenPSC
will win. Likewise, IfPSC
isfalse
butSC
istrue
in the project file,PSC
will win. Like stated above, ifSC
is stated in the command invocation,SC
will win.Blurbs about the logistical things
Had a large group discussion about getting
_IsPublishing
to work in MSBuild witht:/Publish
-- it's not possible. (We may not actually need_IsPublishing
to do this one, but we do for PublishRID and PublishRelease, so it's probably better to keep them aligned.) But we are going to do work to get it to align with VS. We'll need to make changes in their repo(s), but it shouldn't affect anything here, I'd hope. Not sure if we want to wait to merge until we can tell folks they can use this in VS.PublishSelfContained
will also imply a RID but only if we're publishing, since this property should only do anything if you're publishing. IIRC other publish properties already had a history of failing build without a RID.