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

Moving more Tfm specific properties to the targetFramework.props file #34349

Merged
merged 3 commits into from
Apr 3, 2020
Merged

Moving more Tfm specific properties to the targetFramework.props file #34349

merged 3 commits into from
Apr 3, 2020

Conversation

Anipik
Copy link
Contributor

@Anipik Anipik commented Mar 31, 2020

No description provided.

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

This is a good step in the right direction. Did you look at every property that was moved to ensure there weren't further derived properties? Assuming so, then a next step could be to try to eliminate all usage of the properties now in targetframework.props from propertygroups in props files / project bodies (so that we can support defining TargetFramework in the project body).

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. Can you please file an issue for the follow-up work which is making sure that TargetFramework is never read before the project file is loaded?

@Anipik
Copy link
Contributor Author

Anipik commented Apr 3, 2020

did you look at every property that was moved to ensure there weren't further derived properties?

The only tfm properties are left for manual shims. We need to keep them to add manual keyword in the path to avoid having the conflicts with generated shims

Can you please file an issue for the follow-up work which is making sure that TargetFramework is never read before the project file is loaded?

yeah i will do that.

<AppendRuntimeIdentifierToOutputPath>false</AppendRuntimeIdentifierToOutputPath>
<AdditionalBuildTargetFrameworks Condition="'$(BuildAllProjects)' == 'true'">netstandard2.0-$(TargetFrameworkSuffix)</AdditionalBuildTargetFrameworks>
<AdditionalBuildTargetFrameworks Condition="'$(BuildAllProjects)' == 'true'">netstandard2.0</AdditionalBuildTargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

Why are you removing the TargetFrameworkSuffix here?

Copy link
Member

Choose a reason for hiding this comment

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

What about RestoreOutputPath, ProjectAssetsFile and MSBuildProjectExtensionsPath above, which all derive from IntermediateOutputPath which derives from TargetFramework.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As eric mentioned , the next step is to remove the usage of all properties in targetFramework.props from propertyGroups.
eg all these properties will be the candidate for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why are you removing the TargetFrameworkSuffix here?

This was a bug. TargetFrameworkSuffix is a property for innerBuilds and is not defined for outer builds and should not be used in outer build. AdditionalBuildTargetFrameworks is used only in outerbuilds.

Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure what's the scope of this PR. Isn't the goal to fix the broken intermediate/output paths? An example:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scope is just cleanup, nothing is getting fixed here.

@Anipik Anipik merged commit 1ac25b4 into dotnet:master Apr 3, 2020
@Anipik Anipik deleted the followUp branch April 3, 2020 23:10
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
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.

4 participants