-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Don't flow self contained command-line option to referenced projects #6924
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
Changes from 4 commits
9c56b5e
f849bea
59bf77d
ab2662a
512a065
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||
| <!-- | ||||||||
| <!-- | ||||||||
| *********************************************************************************************** | ||||||||
| 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)" | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was previously RuntimeIdentifier$(_GlobalPropertiesToRemoveFromProjectReferences) (no semicolon)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes |
||||||||
| Condition="'%(_MSBuildProjectReferenceExistent.SkipGetTargetFrameworkProperties)' != 'true' and '$(EnableDynamicPlatformResolution)' != 'true'" | ||||||||
| SkipNonexistentTargets="true"> | ||||||||
| <Output TaskParameter="TargetOutputs" ItemName="_ProjectReferenceTargetFrameworkPossibilities" /> | ||||||||
|
|
@@ -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)" | ||||||||
|
||||||||
| Condition="'%(_MSBuildProjectReferenceExistent.SkipGetTargetFrameworkProperties)' != 'true' and '$(EnableDynamicPlatformResolution)' == 'true'" | ||||||||
| SkipNonexistentTargets="true"> | ||||||||
| <Output TaskParameter="TargetOutputs" ItemName="_ProjectReferenceTargetFrameworkPossibilities" /> | ||||||||
|
|
@@ -1850,6 +1850,20 @@ Copyright (C) Microsoft Corporation. All rights reserved. | |||||||
| Condition="'$(ReferringTargetFrameworkForProjectReferences)' == '' or | ||||||||
| ('$(EnableDynamicPlatformResolution)' == 'true' and '%(_ProjectReferenceTargetFrameworkPossibilities.IsVcxOrNativeProj)' == 'true')" /> | ||||||||
|
|
||||||||
| </ItemGroup> | ||||||||
|
|
||||||||
| <!-- IsRidAgnostic metadata is used to determine whether global properties such as RuntimeIdentifier and SelfContained flow to a referenced project. | ||||||||
| However, for multi-targeted projects there may be a different IsRidAgnostic value for each TargetFramework. In that case, this task selects | ||||||||
| the IsRidAgnostic value for the NearestTargetFramework for the project. --> | ||||||||
| <SetRidAgnosticValueForProjects Projects="@(AnnotatedProjects)"> | ||||||||
| <Output ItemName="UpdatedAnnotatedProjects" TaskParameter="UpdatedProjects" /> | ||||||||
| </SetRidAgnosticValueForProjects> | ||||||||
|
|
||||||||
| <ItemGroup> | ||||||||
| <AnnotatedProjects Remove="@(AnnotatedProjects)" /> | ||||||||
| <AnnotatedProjects Include="@(UpdatedAnnotatedProjects)" /> | ||||||||
| <UpdatedAnnotatedProjects Remove="@(UpdatedAnnotatedProjects)" /> | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this line necessary?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||
|
|
||||||||
| <!-- If the NearestTargetFramework property was set and the project multi-targets, SetTargetFramework must be set. --> | ||||||||
| <AnnotatedProjects Condition="'@(AnnotatedProjects)' == '%(Identity)' and '%(AnnotatedProjects.NearestTargetFramework)' != '' and '%(AnnotatedProjects.HasSingleTargetFramework)' != 'true'"> | ||||||||
| <SetTargetFramework>TargetFramework=%(AnnotatedProjects.NearestTargetFramework)</SetTargetFramework> | ||||||||
|
|
@@ -1863,9 +1877,10 @@ Copyright (C) Microsoft Corporation. All rights reserved. | |||||||
| <UndefineProperties>%(AnnotatedProjects.UndefineProperties);TargetFramework</UndefineProperties> | ||||||||
| </AnnotatedProjects> | ||||||||
|
|
||||||||
| <!-- 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> | ||||||||
|
||||||||
| * 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). |
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.
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.
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'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
nagilson marked this conversation as resolved.
Show resolved
Hide resolved
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.
Does _TargetFrameworkInfo ever have more than just $(TargetFramework) included in it?
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.
Yes, it does for multi-targeted projects, which will run this target for each target framework and combine the results.
Outdated
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.
Should this be true and the next one false?
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.
+1, it looks like it will set IsRidAgnostic to false when there are no runtime identifiers
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.
Yes, thanks for catching this.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,9 @@ | ||
| Microsoft.Build.Tasks.SetRidAgnosticValueForProjects | ||
| Microsoft.Build.Tasks.SetRidAgnosticValueForProjects.Projects.get -> Microsoft.Build.Framework.ITaskItem[] | ||
| Microsoft.Build.Tasks.SetRidAgnosticValueForProjects.Projects.set -> void | ||
| Microsoft.Build.Tasks.SetRidAgnosticValueForProjects.SetRidAgnosticValueForProjects() -> void | ||
| Microsoft.Build.Tasks.SetRidAgnosticValueForProjects.UpdatedProjects.get -> Microsoft.Build.Framework.ITaskItem[] | ||
| Microsoft.Build.Tasks.SetRidAgnosticValueForProjects.UpdatedProjects.set -> void | ||
| override Microsoft.Build.Tasks.SetRidAgnosticValueForProjects.Execute() -> bool | ||
| Microsoft.Build.Tasks.XslTransformation.PreserveWhitespace.get -> bool | ||
| Microsoft.Build.Tasks.XslTransformation.PreserveWhitespace.set -> void | ||
| Microsoft.Build.Tasks.XslTransformation.PreserveWhitespace.set -> void |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,9 @@ | ||
| Microsoft.Build.Tasks.SetRidAgnosticValueForProjects | ||
| Microsoft.Build.Tasks.SetRidAgnosticValueForProjects.Projects.get -> Microsoft.Build.Framework.ITaskItem[] | ||
| Microsoft.Build.Tasks.SetRidAgnosticValueForProjects.Projects.set -> void | ||
| Microsoft.Build.Tasks.SetRidAgnosticValueForProjects.SetRidAgnosticValueForProjects() -> void | ||
| Microsoft.Build.Tasks.SetRidAgnosticValueForProjects.UpdatedProjects.get -> Microsoft.Build.Framework.ITaskItem[] | ||
| Microsoft.Build.Tasks.SetRidAgnosticValueForProjects.UpdatedProjects.set -> void | ||
| Microsoft.Build.Tasks.XslTransformation.PreserveWhitespace.get -> bool | ||
| Microsoft.Build.Tasks.XslTransformation.PreserveWhitespace.set -> void | ||
| Microsoft.Build.Tasks.XslTransformation.PreserveWhitespace.set -> void | ||
| override Microsoft.Build.Tasks.SetRidAgnosticValueForProjects.Execute() -> bool |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.Build.Framework; | ||
| using Microsoft.Build.Shared; | ||
| using Microsoft.Build.Utilities; | ||
|
|
||
| namespace Microsoft.Build.Tasks | ||
| { | ||
| public sealed class SetRidAgnosticValueForProjects : TaskExtension | ||
| { | ||
| public ITaskItem[] Projects { get; set; } = Array.Empty<ITaskItem>(); | ||
|
|
||
| [Output] | ||
| public ITaskItem[] UpdatedProjects { get; set; } = Array.Empty<ITaskItem>(); | ||
|
|
||
| public override bool Execute() | ||
| { | ||
| UpdatedProjects = Projects | ||
| .Select(p => | ||
| { | ||
| var hasSingleTargetFrameworkString = p.GetMetadata("HasSingleTargetFramework"); | ||
| if (!ConversionUtilities.ValidBooleanFalse(hasSingleTargetFrameworkString)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you choose !false rather than true? Could it be null here? If so, this seems like wrong behavior.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| { | ||
| // No change to item, it should already have a single-valued IsRidAgnostic value | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we have a Debug.Assert here or something?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| return p; | ||
| } | ||
| var updatedItem = new TaskItem(p); | ||
|
|
||
| var nearestTargetFramework = p.GetMetadata("NearestTargetFramework"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What information are you referring to? These are part of the Project Reference Protocol, or in the case of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| if (string.IsNullOrEmpty(nearestTargetFramework)) | ||
| { | ||
| return p; | ||
| } | ||
|
|
||
| var targetFrameworksArray = p.GetMetadata("TargetFrameworks").Split(';'); | ||
|
|
||
| int targetFrameworkIndex = Array.IndexOf(targetFrameworksArray, nearestTargetFramework); | ||
| if (targetFrameworkIndex < 0) | ||
| { | ||
| return p; | ||
| } | ||
|
|
||
| var isRidAgnosticArray = p.GetMetadata("IsRidAgnostic").Split(';'); | ||
| if (isRidAgnosticArray.Length != targetFrameworksArray.Length) | ||
| { | ||
| return p; | ||
| } | ||
|
|
||
| updatedItem.SetMetadata("IsRidAgnostic", isRidAgnosticArray[targetFrameworkIndex]); | ||
|
|
||
| return updatedItem; | ||
| }) | ||
| .ToArray(); | ||
|
|
||
| return true; | ||
| } | ||
| } | ||
| } | ||
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.
Should GetTargetFrameworks depend on GetTargetFrameworksWithPlatformForSingleSingleTargetFramework explicitly?
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's what it does for single-targeted projects:
msbuild/src/Tasks/Microsoft.Common.CurrentVersion.targets
Lines 1889 to 1890 in 5d102ae
For multi-targeted projects it has to do inner builds for each target framework, which the
GetTargetFrameworksWithPlatformFromInnerBuildstarget handles.