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 x64 emulation support for pkg installers #7893

Merged
merged 3 commits into from
Sep 16, 2021
Merged

Adding x64 emulation support for pkg installers #7893

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77696e6
Adding x64 emulation support for pkg installers
sfoslund Sep 13, 2021
48569a5
PR feedback
sfoslund Sep 16, 2021
762a0c0
PR feedback
sfoslund Sep 16, 2021
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
68 changes: 60 additions & 8 deletions src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ public class GenerateMacOSDistributionFile : BuildTask
[Required]
public string DestinationFile { get; set; }

public string Alternativex64InstallPath { get; set; }
Copy link
Member

@ericstj ericstj Sep 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What's the use case for this parameter?
Maybe we should expose it in the targets

arcade/src/Microsoft.DotNet.Build.Tasks.Installers/build/bundle.targets

Lines 140 to 145 in e7ede87

<GenerateMacOSDistributionFile
TemplatePath="$(MacOSBundleTemplate)"
ProductBrandName="$(ProductBrandName)"
TargetArchitecture="$(TargetArchitecture)"
BundledPackages="@(_MacOSPackagesToBundle)"
DestinationFile="$(_MacOSDistributionFile)" />

And default it in targets instead of code?

arcade/src/Microsoft.DotNet.Build.Tasks.Installers/build/installer.singlerid.targets

Line 403 in 06b4a81

<_MacOSSharedInstallDir>/usr/local/share/dotnet</_MacOSSharedInstallDir>

That could allow for overriding, that said the current path is a private property set unconditionally, so it's not exactly extensible. Also I don't have a scenario today for any product consuming this SDK that might want to opt out of redirection or set its own path, so maybe we don't even need the parameter.

@jkoritzinsky @NikolaMilosavljevic - can you please make a call on that?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an optional parameter for task, so it can be specified or not specified in calls to this task in a target. AFAIK, defaulting in the targets instead of in the task as it is now would be functionally the same.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but the targets don't expose it, so we could just delete it since folks don't have a way of specifying it today. IMHO we should make both of these paths behave a similar way and be co-located since they address the same concern, but I don't own this codebase so I'd like to hear from the owners. We could always clean this up later since it doesn't impact actual functionality.


public override bool Execute()
{
try
Expand All @@ -38,15 +40,62 @@ public override bool Execute()

var titleElement = new XElement("title", $"{ProductBrandName} ({TargetArchitecture})");

var choiceLineElements = BundledPackages.Select(component => new XElement("line", new XAttribute("choice", component.GetMetadata("FileNameWithExtension"))));
IEnumerable<XElement> choiceLineElements;
IEnumerable<XElement> choiceElements;
XElement scriptElement = null;
if (TargetArchitecture.Equals("x64"))
sfoslund marked this conversation as resolved.
Show resolved Hide resolved
{
Alternativex64InstallPath ??= "/usr/local/share/dotnet/x64";
var archScriptContent = @"<![CDATA[
sfoslund marked this conversation as resolved.
Show resolved Hide resolved
function IsX64Machine() {
var machine = system.sysctl(""hw.machine"");
system.log(""Machine type: "" + machine);
var result = machine == ""x64"" || machine.endsWith(""_x64"");
system.log(""IsX64Machine: "" + result);
return result;
}
]]>";
scriptElement = new XElement("script", new XText(archScriptContent));

var choiceElements = BundledPackages
.Select(component => new XElement("choice",
new XAttribute("id", component.GetMetadata("FileNameWithExtension")),
new XAttribute("visible", "true"),
new XAttribute("title", component.GetMetadata("Title")),
new XAttribute("description", component.GetMetadata("Description")),
new XElement("pkg-ref", new XAttribute("id", component.GetMetadata("FileNameWithExtension")))));
choiceLineElements = BundledPackages.SelectMany(component => new XElement[] { new XElement("line", new XAttribute("choice", $"{component.GetMetadata("FileNameWithExtension")}.x64")),
new XElement("line", new XAttribute("choice", $"{component.GetMetadata("FileNameWithExtension")}.arm64"))});
sfoslund marked this conversation as resolved.
Show resolved Hide resolved

choiceElements = BundledPackages
.SelectMany(component => {
sfoslund marked this conversation as resolved.
Show resolved Hide resolved
var visibleAttribute = new XAttribute("visible", "true");
Copy link
Member

Choose a reason for hiding this comment

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

Will this make both choices visible, or will only the selected choice be visible? If both would be visible, maybe we'd want this to mirror the value of selected.

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, only the selected choice is visible. I'm not sure how to double check this though, as there doesn't seem to be any documentation.

Copy link
Member

@ericstj ericstj Sep 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

there doesn't seem to be any documentation.

That's an understatement. This whole area is very cryptically documented, I think we have to go with "try it and see".

So long as you don't see both choices visible then I think it should be OK. We want to make sure that the user doesn't really have the option to toggle this behavior, nor do they see side effects of it.

var titleAttribute = new XAttribute("title", component.GetMetadata("Title"));
var descriptionAttribute = new XAttribute("description", component.GetMetadata("Description"));
var packageRefAttribute = new XElement("pkg-ref", new XAttribute("id", component.GetMetadata("FileNameWithExtension")));
var x64Element = new XElement("choice",
new XAttribute("id", $"{component.GetMetadata("FileNameWithExtension")}.x64"),
new XAttribute("selected", "IsX64Machine()"),
visibleAttribute,
titleAttribute,
descriptionAttribute,
packageRefAttribute);
var arm64Element = new XElement("choice",
sfoslund marked this conversation as resolved.
Show resolved Hide resolved
new XAttribute("id", $"{component.GetMetadata("FileNameWithExtension")}.arm64"),
new XAttribute("selected", "!IsX64Machine()"),
new XAttribute("customLocation", Alternativex64InstallPath),
visibleAttribute,
titleAttribute,
descriptionAttribute,
packageRefAttribute);

return new XElement[] { x64Element, arm64Element };
});
}
else {
sfoslund marked this conversation as resolved.
Show resolved Hide resolved
choiceLineElements = BundledPackages.Select(component => new XElement("line", new XAttribute("choice", component.GetMetadata("FileNameWithExtension"))));

choiceElements = BundledPackages
.Select(component => new XElement("choice",
new XAttribute("id", component.GetMetadata("FileNameWithExtension")),
new XAttribute("visible", "true"),
new XAttribute("title", component.GetMetadata("Title")),
new XAttribute("description", component.GetMetadata("Description")),
new XElement("pkg-ref", new XAttribute("id", component.GetMetadata("FileNameWithExtension")))));
}

var pkgRefElements = BundledPackages
.Select(component => new XElement("pkg-ref",
Expand Down Expand Up @@ -78,6 +127,9 @@ public override bool Execute()
document.Root.Add(new XElement("choices-outline", choiceLineElements));
document.Root.Add(choiceElements);
document.Root.Add(pkgRefElements);
if (scriptElement != null) {
sfoslund marked this conversation as resolved.
Show resolved Hide resolved
document.Root.Add(scriptElement);
}
using XmlWriter writer = XmlWriter.Create(File.OpenWrite(DestinationFile));
document.WriteTo(writer);
}
Expand Down