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

Stop reading TargetFramework prop in props files #52897

Merged
merged 12 commits into from
May 19, 2021

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented May 18, 2021

The TargetFramework property isn't expected to be set in props files
before a project's body is evaluated.

Don't let BuildTargetFramework property fallback to TargetFramework as
BTF's sole intent is to convey the TargetFramework to filter to and not
the current selected TargetFramework. Reduce usage of BTF so that it is
only used in places where code is actually conditioned on filtering.

In addition to that, specify BuildTargetFramework as a global property for
all traversal builds so when invoking one of the traversal projects directly
(src.proj, ref.proj, etc.), BuildTargetFramework doesn't need to specified
manually to get the default behavior (filter on compatible to net6.0).
Remove the inferred BuildingNetCoreAppVertical because of that and
inline its meaning.

Make sendtohelix a normal NoTargets proj so that it has access to the
properties which were moved into the Directory.Build.targets file as
otherwise it wouldn't import that file.

The TargetFramework property isn't expected to be set in props files
before a project's body is evaluated.

Don't let BuildTargetFramework property fallback to TargetFramework as
BTF's sole intent is to convey the TargetFramework to filter to and not
the current selected TargetFramework. Reduce usage of BTF so that it is
only used in places where code is actually conditioned on filtering.
@ghost
Copy link

ghost commented May 18, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

The TargetFramework property isn't expected to be set in props files
before a project's body is evaluated.

Don't let BuildTargetFramework property fallback to TargetFramework as
BTF's sole intent is to convey the TargetFramework to filter to and not
the current selected TargetFramework. Reduce usage of BTF so that it is
only used in places where code is actually conditioned on filtering.

Author: ViktorHofer
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@ghost ghost added this to In Progress in Infrastructure Backlog May 18, 2021
@am11
Copy link
Member

am11 commented May 18, 2021

uhg, i think for Windows, we would need to escape the quote as it's already quoted. I don't have the Windows build machine, can we test it here, changing " to \" in the patch?

@ViktorHofer
Copy link
Member Author

Sure

@ViktorHofer
Copy link
Member Author

You mean like that?

@am11
Copy link
Member

am11 commented May 18, 2021

Yup, thanks. Lets see if it makes windows-wasm leg happy. :)
(your idea of setting it conditionally was more sensible if it doesn't work, we can try that)

@ViktorHofer
Copy link
Member Author

Hmm still failing:

  else was unexpected at this time.
D:\workspace\_work\1\s\src\libraries\Native\build-native.proj(48,5): error MSB3073: The command ""D:\workspace\_work\1\s\src\libraries\Native\build-native.cmd" wasm Release outconfig net6.0-Browser-Release-wasm -os Browser /p:OfficialBuildId=\"\"" exited with code 255

@ViktorHofer ViktorHofer requested review from ericstj and Anipik and removed request for marek-safar May 19, 2021 08:33
@ViktorHofer ViktorHofer self-assigned this May 19, 2021
@ViktorHofer
Copy link
Member Author

The PR is ready for review, PTAL.

@@ -1,7 +1,6 @@
<Project Sdk="Microsoft.Build.Traversal" DefaultTargets="Pack">

<PropertyGroup>
<TraversalGlobalProperties>BuildAllProjects=true</TraversalGlobalProperties>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we no longer need BuildAllProjects with the new version ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The libraries packages don't need the BuildAllProjects global property.

Copy link
Contributor

@Anipik Anipik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ViktorHofer ViktorHofer merged commit 68c5658 into dotnet:main May 19, 2021
Infrastructure Backlog automation moved this from In Progress to Done May 19, 2021
@ViktorHofer ViktorHofer deleted the StopReadTFMInProps branch May 19, 2021 18:04
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@@ -15,7 +15,7 @@ parameters:
scenarios: ''

steps:
- script: $(_msbuildCommand)
- script: $(_msbuildCommand) -restore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why do we need to restore sendtohelix?

@ghost ghost locked as resolved and limited conversation to collaborators Jul 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants