-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
SetPlatform: Use Platform Instead Of PlatformTarget #6889
Conversation
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
@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? |
Docs on how to deploy locally if you need it |
@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 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 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. |
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 |
Fixes #6887
Discussion: #6871
Context
PlatformTarget
was initially passed to managed projects becausePlatformTarget
is passed directly to the compiler, while Platform is passed to the cpp compiler. After a deeper investigation, it turns out we can passPlatform
andPlatformTarget
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