-
Notifications
You must be signed in to change notification settings - Fork 386
adding AddRuntimeOSTask #4713
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
adding AddRuntimeOSTask #4713
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using Microsoft.Build.Construction; | ||
| using Microsoft.Build.Framework; | ||
|
|
||
| namespace Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk | ||
| { | ||
| public class GenerateRuntimeOSPropsFile : BuildTask | ||
| { | ||
| public const string RuntimeOSProperty = "RuntimeOS"; | ||
|
|
||
| [Required] | ||
| public string RuntimePropsFilePath {get ; set; } | ||
|
Anipik marked this conversation as resolved.
|
||
|
|
||
| public override bool Execute() | ||
| { | ||
| ProjectRootElement project = ProjectRootElement.Create(); | ||
| CreateRuntimeIdentifier(project); | ||
| project.Save(RuntimePropsFilePath); | ||
| return !Log.HasLoggedErrors; | ||
| } | ||
|
|
||
| public void CreateRuntimeIdentifier(ProjectRootElement project) | ||
|
Anipik marked this conversation as resolved.
|
||
| { | ||
| var rid = PlatformAbstractions.RuntimeEnvironment.GetRuntimeIdentifier(); | ||
|
Anipik marked this conversation as resolved.
|
||
| string[] ridParts = rid.Split('-'); | ||
|
|
||
| if (ridParts.Length < 1) | ||
| { | ||
| throw new System.InvalidOperationException($"Unknown rid format {rid}."); | ||
| } | ||
|
|
||
| string osNameAndVersion = ridParts[0]; | ||
|
|
||
| var propertyGroup = project.CreatePropertyGroupElement(); | ||
| project.AppendChild(propertyGroup); | ||
|
|
||
| var runtimeProperty = propertyGroup.AddProperty(RuntimeOSProperty, $"{osNameAndVersion}"); | ||
|
Anipik marked this conversation as resolved.
|
||
| runtimeProperty.Condition = $"'$({RuntimeOSProperty})' == ''"; | ||
|
|
||
| Log.LogMessage($"Running on OS with RID {rid}, so defaulting RuntimeOS to '{osNameAndVersion}'"); | ||
|
Anipik marked this conversation as resolved.
|
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| <UsingTask TaskName="ChooseBestP2PTargetFrameworkTask" AssemblyFile="$(DotNetBuildTasksTargetFrameworkSdkAssembly)"/> | ||
| <UsingTask TaskName="ChooseBestTargetFrameworksTask" AssemblyFile="$(DotNetBuildTasksTargetFrameworkSdkAssembly)"/> | ||
| <UsingTask TaskName="AddTargetFrameworksToProjectTask" AssemblyFile="$(DotNetBuildTasksTargetFrameworkSdkAssembly)"/> | ||
| <UsingTask TaskName="GenerateRuntimeOSPropsFile" AssemblyFile="$(DotNetBuildTasksTargetFrameworkSdkAssembly)"/> | ||
|
Anipik marked this conversation as resolved.
|
||
|
|
||
| <PropertyGroup> | ||
| <_OriginalTargetFrameworks>$(TargetFrameworks)</_OriginalTargetFrameworks> | ||
|
|
@@ -116,4 +117,9 @@ | |
| <PackageTargetFrameworks>@(PackageTargetFrameworksFinalList)</PackageTargetFrameworks> | ||
| </PropertyGroup> | ||
| </Target> | ||
|
|
||
| <Target Name="AddRuntimeOSProperty"> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Who calls this and when is it imported?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Restore Depends on this target.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, can't we use? RepoTasksOutputFile?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this is not done yet. |
||
| <GenerateRuntimeOSPropsFile RuntimePropsFilePath="$(RuntimePropsFile)" /> | ||
| </Target> | ||
|
|
||
| </Project> | ||
Uh oh!
There was an error while loading. Please reload this page.