Update SDK and remove RID calculation in favor of RuntimeInformation#35538
Update SDK and remove RID calculation in favor of RuntimeInformation#35538ViktorHofer merged 9 commits intomasterfrom
Conversation
|
Tagging subscribers to this area: @ViktorHofer |
|
@eerhardt any idea what could be causing the assembly not to be loaded? Who would be the right folks to be looped in here? cc @vitek-karas |
|
@jkotas maybe related to 7d4f73c#diff-3b28fb8ad423b63bc5edd1e92e1de6c0? |
Yes, I removed PlatformAbstractions from the To fix it, you will need to bring PlatformAbstractions with your Task that requires it. But also know that we are working on removing PlatformAbstractions completely. See #3470. So eventually, we will want to get this Task off of PlatformAbstractions as well. |
|
Maybe a better approach would be to remove |
|
The top level build knows the RID that we building for (e.g. look for |
|
Looking deeper, I think this
|
|
Thanks for the suggestions. I will consume Arcade's version for now and remove the copy in dotnet/runtime. Adding the commit here to test the change. |
6a9197c to
8555467
Compare
7c1a3e2 to
ea53e66
Compare
c7ef8e8 to
bc362b6
Compare
bc362b6 to
3b2ef2d
Compare
|
@ViktorHofer we can remove this as well https://github.com/dotnet/runtime/blob/master/eng/pipelines/libraries/build-job.yml#L148 |
eng/Configurations.props
Outdated
| <PropertyGroup> | ||
| <HostRuntimeIdentifier Condition="'$(HostRuntimeIdentifier)' == '' and '$(MSBuildRuntimeType)' == 'core'">$([System.Runtime.InteropServices.RuntimeInformation]::RuntimeIdentifier)</HostRuntimeIdentifier> | ||
| <HostRuntimeIdentifier Condition="'$(HostRuntimeIdentifier)' == '' and '$(MSBuildRuntimeType)' != 'core'">win-$([System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture.ToString().ToLowerInvariant)</HostRuntimeIdentifier> | ||
| <RuntimeOS Condition="'$(RuntimeOS)' == '' and '$(HostRuntimeIdentifier)' != ''">$(HostRuntimeIdentifier.Remove($(HostRuntimeIdentifier.LastIndexOf('-'))))</RuntimeOS> |
There was a problem hiding this comment.
This should not be called RuntimeOS. The other places that use RuntimeOS in this repo use it as "The machine that we are targeting.". The meaning here is "The machine that are running on.".
There was a problem hiding this comment.
Also, I am not sure whether it makes sense to move these Host* properties to the shared file. They should not be ever used. Their use in the installer partition is less than ideal.
There was a problem hiding this comment.
This should not be called RuntimeOS.
This is already called RuntimeOS in libraries. That property is currently generated in https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/AddRuntimeOSPropertyTask.cs#L12 and then imported in https://github.com/dotnet/runtime/blob/master/src/libraries/Directory.Build.props#L29.
For the sake of this PR I will move the RuntimeOS property into libraries.
Also, I am not sure whether it makes sense to move these Host* properties to the shared file.
The HostRuntimeIdentifier property is used in both libraries and installer at the moment that's why I'm extracting the property into this shared file.
There was a problem hiding this comment.
The other places that use RuntimeOS in this repo use it as "The machine that we are targeting.". The meaning here is "The machine that are running on.".
There is a property directly below this named $(TargetOS), why don't we use that property to mean "The machine that we are targeting"? That would be a much better name for that property IMO.
There was a problem hiding this comment.
Note: src\libraries uses RuntimeOS to mean "The machine that we building on." That is the MSBuild Target that is being removed in this PR - see the GenerateRuntimeOSPropsFileBeforeRestore.
Maybe it would make sense to rectify "RuntimeOS" in a follow up PR? That way we can make forward progress in getting the new SDK in.
There was a problem hiding this comment.
I moved the property into libraries where it was imported beefore to maintain the existing behavior and semantic. Let's follow-up on the nomenclature in an issue.
eerhardt
left a comment
There was a problem hiding this comment.
Thanks for pushing this through @ViktorHofer. Looks good.
|
/azp run runtime |
|
Pull request contains merge conflicts. |
|
/azp run runtime-live-build |
|
Pull request contains merge conflicts. |
Required for #35285.
Upgrading the SDK to
5.0.100-preview.5.20228.8.