-
Notifications
You must be signed in to change notification settings - Fork 352
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
Account for target installer arch in mac pkg id generation #7831
Conversation
I'm not sure how to validate this change, so please point me in the right direction for unit tests/ manual validation. |
@dagood, @agocke, or @jkoritzinsky might have an idea for how to test. My guess is the best bet is to just apply the patch and build locally and test the installers. For my MSI changes I built private copies of the SDKs then ingested those into a full runtime build in PR. I'm not sure you need to do that for such a small change as this. |
@@ -399,7 +399,7 @@ | |||
RemoveProperties="@(_GlobalPropertiesToRemoveForPublish)" /> | |||
<PropertyGroup> | |||
<_MacOSVersionComponent Condition="'$(IncludeVersionInMacOSComponentName)' == 'true'">.$(InstallerPackageVersion)</_MacOSVersionComponent> | |||
<_MacOSComponentName Condition="'$(_MacOSComponentName)' == ''">com.microsoft.dotnet.$(MacOSComponentNamePackType)$(_MacOSVersionComponent).component.osx.x64</_MacOSComponentName> | |||
<_MacOSComponentName Condition="'$(_MacOSComponentName)' == ''">com.microsoft.dotnet.$(MacOSComponentNamePackType)$(_MacOSVersionComponent).component.osx.$(InstallerTargetArchitecture)</_MacOSComponentName> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the right change to make, but it means that ARM64 installations are not going to upgrade well WRT removing the old files. We might need to document that those should be manually cleaned up if folks wish to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also consider/test what'll happen if someone installs x64, then installs arm64, then upgrades x64 to the next release.
And perhaps a more interesting subscenario in the future: what if the first x64 installer installs to /usr/local/share/dotnet
, the arm64 installer replaces some files, and then the second x64 installer has the logic to install to /usr/local/share/dotnet/x64
? (Would the x64 upgrade delete files from the arm64 install because it thinks the old x64 installer was the one to place them down? I'm not familiar with how these files are tracked.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the first x64 installer installs to /usr/local/share/dotnet, the arm64 installer replaces some files, and then the second x64 installer has the logic to install to /usr/local/share/dotnet/x64
I believe we scoped out upgrade from pre x64-on-ARM64 installers, since the assumption is most of the ARM64 machines will be new installs. @richlander is that correct? It still may be interesting to understand what happens since that could be a state folks get into.
We'll need more changes eventually, not sure if it's this PR or a follow up. The bundles produced from this use a template: https://github.com/dotnet/runtime/blob/c2bd3ca1c2815a6550996bbc62719421aaaaccca/src/installer/pkg/sfx/bundle/shared-framework-distribution-template-x64.xml#L2 And task: arcade/src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs Line 33 in 0ec8dba
|
@mmitche who from the arcade side would be best to review this? |
Yeah, AFAIK nobody got to automated testing. I'm not even sure if we have access to "blank slate" VMs/images to run things on. At least, the hosted macOS agents say they have a ton of .NETs installed. IIRC I had to use |
@@ -399,7 +399,7 @@ | |||
RemoveProperties="@(_GlobalPropertiesToRemoveForPublish)" /> | |||
<PropertyGroup> | |||
<_MacOSVersionComponent Condition="'$(IncludeVersionInMacOSComponentName)' == 'true'">.$(InstallerPackageVersion)</_MacOSVersionComponent> | |||
<_MacOSComponentName Condition="'$(_MacOSComponentName)' == ''">com.microsoft.dotnet.$(MacOSComponentNamePackType)$(_MacOSVersionComponent).component.osx.x64</_MacOSComponentName> | |||
<_MacOSComponentName Condition="'$(_MacOSComponentName)' == ''">com.microsoft.dotnet.$(MacOSComponentNamePackType)$(_MacOSVersionComponent).component.osx.$(InstallerTargetArchitecture)</_MacOSComponentName> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also consider/test what'll happen if someone installs x64, then installs arm64, then upgrades x64 to the next release.
And perhaps a more interesting subscenario in the future: what if the first x64 installer installs to /usr/local/share/dotnet
, the arm64 installer replaces some files, and then the second x64 installer has the logic to install to /usr/local/share/dotnet/x64
? (Would the x64 upgrade delete files from the arm64 install because it thinks the old x64 installer was the one to place them down? I'm not familiar with how these files are tracked.)
I plan on making these changes in a follow up PR, likely next week. In the meantime, is this good to merge? |
Yep, let's do it. We can validate this work in 7.0. |
Fixes dotnet/installer#11813 (comment)