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 Negotiation - Allow AnyCPU projects to resolve specific platform references #6897

Open
jhennessey opened this issue Sep 29, 2021 · 6 comments

Comments

@jhennessey
Copy link

The SetPlatform Negotiation feature does not work "out-of-the-box" if there is an AnyCPU project that references a specific platform (x86/x64) project. For example, the following combination of projects will fail (or possibly fallback to a default platform):

ProjectA [x86/x64] -> ProjectB [AnyCPU] -> ProjectC [x86/x64]

In this case, it would be ideal if ProjectB could infer that it should build ProjectC as either x86 or x64.

Note: In real-world scenarios, ProjectC is generally forced to be both x86 / x64 in order to accommodate a third-party native wrapper.

Additional Context: Created issue based on this comment from @benvillalobos in #6889.

Discussion Point: As mentioned by @AArnott here, you will generally get a processor architecture mismatch warning when building the specified project chain (but it does work). To address that warning (without working around it via setting the ResolveAssemblyWarnOrErrorOnTargetArchitectureMismatch property) requires ALL projects to have both x86 and x64 platform configurations. This in turn means that you have to build everything twice to support your dual x86 and x64 build…which isn't ideal for really large codebases.

@jhennessey jhennessey added the needs-triage Have yet to determine what bucket this goes in. label Sep 29, 2021
@jhennessey
Copy link
Author

This code addresses this issue by passing a new property DynamicPlatformResolutionPreferredSpecificPlatform which gets set to the first Platform value that isn't AnyCPU. That property gets propagated to each reference and, when necessary, gets used to create a platform look-up table that gets passed to the GetCompatiblePlatform task: jhennessey@e103be1

@benvillalobos @AArnott - Would be curious on your thoughts on that approach.

@benvillalobos benvillalobos added Area: SetPlatform and removed needs-triage Have yet to determine what bucket this goes in. labels Sep 29, 2021
@AArnott
Copy link
Contributor

AArnott commented Sep 29, 2021

Thanks for writing this up along with your thoughts and a sample of your proposal.

Your new property doesn't sound like it is guaranteed to be set as a global property for every single P2P reference. Since it doesn't change the OutputPath, that concerns me because it could lead to overbuild of a project, and thus timing breaks when the project builds twice concurrently, once with this global property and once without.

In your example, ProjectB will build twice if a ProjectD [AnyCPU] exists in the graph with no x86/x64 project referencing it. ProjectB [AnyCPU] will try both to "copy local" the x86 and the x64 binaries of ProjectC between ProjectB's two builds. The result of the build is now non-deterministic as to which binary will "win", and likely to break the build when ProjectB builds concurrently.
As an AnyCPU project, its output shouldn't vary by CPU architecture at all. And IMO it shouldn't have ProjectC's binaries in its output directory both because it professes to be AnyCPU and because of this undefined end result.

One way we could potentially make this work is to have ProjectB build twice, with two separate output directories. But you've already stated your concern that that slows down the build. What if we only compiled once though? If ProjectB had an AnyCPU "inner build", but then its outer build deployed the output to two separate locations, and did copy-local of the appropriate x86 or x64 dependencies as appropriate, that would at least pacify some of my concerns. But compilation is sometimes not the dominating time in a large repo build so I don't know how much this buys you.

Why does ProjectB have ProjectC as a dependency in your scenario anyway? Does ProjectB compilation actually reference the output from ProjectC in the compilation step, or is it merely to express a runtime dependency?

@benvillalobos
Copy link
Member

Since it doesn't change the OutputPath, that concerns me because it could lead to overbuild of a project

It would change outputpath, wouldn't it? In B to C (in the original post), C would be told to build with Platform=whateverA built as, so when C builds it would get its outputpath set accordingly.

In your example, ProjectB will build twice if a ProjectD [AnyCPU] exists in the graph with no x86/x64 project referencing it

Could you draw out the graph here? I might be misunderstanding

Speaking strictly functionally:

I like the change. I'm not super familiar with AdditionalProperties, but if it works like it looks like it does then it's worth including IMO. Andrew is asking the right questions (read: asking "why?"), but I'm generally in favor of allowing scenarios that I don't quite understand but regardless are scenarios customers have. Probably because I've seen so many that already exist and that we need to support 👀

@AArnott
Copy link
Contributor

AArnott commented Oct 2, 2021

It would change outputpath, wouldn't it? In B to C (in the original post), C would be told to build with Platform=whateverA built as, so when C builds it would get its outputpath set accordingly.

I'm talking about the output path of B. It would build twice (once with DynamicPlatformResolutionPreferredSpecificPlatform set and once without), but with the same output path. This would slow down the build (at best) or cause timing breaks (at worst).

Could you draw out the graph here? I might be misunderstanding

Sure. The problem requires that two paths to B exist--one that has gone through a platform-specific project and one that has not.
In the graphs below, DPRPSS = DynamicPlatformResolutionPreferredSpecificPlatform as @jhennessey described above.
Thinking about this more, this case would not be a problem, because the graph starts with an architecture:

                   ┌────────────┐             ┌───────────┐    ┌────────────┐
        ┌─────┬───►│ A (x86/x64)├────────────►│ B (AnyCPU)├───►│ C (x86/x64)│
 ┌──────┴─────┤    └────────────┘ DPRPSS=x64  └───────────┘    └────────────┘
 │ dirs.proj  │                    P=AnyCPU        ▲             P=x64
 └───────┬────┘                                    │
P=x64    │         ┌────────────┐                  │
         └────────►│ D (AnyCPU) ├──────────────────┘
                   └────────────┘  DPRPSS=x64
                                    P=AnyCPU

But this case would build B twice:

                  ┌────────────┐             ┌───────────┐          ┌────────────┐
       ┌─────┬───►│ A (x86)    ├────────────►│ B (AnyCPU)├─────────►│ C (x86/x64)│
┌──────┴─────┤    └────────────┘ DPRPSS=x86  └───────────┘  P=x86   └────────────┘
│ dirs.proj  │                    P=AnyCPU        ▲
└───────┬────┘                                    │
        │         ┌────────────┐                  │
        └────────►│ D (AnyCPU) ├──────────────────┘
                  └────────────┘   P=AnyCPU

In this example, A only offers one architecture (x86 in this case) so specifying it explicitly is not required as the one is the default. Notice how there is now a path to B that does not set DPRPSS and that does. B now builds twice.

@benvillalobos
Copy link
Member

Ah, thanks for the graph. My confusion was AdditionalProperties sounding too good to be true (giving projects properties for free, not globally).

So it sounds like the realistic scenario here is some projects should build as every platform it contains because there will be multiple projects that depend on it. Are we leaning toward a future where those projects contain some "build me entirely" property that allows it to have its own "outer" and "inner" builds for each platform?

@AArnott
Copy link
Contributor

AArnott commented Oct 5, 2021

I would very much like to see projects building all their supported platforms the same way they build their inner TFMs -- when they are built as top-level projects (without a global property specifying the platform to build.) But like multi-TFM projects, as a target of a P2P, only the TFM (or platform) that the referencing project requires should be built.

When I think about the scenario of B (AnyCPU) referencing C (x86/x64), ultimately B still (typically) needs to "copy local" the outputs of one of C's platform builds. So IMO B should still always specify the platform it wants explicitly and then only that platform should be built, and then copied local. That is assuming that B should reference C at all.

The best case I can think of for why a B AnyCPU should reference C is that B is just a library and doesn't touch any of C's arch-specific APIs. Theoretically if C was only arch-specific as an implementation detail, an AnyCPU library could reference it (and not copy local at all), leaving it to app A (x86) to copy both C's x86 output and B's AnyCPU output into A's output directory, and it would all work.
This works best when C offers an AnyCPU reference assembly in addition to its arch-specific implementation assemblies. .NET Framework and .NET Core BCL do this all the time. We build AnyCPU projects most of the time, referencing BCL assemblies that at runtime may very well be arch-specific, but the ref assemblies we use to compile our projects do not include any APIs that vary with architecture. If there was good build/compiler tooling to support it, I'd say C should leverage that. In that case, B would get the anycpu ref assembly and life is good. And if another arch-specific project were to reference C, they would get the arch-specific build output on the same principle that led into this platform negotiation feature's original design: a P2P should consume the most precisely matching platform possible, because platform-specific is always better, or it wouldn't exist and we'd just have AnyCPU.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants