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

Fix sequencing of TargetFrameworkSuffix stripping #4993

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

ViktorHofer
Copy link
Member

Hooking into CollectPackageReferences for stripping the TargetFrameworkSuffix is a big hammer and unnecessary as the exact targets that read the TargetFrameworks value are _GetRestoreTargetFrameworksOutput;_GetRestoreTargetFrameworksAsItems. Those targets are always invoked as part of the _GenerateRestoreGraph on which we hook the attaching of the TargetFarmeworkSuffix then.

Keeping the hook on CollectPackageReferences doesn't guarantee that attaching of the Suffix later as that target is also called outside of the Restore phase. This fixes that and allows P2P OS-specific references.

Hooking into CollectPackageReferences for stripping the TargetFrameworkSuffix is a big hammer and unnecessary as the exact targets that read the TargetFrameworks value are _GetRestoreTargetFrameworksOutput;_GetRestoreTargetFrameworksAsItems. Those targets are always invoked as part of the _GenerateRestoreGraph on which we hook the attaching of the TargetFarmeworkSuffix then.
@ViktorHofer ViktorHofer requested review from ericstj and Anipik March 6, 2020 12:24
@ViktorHofer ViktorHofer self-assigned this Mar 6, 2020
@ViktorHofer ViktorHofer changed the title Fix sequencing of TargetFramework stripping Fix sequencing of TargetFrameworkSuffix stripping Mar 6, 2020
@ericstj
Copy link
Member

ericstj commented Mar 6, 2020

It's unfortunate that have to use private targets here. I'm OK if this unblocks you.

An alternative, if feasible, would be to do something similar to TargetFramework. If we can be sure that everything that needs to understand the suffixed TargetFrameworks is owned by us (this I'm not sure of) then we could just assign the suffixed TargetFrameworks to our own property, and do the stripping as part of evaluation and never undo it.

@Anipik
Copy link
Contributor

Anipik commented Mar 6, 2020

Hopefully we can remove this stuff after the new tfm work

@ericstj
Copy link
Member

ericstj commented Mar 6, 2020

Hopefully we can remove this stuff after the new tfm work

Yeah, that's possible. The new TFMs have the right precedence X-Y compatible with X, so even if we map X-Y to something different (TFM=X, RID=Y) than the actual SDK (TFM=X-Y) I suspect we can make it work. 🤞

@ViktorHofer ViktorHofer merged commit b29579a into master Mar 6, 2020
@ViktorHofer ViktorHofer deleted the ViktorHofer-tfm-stripping branch March 6, 2020 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants