Don't flow self contained command-line option to referenced projects #6924

dsplaisted · 2021-10-07T22:47:22Z

Context

In .NET 6, if you run a command such as dotnet build -r win-x64, you will get the following error:

C:\Program Files\dotnet\sdk\6.0.100-rtm.21474.29\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(1105,5): warning NETSDK1179: One of '--self-contained' or '--no-self-contained' options are required when '--runtime' is used. [C:\git\repro\ConsoleTest\ConsoleTest.csproj]

However, if you follow the suggestion and change it to dotnet build -r win-x64 --self-contained, then you will likely get an error such as the following if there are any referenced projects:

C:\Program Files\dotnet\sdk\6.0.100-rc.2.21501.4\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(150,5): error NETSDK1031: It is not supported to build or publish a self-contained application without specifying a RuntimeIdentifier. You must either specify a RuntimeIdentifier or set SelfContained to false. [C:\Users\brady\source\TodoApp\TodoApp.API\TodoApp.API.csproj]

This happens because

Global properties (ie those specified on the command line) flow to referenced projects by default
We normally stop the RuntimeIdentifier from flowing to referenced projects
We don't currently stop the SelfContained property from flowing to referenced projects
So the referenced project gets built with SelfContained set to true but no RuntimeIdentifier, which is an error

Changes Made

If --self-contained is specified on the command-line, and the RuntimeIdentifier is blocked from flowing to referenced projects, then we also block the SelfContained property from flowing to referenced projects.

Note that this treats --self-contained differently from /p:SelfContained=true, as there is at least one project that relies on /p:SelfContained=true flowing across project references, and they set RuntimeIdentifier in their .props files if SelfContained is true.

Testing

I will add automated tests to the SDK repo for this scenario, and test locally with those tests and the changes in this PR before merging.

Notes

FYI @richlander @sfoslund @marcpopMSFT, I think we should take this for 17.0 GA

rainersigwald

I talked to Daniel about this earlier. I am nervous about taking a change like this so late, but I agree that the impact given the warning is bad.

I wanted an escape hatch at minimum, and this provides that pretty well by checking _CommandLineDefinedSelfContained and allowing specifying it as a property instead.

So I'm ok with this (if not particularly enthused).

benvillalobos · 2021-10-07T23:24:41Z

If _CommandLineDefinedSelfContained is the expected escape hatch, why mark it with an underscore?

I'm a little confused at why there's a distinction between --self-contained and /p:SelfContained=true, but I won't block on it.

Also I'm not familiar with the self contained flag, is this a new concept or has it been around for a while?

dsplaisted · 2021-10-08T01:25:17Z

@benvillalobos More background than you probably want to dig into is available here: https://github.com/dotnet/designs/blob/main/accepted/2021/architecture-targeting.md

We wouldn't expect someone to define _CommandLineDefinedSelfContained themselves, the CLI automatically sets it if you use the --self-contained or --no-self-contained options: https://github.com/dotnet/sdk/blob/f72371bb4e6b5d3394ec2c261cf8c43a6697b815/src/Cli/dotnet/CommonOptions.cs#L115-L125

So the escape hatch is basically to use /p:SelfContained=true instead of --self-contained.

@Forgind We shouldn't merge this yet until I've had a chance to test it. I plan on writing automated tests in dotnet/sdk, but we won't be able to enable them in CI until the fix merges and flows. But I should still run them manually first.

Forgind · 2021-10-08T14:45:02Z

Ok; sounds good. I didn't actually look at the change, just saw three approvals and added the label. Feel free to add it after testing.

dsplaisted · 2021-10-08T22:31:33Z

I tried testing this, and this isn't ready.

First the good news: The impact of the bug is less than I thought. It only affects cases where the referenced project has an OutputType of Exe (or similar). For Library projects, the fact that SelfContained flows through doesn't really affect anything.

However, once I made the change to prevent the SelfContained value from flowing, I got this error:

NETSDK1150: The referenced project '..\TodoApp.API\TodoApp.API.csproj' is a non self-contained executable. A non self-contained executable cannot be referenced by a self-contained executable. For more information, see https://aka.ms/netsdk1150

Perhaps in this case both the RuntimeIdentifier and the SelfContained value should flow to the referenced Exe project, for example by setting the IsRidAgnostic metadata on the project build metadata to false if the output type is Exe or similar. However, it's still not simple to do that, as the NETSDK1150 check is based on the value of SelfContained from the GetTargetFrameworks call, where we're unconditionally removing SelfContained from the global properties.

I'll try to think about this some more, but it seems like we probably won't take a fix for this in 17.0.

dsplaisted · 2021-10-12T19:45:46Z

I've done some more work and I think I have a pretty good idea of how we should solve this. It involves changes to MSBuild (now included in this PR) as well as to the SDK, in dotnet/sdk#21986.

I'd like some feedback on whether that's the right direction. It will be a breaking change (see the breaking change document in the SDK PR), so I think we probably would want to target these fixes for 6.0.200 rather than 6.0.100 GA.

rainersigwald · 2021-10-15T20:06:06Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

-      <!-- If the project is RID agnostic, undefine the RuntimeIdentifier property to avoid another evaluation. -->
-      <AnnotatedProjects Condition="'@(AnnotatedProjects)' == '%(Identity)' and '%(AnnotatedProjects.IsRidAgnostic)' == 'true'">
-        <UndefineProperties>%(AnnotatedProjects.UndefineProperties);RuntimeIdentifier</UndefineProperties>


Is it possible to keep these and respect them if they're manually implemented? We documented IsRidAgnostic in

msbuild/documentation/ProjectReference-Protocol.md

Lines 71 to 73 in a7d5790

* This should return either

* a string of the form `TargetFramework=$(NearestTargetFramework);ProjectHasSingleTargetFramework=$(_HasSingleTargetFramework);ProjectIsRidAgnostic=$(_IsRidAgnostic)`, where the value of `NearestTargetFramework` will be used to formulate `TargetFramework` for the following calls and the other two properties are booleans, or

* an item with metadata `DesiredTargetFrameworkProperties` (key-value pairs of the form `TargetFramework=net46`), `HasSingleTargetFramework` (boolean), and `IsRidAgnostic` (boolean).

Do you think anyone would have actually used that? It seems like it would be hard to hook into. Searching GitHub seems to give about 7 results, though it's not actually displaying them for me correctly so I can't tell if they're real.

I'm still worried about this. It looks like at least CsWinRT uses it: https://github.com/microsoft/CsWinRT/blob/3ec398157f17baedec341de5d852747d420b9ecf/nuget/Microsoft.Windows.CsWinRT.Authoring.targets#L162

Changed significantly since review

dsplaisted · 2021-11-18T01:30:47Z

@rainersigwald I've retargeted this to vs17.1, and I think it (together with dotnet/sdk#21986) is ready for review again

rainersigwald · 2021-11-18T16:30:44Z

Our primary 17.1 branch is main so I retargeted for you again. Reviewing.

rainersigwald · 2021-11-18T16:39:34Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

-      <!-- If the project is RID agnostic, undefine the RuntimeIdentifier property to avoid another evaluation. -->
-      <AnnotatedProjects Condition="'@(AnnotatedProjects)' == '%(Identity)' and '%(AnnotatedProjects.IsRidAgnostic)' == 'true'">
-        <UndefineProperties>%(AnnotatedProjects.UndefineProperties);RuntimeIdentifier</UndefineProperties>


I'm still worried about this. It looks like at least CsWinRT uses it: https://github.com/microsoft/CsWinRT/blob/3ec398157f17baedec341de5d852747d420b9ecf/nuget/Microsoft.Windows.CsWinRT.Authoring.targets#L162

rainersigwald · 2021-11-18T19:45:22Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

-        <!-- indicate to caller that project is RID agnostic so that a global property RuntimeIdentifier value can be removed -->
-        <IsRidAgnostic>false</IsRidAgnostic>
-        <IsRidAgnostic Condition=" '$(RuntimeIdentifier)' == '' and '$(RuntimeIdentifiers)' == '' ">true</IsRidAgnostic>
+        <AcceptsRuntimeIdentifier>@(_TargetFrameworkInfo->'%(AcceptsRuntimeIdentifier)')</AcceptsRuntimeIdentifier>


This would be multivalued since it's a transform over a list, right? Is that what you expect? I would expect maybe something like AnyHasMetadataValue('AcceptsRuntimeIdentifier', 'true').

beatcracker · 2021-11-24T18:33:36Z

I'm looking into this due to dotnet/sdk/issues/10566, and I've found that just checking for _CommandLineDefinedRuntimeIdentifier works for my simple case (no rid in the projects, self-contained / rid are supplied via the commandline):

<IsRidAgnostic Condition=" '$(RuntimeIdentifier)' == '' and '$(RuntimeIdentifiers)' == '' and $(_CommandLineDefinedRuntimeIdentifier) == '' ">true</IsRidAgnostic>

Obviously, I've just skimmed the target source and I have no deep knowledge, but I hope this helps.

Forgind · 2022-04-20T21:36:24Z

@dsplaisted, are you going to be able to get around to this soon? Or should we close it for now or mark it as a draft?

dsplaisted · 2022-04-25T17:41:49Z

@Forgind Is there a way to convert a pull request back to a draft PR?

Fixes dotnet/sdk#21677

Also always treat SelfContained the same as RuntimeIdentifier as far as whether the properties should be allowed to flow across project references

dsplaisted · 2022-08-12T14:37:56Z

@rainersigwald @dotnet/msbuild I've updated and rebased this PR, and I believe it is now ready for review again. I switched back to using IsRidAgnostic instead of creating AcceptsRuntimeIdentifier, which should help make this more compatible.

src/Tasks/Microsoft.Common.CurrentVersion.targets

Forgind · 2022-08-12T21:49:58Z

src/Tasks/Microsoft.Common.CrossTargeting.targets

-        <!-- indicate to caller that project is RID agnostic so that a global property RuntimeIdentifier value can be removed -->
-        <IsRidAgnostic>false</IsRidAgnostic>
-        <IsRidAgnostic Condition=" '$(RuntimeIdentifier)' == '' and '$(RuntimeIdentifiers)' == '' ">true</IsRidAgnostic>
+        <IsRidAgnostic>@(_TargetFrameworkInfo->'%(IsRidAgnostic)')</IsRidAgnostic>


Should GetTargetFrameworks depend on GetTargetFrameworksWithPlatformForSingleSingleTargetFramework explicitly?

That's what it does for single-targeted projects:

msbuild/src/Tasks/Microsoft.Common.CurrentVersion.targets

Lines 1889 to 1890 in 5d102ae

<Target Name="GetTargetFrameworks"

DependsOnTargets="GetTargetFrameworksWithPlatformForSingleTargetFramework"

For multi-targeted projects it has to do inner builds for each target framework, which the GetTargetFrameworksWithPlatformFromInnerBuilds target handles.

Forgind · 2022-08-12T21:50:55Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -1779,7 +1779,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
        BuildInParallel="$(BuildInParallel)"
        Properties="%(_MSBuildProjectReferenceExistent.SetConfiguration); %(_MSBuildProjectReferenceExistent.SetPlatform)"
        ContinueOnError="!$(BuildingProject)"
-        RemoveProperties="%(_MSBuildProjectReferenceExistent.GlobalPropertiesToRemove);TargetFramework;RuntimeIdentifier$(_GlobalPropertiesToRemoveFromProjectReferences)"
+        RemoveProperties="%(_MSBuildProjectReferenceExistent.GlobalPropertiesToRemove);TargetFramework;RuntimeIdentifier;SelfContained;$(_GlobalPropertiesToRemoveFromProjectReferences)"


It was previously RuntimeIdentifier$(_GlobalPropertiesToRemoveFromProjectReferences) (no semicolon)
That looks like a bug to me, but I just wanted to make sure that's intentional 🙂

Good point. It does seem like it might have been a bug before. Now there may be extra semicolons, but that should be OK, right? The MSBuild task should just ignore the extra ones I think.

Forgind · 2022-08-12T21:51:43Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+             Generally this value will come from the IsRidAgnostic property set by the .NET SDK.  If that's not set, then the fallback logic here will be that the project
+             is RID agnostic if it doesn't have RuntimeIdentifier or RuntimeIdentifiers properties set. -->
+        <IsRidAgnostic>$(IsRidAgnostic)</IsRidAgnostic>
+        <IsRidAgnostic Condition=" '$(IsRidAgnostic)' == '' and '$(RuntimeIdentifier)' == '' and '$(RuntimeIdentifiers)' == '' ">false</IsRidAgnostic>


Should this be true and the next one false?

+1, it looks like it will set IsRidAgnostic to false when there are no runtime identifiers

Yes, thanks for catching this.

Forgind · 2022-08-12T21:53:14Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -1941,6 +1942,16 @@ Copyright (C) Microsoft Corporation. All rights reserved.
        <TargetPlatformMonikers>$(TargetPlatformMoniker)</TargetPlatformMonikers>
        <TargetPlatformMonikers Condition="'$(TargetPlatformMoniker)' == ''">None</TargetPlatformMonikers>
        <AdditionalPropertiesFromProject>$(_AdditionalTargetFrameworkInfoProperties)</AdditionalPropertiesFromProject>
+


Does _TargetFrameworkInfo ever have more than just $(TargetFramework) included in it?

Yes, it does for multi-targeted projects, which will run this target for each target framework and combine the results.

joeloff · 2022-08-18T19:44:59Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -1795,7 +1795,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
        Targets="GetTargetFrameworks"
        BuildInParallel="$(BuildInParallel)"
        ContinueOnError="!$(BuildingProject)"
-        RemoveProperties="%(_MSBuildProjectReferenceExistent.GlobalPropertiesToRemove);TargetFramework;RuntimeIdentifier;Platform;Configuration$(_GlobalPropertiesToRemoveFromProjectReferences)"
+        RemoveProperties="%(_MSBuildProjectReferenceExistent.GlobalPropertiesToRemove);TargetFramework;RuntimeIdentifier;SelfContained;Platform;Configuration$(_GlobalPropertiesToRemoveFromProjectReferences)"


Do we need a ; between Configuration and $(_GlobalPropertiesToRemoveFromProjectReferences)?

Looking at all usages of _GlobalPropertiesToRemoveFromProjectReferences in MSBuild, it seems that it is always expected to have a ; in front of it. That said, I think adding a ; is better anyway.

Forgind · 2022-08-18T22:10:11Z

src/Tasks/SetRidAgnosticValueForProjects.cs

+                .Select(p =>
+                {
+                    var hasSingleTargetFrameworkString = p.GetMetadata("HasSingleTargetFramework");
+                    if (!ConversionUtilities.ValidBooleanFalse(hasSingleTargetFrameworkString))


Why did you choose !false rather than true? Could it be null here? If so, this seems like wrong behavior.

If the value is not set I want this to be a no-op. That seems the safest.

For projects using the .NET SDK, this should always be set. For other types of projects it might not be.

Forgind · 2022-08-18T22:11:59Z

src/Tasks/SetRidAgnosticValueForProjects.cs

+                    var hasSingleTargetFrameworkString = p.GetMetadata("HasSingleTargetFramework");
+                    if (!ConversionUtilities.ValidBooleanFalse(hasSingleTargetFrameworkString))
+                    {
+                        // No change to item, it should already have a single-valued IsRidAgnostic value


Should we have a Debug.Assert here or something?

Debug.Assert gets compiled out in release builds, so that would be pretty unlikely to catch anything anyway, wouldn't it be?

It is, but if something goes wrong, we'd probably try to debug with a debug build, so I thought it might be helpful. Not a big deal if you don't want to add it.

Forgind · 2022-08-18T22:54:19Z

src/Tasks/SetRidAgnosticValueForProjects.cs

+                    }
+                    var updatedItem = new TaskItem(p);
+
+                    var nearestTargetFramework = p.GetMetadata("NearestTargetFramework");


Are projects actually expected to have all of this information? I feel like this relies on projects carrying around a lot of (generally unimportant) baggage. Not saying that doesn't happen, but it's a bit surprising to me.

What information are you referring to? These are part of the Project Reference Protocol, or in the case of NearestTargetFramework, set by the GetReferenceNearestTargetFrameworkTask task which runs just before this one.

I wasn't aware of NearestTargetFramework, but that makes sense. I was also surprisedby IsRidAgnostic being an array of IsRidAgnostic values with one per target framework. I'd expected (and the Project Reference Protocol seems to say) that it's just a boolean. It seems like you're changing the type in this task from boolean[] to boolean?

Forgind · 2022-08-18T22:56:20Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+    <ItemGroup>
+      <AnnotatedProjects Remove="@(AnnotatedProjects)" />
+      <AnnotatedProjects Include="@(UpdatedAnnotatedProjects)" />
+      <UpdatedAnnotatedProjects Remove="@(UpdatedAnnotatedProjects)" />


Why is this line necessary?

It just clears it out so that it's not taking up memory anymore. It's a pretty clunky pattern for when you want to have a task that updates item metadata.

Forgind

I'm least confident about the IsRidAgnostic array, and I haven't had a chance to manually test this myself, but I don't know of anything broken in here, you said you wanted it in this week, and I have something in five minutes 🙂

Fixes #7995 Context #6924 changed how IsRidAgnostic works. Normally, the .NET SDK should set the IsRidAgnostic property (added in dotnet/sdk#21986). But when using an older version of the .NET SDK, there is fallback logic in the GetTargetFrameworksWithPlatformForSingleTargetFramework target to replicate the previous logic. However, this fallback logic was incorrect, as it was setting item metadata, but reading from the property to determine whether to set it to the default value. So if the property wasn't set, the IsRidAgnostic metadata would always be false. Testing Manual testing

rainersigwald previously approved these changes Oct 7, 2021

View reviewed changes

sfoslund approved these changes Oct 7, 2021

View reviewed changes

benvillalobos approved these changes Oct 7, 2021

View reviewed changes

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Oct 7, 2021

Forgind removed the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Oct 8, 2021

dsplaisted mentioned this pull request Oct 12, 2021

Update handling of SelfContained and RuntimeIdentifier properties when specified on the command line dotnet/sdk#21986

Merged

rainersigwald reviewed Oct 15, 2021

View reviewed changes

dsplaisted changed the base branch from vs17.0 to vs17.1 November 18, 2021 01:26

dsplaisted force-pushed the stop-selfcontained-flow branch 2 times, most recently from 0255bd1 to 32755de Compare November 18, 2021 01:29

rainersigwald changed the base branch from vs17.1 to main November 18, 2021 16:30

rainersigwald reviewed Nov 22, 2021

View reviewed changes

dsplaisted marked this pull request as draft April 25, 2022 18:11

dsplaisted mentioned this pull request May 3, 2022

Publishing standalone app (C#) fails with C++/CLI dependency dotnet/sdk#12345

Open

nagilson mentioned this pull request Jul 20, 2022

Use the Implicit SDK RID for RID-Specific Apps by Default dotnet/sdk#26143

Merged

4 tasks

dsplaisted added 3 commits July 21, 2022 20:45

Don't flow self contained command-line option to referenced projects

9c56b5e

Fixes dotnet/sdk#21677

Switch from IsRidAgnostic metadata to AcceptsRuntimeIdentifier property

f849bea

Also always treat SelfContained the same as RuntimeIdentifier as far as whether the properties should be allowed to flow across project references

Use IsRidAgnostic instead of AcceptsRuntimeIdentifier

59bf77d

dsplaisted force-pushed the stop-selfcontained-flow branch from 32755de to 59bf77d Compare August 12, 2022 00:50

dsplaisted marked this pull request as ready for review August 12, 2022 14:35

Forgind reviewed Aug 12, 2022

View reviewed changes

Add task to handle IsRidAgnostic metadata for multitargeted projects

ab2662a

dsplaisted requested review from joeloff and a team August 18, 2022 19:33

joeloff reviewed Aug 18, 2022

View reviewed changes

Apply code review feedback

512a065

Forgind reviewed Aug 18, 2022

View reviewed changes

Forgind approved these changes Aug 19, 2022

View reviewed changes

dsplaisted merged commit e1716a1 into dotnet:main Aug 19, 2022

rainersigwald mentioned this pull request Sep 22, 2022

Update vs to Version 17.4.0 Preview 2.0 from PREVIEW 1 cause errors in publish worker service #7995

Closed

dsplaisted mentioned this pull request Sep 22, 2022

Fix IsRidAgnostic fallback logic #8002

Merged

nagilson mentioned this pull request Oct 18, 2022

[7.0.2xx] Change Global Property Flow to Not Flow Publish* Properties dotnet/sdk#28645

Open

rainersigwald mentioned this pull request Nov 14, 2022

MSBuild version 17.4.0 getting CS2012 files in use by another process errors #8154

Open

	* This should return either
	* a string of the form `TargetFramework=$(NearestTargetFramework);ProjectHasSingleTargetFramework=$(_HasSingleTargetFramework);ProjectIsRidAgnostic=$(_IsRidAgnostic)`, where the value of `NearestTargetFramework` will be used to formulate `TargetFramework` for the following calls and the other two properties are booleans, or
	* an item with metadata `DesiredTargetFrameworkProperties` (key-value pairs of the form `TargetFramework=net46`), `HasSingleTargetFramework` (boolean), and `IsRidAgnostic` (boolean).

	<Target Name="GetTargetFrameworks"
	DependsOnTargets="GetTargetFrameworksWithPlatformForSingleTargetFramework"

Don't flow self contained command-line option to referenced projects #6924

Don't flow self contained command-line option to referenced projects #6924

Conversation

dsplaisted commented Oct 7, 2021

Context

Changes Made

Testing

Notes

rainersigwald left a comment

Choose a reason for hiding this comment

benvillalobos commented Oct 7, 2021 • edited Loading

dsplaisted commented Oct 8, 2021 • edited Loading

Forgind commented Oct 8, 2021 • edited Loading

dsplaisted commented Oct 8, 2021

dsplaisted commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsplaisted commented Nov 18, 2021

rainersigwald commented Nov 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beatcracker commented Nov 24, 2021 • edited Loading

Forgind commented Apr 20, 2022

dsplaisted commented Apr 25, 2022

dsplaisted commented Aug 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind left a comment

Choose a reason for hiding this comment

benvillalobos commented Oct 7, 2021 •

edited

Loading

dsplaisted commented Oct 8, 2021 •

edited

Loading

Forgind commented Oct 8, 2021 •

edited

Loading

beatcracker commented Nov 24, 2021 •

edited

Loading