Pass architecture to bundler #13402
Pass architecture to bundler #13402
Conversation
sbomer
requested review from
swaroop-sridhar,
dsplaisted,
vitek-karas and
agocke
|
The change looks good, but since the change needs to be in 5.0.1xx-rc2, do we need to checkin into master as well? |
|
It will need to go into master too - my understanding is that the RC branches will periodically be merged to master, but I'll probably port it myself after it goes into RC2, to unblock dependency flow more quickly. @wli3 @dsplaisted is that an ok way to do it? |
| Architecture targetArch = RuntimeIdentifier.EndsWith("x64") ? Architecture.X64 : | ||
| RuntimeIdentifier.EndsWith("x86") ? Architecture.X86 : | ||
| RuntimeIdentifier.EndsWith("arm64") ? Architecture.Arm64 : | ||
| RuntimeIdentifier.EndsWith("arm") ? Architecture.Arm : | ||
| throw new ArgumentException(nameof (RuntimeIdentifier)); |
There was a problem hiding this comment.
This is problematic - for example win81-arm64-aot is a valid RID - which would throw here. Can't we get the target architecture directly from the MSBuild as a parameter/property?
There was a problem hiding this comment.
Good to know. What are the aot rids, and where can I find out more about them? :)
There is a PlatformTarget computed from the RID. However it doesn't have a case for ARM64, even though it is used elsewhere. The linker targets might also be incorrect. @wli3 @dsplaisted do you have a recommendation on how to get the target architecture?
There was a problem hiding this comment.
We should fix PlatformTarget - this comment is obviously wrong: https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L90
There was a problem hiding this comment.
That said for 5.0 I think it would be OK to implement a similar algorithm as the SDK has (so EndsWith or Contains) and call it good enough. For 6.0 we should definitely teach the SDK about ARM64 (since it's a perfectly valid platform) and make sure we have ideally only one place which computes the target architecture.
There was a problem hiding this comment.
There's also Platform which sets PlatformTarget but again doesn't have an ARM64 case. And MSBuild has a ProcessorArchitecture which comes from PlatformTarget.
I agree we should fix PlatformTarget but am hesitant to touch it in 5.0 - so I'll just fix the RID parsing here unless SDK folks would prefer something else.
I'm curious what the aot RIDs are for, but had trouble finding out much about them. Is there a document somewhere?
There was a problem hiding this comment.
If you want to use the RuntimeIdentifier the correct way to do it (eventually) is probably to pass the runtime graph (a file now included in the SDK) to the task, and then use graph operations to figure out if the current RuntimeIdentifier inherits from a more generic RuntimeIdentifier specifying the architecture you're interested in.
Also, per this, we should use PlatformTarget instead of Platform in this type of logic, as Platform can be an arbitrary alias that the developer defines.
There was a problem hiding this comment.
The AOT RIDs are for AOT (Ahead of time compilers) - one such example is building for .NET Native. I don't think they're "widely" used for now.
I don't think bundler should be in the business of trying to understand RIDs - that's a complicated topic. SDK needs to correctly define properties describing the target - in this case PlatformTarget - and bundler should just use them. (That is obviously the ideal state, not what we need to do for 5.0)
Yes, it should flow eventually, but yes it's also fine to do a dual checkin if you want it in master sooner. |
To fix ARM64 Linux behavior. See dotnet/runtime#41809.
We fall back to linux so that RIDs like ubuntu* are supported.
* Pass architecture to bundler To fix ARM64 Linux behavior. See dotnet/runtime#41809. * Fix how we get architecture from RIDs * Update dependencies from runtime * Revert incorrect targetOS change We fall back to linux so that RIDs like ubuntu* are supported.
* Pass architecture to bundler To fix ARM64 Linux behavior. See dotnet/runtime#41809. * Fix how we get architecture from RIDs * Update dependencies from runtime * Revert incorrect targetOS change We fall back to linux so that RIDs like ubuntu* are supported.
To fix ARM64 Linux behavior. See dotnet/runtime#41809.
This depends on the runtime change, but I wanted to get feedback before that change flows.