Adding x64 emulation support for pkg installers #7893
Conversation
|
|
||
| choiceElements = BundledPackages | ||
| .SelectMany(component => { | ||
| var visibleAttribute = new XAttribute("visible", "true"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs
Outdated
Show resolved
Hide resolved
|
@dagood, I had included you as I thought you did the mac installer work in the past. If not do you happen to know who originally wrote the code for GenerateMacOSDistributionFile? I see @jkoritzinsky added this, but I thought that was refactoring however I cannot find a previous instance of this task, so maybe it was new. |
|
The task is completely new for the new target framework SDK: #5714. The way the templating worked changed. (I didn't review that in depth.) The distribution XML I worked with earlier was https://github.com/dotnet/core-setup/blob/master-archive/src/pkg/packaging/osx/sharedframework/shared-framework-distribution-template.xml, which was pretty simple and never changed, so I never got in-depth knowledge. (Unfortunately. 😄) /cc @dleeapho |
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs
Outdated
Show resolved
Hide resolved
sfoslund
force-pushed
the
MacPkgGenerationEmulation
branch
from
f0e60ba to
762a0c0
Compare
| @@ -30,6 +30,8 @@ public class GenerateMacOSDistributionFile : BuildTask | |||
| [Required] | |||
| public string DestinationFile { get; set; } | |||
|
|
|||
| public string Alternativex64InstallPath { get; set; } | |||
There was a problem hiding this comment.
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
And default it in targets instead of code?
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
* Adding x64 emulation support for pkg installers * PR feedback * PR feedback
No description provided.