adding AddRuntimeOSTask #4713
adding AddRuntimeOSTask #4713
Conversation
...Tasks.TargetFramework.Sdk/src/build/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/AddRuntimeOSPropertyTask.cs
Outdated
Show resolved
Hide resolved
| @@ -116,4 +117,9 @@ | |||
| <PackageTargetFrameworks>@(PackageTargetFrameworksFinalList)</PackageTargetFrameworks> | |||
| </PropertyGroup> | |||
| </Target> | |||
|
|
|||
| <Target Name="AddRuntimeOSProperty"> | |||
There was a problem hiding this comment.
Who calls this and when is it imported?
There was a problem hiding this comment.
I see, you're expecting that to happen in the runtime repo. I guess that's ok since none of the Config system depends on this property. In that case, I wonder if we should put this in the root arcade SDK rather than config system. It was in the config system historically because we added it in 2.0 at the same time we added the config system and did it in the generated props. We could reconsider that. Maybe file a follow up issue to consider a move, and de-dup with what @dagood added.
There was a problem hiding this comment.
Restore Depends on this target.
There was a problem hiding this comment.
I'm pretty lost on what this change is for. But yeah, seems really similar to
Target: https://github.com/dotnet/runtime/blob/3a9f8ae2142647777161dbc7cd78ef890c37df06/tools-local/tasks/installer.tasks/installer.tasks.csproj#L48-L69
Task: https://github.com/dotnet/runtime/blob/3a9f8ae2142647777161dbc7cd78ef890c37df06/tools-local/tasks/installer.tasks/GetTargetMachineInfo.cs
How we write the file before Restore runs: https://github.com/dotnet/runtime/blob/3a9f8ae2142647777161dbc7cd78ef890c37df06/eng/Build.props#L65
There was a problem hiding this comment.
Actually, can't we use? RepoTasksOutputFile?
There was a problem hiding this comment.
It looks like we strip of architecture so that it can be specified independently. That seems like a reasonable thing to do. Maybe we can get the installer task to do that too and set the property without the arch. It could be moved down into the base Arcade SDK if that makes sense I dunno. It seems reasonable to dedup these. @Anipik and @dagood how about you work together after the initial config system work is done to unify these two?
There was a problem hiding this comment.
Yeah, sounds reasonable. /cc @NikolaMilosavljevic
Putting it in Arcade would be best IMO. dotnet/windowsdesktop has a copy: https://github.com/dotnet/windowsdesktop/blob/c5ca8a09a6e8eed54f6a37ce78c3468c00b5f6b4/Directory.Build.targets#L64.
There was a problem hiding this comment.
@Anipik and @dagood how about you work together after the initial config system work is done to unify these two?
Was this ever done? I don't see these two being unified.
cc @ViktorHofer - since we are hitting a similar issue with dotnet/runtime#35538 (comment)
There was a problem hiding this comment.
Not that I'm aware of. I no longer work in this area so it's possible I missed further discussion, but it has been a while. (@NikolaMilosavljevic)
There was a problem hiding this comment.
No, this is not done yet.
src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/AddRuntimeOSPropertyTask.cs
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/AddRuntimeOSPropertyTask.cs
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/AddRuntimeOSPropertyTask.cs
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/AddRuntimeOSPropertyTask.cs
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/AddRuntimeOSPropertyTask.cs
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/AddRuntimeOSPropertyTask.cs
Show resolved
Hide resolved
...Tasks.TargetFramework.Sdk/src/build/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets
Show resolved
Hide resolved
This tasks creates a new file with runtimeOS property.
The filepath is a property which is set on the runtime repo side to a tmp folder and is conditionally Imported if the path exists.