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

SetPlatform: Use Platform Instead Of PlatformTarget #6889

Merged
merged 2 commits into from
Sep 27, 2021

Conversation

benvillalobos
Copy link
Member

@benvillalobos benvillalobos commented Sep 24, 2021

Fixes #6887
Discussion: #6871

Context

PlatformTarget was initially passed to managed projects because PlatformTarget is passed directly to the compiler, while Platform is passed to the cpp compiler. After a deeper investigation, it turns out we can pass Platform and PlatformTarget will be defined either in the project file or by the SDK based on Platform.

Changes Made

Pass and undefine Platform instead of PlatformTarget.

Usage of PlatformTarget has been removed altogether, in favor of Platform.

Testing

Tested on all projects here: https://github.com/BenVillalobos/setplatform-repro
Also confirmed by jhennessey that this solves the issue.

Notes

Instead of a mix between PlatformTarget and Platforms.
This fixes an issue with legacy projects that define values
for OutputPath and PlatformTarget based on the Platform|Configuration.
It also fixes an issue where a user can start a command line build by
passing /p:Platform=foo, and that global property would carry to ALL
p2p hops. This can lead to weird issues such as the Platform being x86
when a p2p hop might pass PlatformTarget=AnyCPU
@benvillalobos
Copy link
Member Author

@jhennessey We're trying to get this merged in ASAP. Any way you could try this diff soon and see if this still builds your big ol' project?

@Forgind
Copy link
Member

Forgind commented Sep 27, 2021

Docs on how to deploy locally if you need it

@jhennessey
Copy link

jhennessey commented Sep 27, 2021

We're trying to get this merged in ASAP. Any way you could try this diff soon and see if this still builds your big ol' project?

@benvillalobos - It works! :).

I'll mention that I've been working on a fix locally which I think should be addressed with P2P builds. The issue is that the following combination will fail (assume Platform=x64 was passed on the command line):

LibraryA [x64] -> LibraryB [AnyCPU] -> LibraryC [x64]

In this case, you'll end up getting the "...BaseOutputPath/OutputPath property is not set..." error for LibraryC. Should I file a separate bug for this?

@benvillalobos
Copy link
Member Author

benvillalobos commented Sep 27, 2021

It works! :).

🥳

LibraryA [x64] -> LibraryB [AnyCPU] -> LibraryC [x64]
In this case, you'll end up getting the "...BaseOutputPath/OutputPath property is not set..." error for LibraryC. Should I file a separate bug for this?

Yes please! To be clear, the issue you're running into is that B doesn't know what came before it, so it logs a warning and undefines Platform for C, then tells C to build on its own, when ideally B should "just know" and tell C to build as x64? I don't know what your project file looks like, but most legacy projects define a default value for Platform when it's not defined, and that value is AnyCPU. Would changing that value for your C project work? (might be best to file the issue and continue the discussion there). The expected workaround for now is to add SetPlatform metadata on your project reference. If you assign SetPlatform metadata, the logic to "figure it out" is skipped over.

It's currently a known issue, and one that's difficult to fix without implementing some sort of "walk" through every P2P to figure this out ahead of time. It was discussed and put to the side until setplatform version 1 could be merged.

@AArnott
Copy link
Contributor

AArnott commented Sep 27, 2021

LibraryA [x64] -> LibraryB [AnyCPU] -> LibraryC [x64]

I would argue that the LibraryB -> LibraryC P2P should not exist. LibraryB cannot be an AnyCPU library when it depends on an x64 binary. In some cases msbuild or csc will emit warnings about a reference like this. When you really want to do it anyway, as @benvillalobos says, I would expect you to add SetPlatform metadata on the ProjectReference item directly to make msbuild allow you to do this (without a warning).

@benvillalobos benvillalobos 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 Sep 27, 2021
@benvillalobos benvillalobos merged commit 3a1e456 into dotnet:main Sep 27, 2021
@benvillalobos benvillalobos deleted the setplatform-passplatform branch September 27, 2021 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ProjectReference Platform Negotiation - Pass Platform Instead of PlatformTarget
5 participants