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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 29, 2020
Merged

adding AddRuntimeOSTask #4713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b0b413
adding AddRuntimeOSTask
Anipik Jan 29, 2020
3b56164
AddRuntimeOSPropertyTask -> GenerateRuntimeOSPropsFile
Anipik Jan 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/AddRuntimeOSPropertyTask.cs
View file
Open in desktop
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";
Anipik marked this conversation as resolved.
Show resolved Hide resolved

[Required]
public string RuntimePropsFilePath {get ; set; }
Anipik marked this conversation as resolved.
Show resolved Hide 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.
Show resolved Hide resolved
{
var rid = PlatformAbstractions.RuntimeEnvironment.GetRuntimeIdentifier();
Anipik marked this conversation as resolved.
Show resolved Hide 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.
Show resolved Hide resolved
runtimeProperty.Condition = $"'$({RuntimeOSProperty})' == ''";

Log.LogMessage($"Running on OS with RID {rid}, so defaulting RuntimeOS to '{osNameAndVersion}'");
Anipik marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
6 changes: 6 additions & 0 deletions ...ks.TargetFramework.Sdk/src/build/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Show resolved Hide resolved

<PropertyGroup>
<_OriginalTargetFrameworks>$(TargetFrameworks)</_OriginalTargetFrameworks>
Expand Down Expand Up @@ -116,4 +117,9 @@
<PackageTargetFrameworks>@(PackageTargetFrameworksFinalList)</PackageTargetFrameworks>
</PropertyGroup>
</Target>

<Target Name="AddRuntimeOSProperty">
Copy link
Member

Choose a reason for hiding this comment

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

Who calls this and when is it imported?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restore Depends on this target.

Copy link
Member

Choose a reason for hiding this comment

The 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
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

Copy link
Member

Choose a reason for hiding this comment

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

Actually, can't we use? RepoTasksOutputFile?

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@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)

Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is not done yet.

<GenerateRuntimeOSPropsFile RuntimePropsFilePath="$(RuntimePropsFile)" />
</Target>

</Project>