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

Update SDK and remove RID calculation in favor of RuntimeInformation #35538

Merged
merged 9 commits into from
May 4, 2020

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Apr 27, 2020

Required for #35285.

Upgrading the SDK to 5.0.100-preview.5.20228.8.

@ViktorHofer ViktorHofer added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 27, 2020
@ghost
Copy link

ghost commented Apr 27, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@ViktorHofer
Copy link
Member Author

@eerhardt any idea what could be causing the assembly not to be loaded?

tools-local/tasks/installer.tasks/installer.tasks.csproj(52,5): error MSB4018: (NETCORE_ENGINEERING_TELEMETRY=Restore) The "GetTargetMachineInfo" task failed unexpectedly.
System.IO.FileNotFoundException: Could not load file or assembly 'Microsoft.DotNet.PlatformAbstractions, Version=2.1.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'. The system cannot find the file specified.

File name: 'Microsoft.DotNet.PlatformAbstractions, Version=2.1.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'
   at Microsoft.DotNet.Build.Tasks.GetTargetMachineInfo.Execute()
   at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
   at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask)

Who would be the right folks to be looped in here? cc @vitek-karas

@ViktorHofer
Copy link
Member Author

@jkotas maybe related to 7d4f73c#diff-3b28fb8ad423b63bc5edd1e92e1de6c0?

@eerhardt
Copy link
Member

@eerhardt any idea what could be causing the assembly not to be loaded?

Yes, I removed PlatformAbstractions from the dotnet/sdk recently: dotnet/sdk#11076. Therefore that assembly is no longer loaded by default when running a Task on .NET Core MSBuild.

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.

@eerhardt
Copy link
Member

Maybe a better approach would be to remove GetTargetMachineInfo from the runtime repo, and instead use the copy that is in dotnet/arcade:

https://github.com/dotnet/arcade/blob/1924d7ea148c9f26ca3d82b60f0a775a5389ed22/src/Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk/src/GetTargetMachineInfo.cs#L10

@jkotas
Copy link
Member

jkotas commented Apr 27, 2020

The top level build knows the RID that we building for (e.g. look for __DistroRid). Would it make sense to just pass that to this place, instead of trying to compute it again using a different algorithm?

@eerhardt
Copy link
Member

Looking deeper, I think this GetTargetMachineInfo task is a hold over from core-setup days, and should probably go away. We should unify it with the other ways we are computing the rid:

  1. __DistroRid as @jkotas mentions above.
  2. The duplicated GetTargetMachineInfo task in arcade
  3. GenerateRuntimeOSPropsFile in arcade, which does roughly the same thing.

cc @dagood @Anipik

@ViktorHofer
Copy link
Member Author

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.

@ViktorHofer ViktorHofer force-pushed the ViktorHofer-sdk-update branch from 6a9197c to 8555467 Compare April 28, 2020 06:49
@ViktorHofer ViktorHofer changed the title [No Merge] Update SDK to P5 Remove GetTargetMachineInfo task from runtime Apr 28, 2020
@ViktorHofer ViktorHofer removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 28, 2020
@ViktorHofer ViktorHofer force-pushed the ViktorHofer-sdk-update branch 3 times, most recently from 7c1a3e2 to ea53e66 Compare April 28, 2020 06:53
@ViktorHofer ViktorHofer force-pushed the ViktorHofer-sdk-update branch 7 times, most recently from c7ef8e8 to bc362b6 Compare April 28, 2020 14:17
@ViktorHofer ViktorHofer force-pushed the ViktorHofer-sdk-update branch from bc362b6 to 3b2ef2d Compare April 28, 2020 14:18
@ViktorHofer ViktorHofer changed the title Remove GetTargetMachineInfo task from runtime Remove GetTargetMachineInfo task and manual RID calculation in favor of RuntimeInformation Apr 28, 2020
@ViktorHofer ViktorHofer requested a review from eerhardt April 28, 2020 14:24
@eerhardt
Copy link
Member

<HostMachineInfoProps>$(ArtifactsObjDir)HostMachineInfo.props</HostMachineInfoProps>

Can this HostMachineInfoProps be removed as well? I don't think anyone is using it anymore.


Refers to: src/installer/Directory.Build.props:33 in a7c4511. [](commit_id = a7c4511, deletion_comment = False)

eng/Configurations.props Outdated Show resolved Hide resolved
@Anipik
Copy link
Contributor

Anipik commented Apr 28, 2020

@ViktorHofer we can remove this as well https://github.com/dotnet/runtime/blob/master/eng/pipelines/libraries/build-job.yml#L148

@ViktorHofer ViktorHofer changed the title Remove GetTargetMachineInfo task and manual RID calculation in favor of RuntimeInformation Update SDK and remove RID calculation in favor of RuntimeInformation Apr 29, 2020
@ViktorHofer ViktorHofer requested a review from eerhardt April 29, 2020 12:27
@@ -38,6 +37,10 @@
</PropertyGroup>

<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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@eerhardt eerhardt Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing this through @ViktorHofer. Looks good.

@ViktorHofer
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@ViktorHofer
Copy link
Member Author

/azp run runtime-live-build

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@ViktorHofer ViktorHofer merged commit a1af15d into master May 4, 2020
@safern safern deleted the ViktorHofer-sdk-update branch May 4, 2020 18:01
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants