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.

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

Fix MSI upgrade and shortcut issues #12792

Merged
merged 3 commits into from May 27, 2020
Merged

Conversation

heaths
Copy link
Contributor

@heaths heaths commented May 26, 2020

Fixes #11995 and fixes #12011 based on changes described in #12011 (comment).

@iSazonov
Copy link
Collaborator

@heaths Please look CI fail.

@iSazonov iSazonov added the CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log label May 26, 2020
@iSazonov iSazonov added this to the 7.1.0-preview.4 milestone May 26, 2020
@SteveL-MSFT
Copy link
Member

@heaths take a look at https://dev.azure.com/powershell/PowerShell/_build/results?buildId=53706&view=logs&j=1057fe42-c88b-5deb-88ad-59245c714665&t=1888043b-254d-5761-80ec-47d0d8adc59a, the test detected some file changes, if this is expected, just follow the instructions in the output

@TravisEz13
Copy link
Member

I updated the wxs for you and filed: #12799

@heaths
Copy link
Contributor Author

heaths commented May 26, 2020

@TravisEz13, thanks. I was going to get to this tonight.

Were these actually removed, though? I'm not clear on whether their presence or not is actually a bug. They aren't needed except for ARPPRODUCTICON which must reference a row in the Icon table but, unless you're advertising your product (almost no one does that anymore - Office stopped over a decade ago), shouldn't be used for shortcuts.

@iSazonov
Copy link
Collaborator

Were these actually removed, though?

I wonder too how WiX works - wxs file still contains references to the icons.

@heaths
Copy link
Contributor Author

heaths commented May 27, 2020

@iSazonov, what exactly do you mean? As a long-time developer on WiX (I designed and implemented many features in it, most notably package ref-counting that other installers use as well), I could probably explain if you can clarify what you meant.

When the <Icon> is used, the binding (during linking, which is light.exe) knows to stream in a file reference; otherwise, when just a normal <File> is process the binder puts it in a cabinet that is later left external or streamed into the MSI based on <Media> entries (<MediaTemplate> provides default behavior and is often easier for most packages).

My main change here was to remove most GUIDs. They aren't needed, except for the UpgradeCode property (you normally want that fixed) and shared components without a single key path resource. In fact, just being a shared component - if it always resolves to the same key path - will get the same GUID. This change is mainly about letting WiX create a durable GUID based on the key path at build time, which is far more reliable and easier than people having to maintain it, which wasn't being done here and the main problem with files and registry values not getting cleaned up (they were ref-counted). See https://heaths.dev/setup/2020/05/13/simple-wix-project.html for more explanation and links to many of my old blog posts on what is now the VS Setup team blog (used to be my professional blog - which I used mostly for team purposes - up until a year or so ago).

Heath Stewart’s Blog
Globally-unique identifiers (GUIDs) are often the brunt of jokes. Windows Installer packages (.msi files) are full of them, from the required ProductCode, to highly-recommended UpgradeCode, package codes, and required component GUIDs. Authoring an MSI doesn’t require you create and manage so many GUIDs, however. In fact, Windows Installer XML (WiX) has in the last few years made great strides in making sure you don’t have to, and recommends you don’t.

@iSazonov
Copy link
Collaborator

what exactly do you mean?

I mean @TravisEz13 's commit and your question "Were these actually removed, though?".

@heaths
Copy link
Contributor Author

heaths commented May 27, 2020

Last I updated the file via git rebase they were still there.

@iSazonov
Copy link
Collaborator

Yes, they are on file system. My concern is that it seems wxs file contains references to the files.

@heaths
Copy link
Contributor Author

heaths commented May 27, 2020

They shouldn't anymore. It's possible when I generated my repo wasn't entirely clean. It was a bit behind before I rebased on upstream/master this past weekend since I've been swamped with work stuff.

@TravisEz13
Copy link
Member

@heaths

Were these actually removed, though? I'm not clear on whether their presence or not is actually a bug.

in CI we generate multiple packages at at time. There is probably a bug where one of the package generations deletes the icons. There is code in the packaging to create a staging folder. We need to make sure all types of packages that add/or remove files from the build use a staging directory.

I filed a bug to investigate.

I updated the wxs for you and filed: #12799

@TravisEz13
Copy link
Member

#12799 was an existing bug. We can merge this PR.

@TravisEz13 TravisEz13 assigned TravisEz13 and unassigned daxian-dbw May 27, 2020
@TravisEz13 TravisEz13 merged commit f7c701c into PowerShell:master May 27, 2020
@heaths heaths deleted the issue11995 branch May 27, 2020 23:18
@heaths
Copy link
Contributor Author

heaths commented May 27, 2020

Thanks. If I can find time, there's some more clean-up I'd like to do (you can do almost everything in the .wxs itself, which makes it easier to maintain since it's not spread all over), but wasn't necessary for now. I also recommend passing parameters instead of setting environment variables, as the scope is smaller (though, at least you scope them to the current and child processes).

@iSazonov
Copy link
Collaborator

@heaths Thanks for great contribution!

@TravisEz13
Copy link
Member

Thanks for your contribution

@ghost
Copy link

ghost commented Jun 25, 2020

🎉v7.1.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

<?define ExplorerContextMenuDialogText = "&$(var.ProductName) $(env.SimpleProductVersion) ($(sys.BUILDARCH))"?>
<?define UpgradeCodePreview = "1d00683b-0f84-4db8-a64f-2f98ad42fe06"?>
<?define UpgradeCodeRelease = "86abcfbd-1ccc-4a88-b8b2-0facfde29094"?>
Copy link
Member

Choose a reason for hiding this comment

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

preview upgrade codes

x64 - 39243d76-adaf-42b1-94fb-16ecf83237c8	
x86 - 86abcfbd-1ccc-4a88-b8b2-0facfde29094

Copy link
Member

Choose a reason for hiding this comment

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

release upgrade codes

"UpgradeCodeX64", '31ab5147-9a97-4452-8443-d9709f0516e1', "Process")	
"UpgradeCodeX86", '1d00683b-0f84-4db8-a64f-2f98ad42fe06', "Process")

Copy link
Member

Choose a reason for hiding this comment

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

Created #13036 to fix this

Comment on lines -3012 to -3025
[Environment]::SetEnvironmentVariable("PwshPath", "[$productDirectoryName]", "Process")
[Environment]::SetEnvironmentVariable("UpgradeCodeX64", '31ab5147-9a97-4452-8443-d9709f0516e1', "Process")
[Environment]::SetEnvironmentVariable("UpgradeCodeX86", '1d00683b-0f84-4db8-a64f-2f98ad42fe06', "Process")
[Environment]::SetEnvironmentVariable("IconPath", 'assets\Powershell_black.ico', "Process")
# The ApplicationProgramsMenuShortcut GUID should be changed when bumping the major version because the installation directory changes
[Environment]::SetEnvironmentVariable("ApplicationProgramsMenuShortcut", '6a69de6c-183d-4bf4-a40e-83007d6293bf', "Process")
}
else
{
[Environment]::SetEnvironmentVariable("PwshPath", "[$productDirectoryName]preview", "Process")
[Environment]::SetEnvironmentVariable("UpgradeCodeX64", '39243d76-adaf-42b1-94fb-16ecf83237c8', "Process")
[Environment]::SetEnvironmentVariable("UpgradeCodeX86", '86abcfbd-1ccc-4a88-b8b2-0facfde29094', "Process")
[Environment]::SetEnvironmentVariable("IconPath", 'assets\Powershell_av_colors.ico', "Process")
[Environment]::SetEnvironmentVariable("ApplicationProgramsMenuShortcut", 'ab727c4f-2311-474c-9ade-f2c6fd7f7322', "Process")
Copy link
Member

Choose a reason for hiding this comment

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

Add comment to Hightlight change

}
else
{
[Environment]::SetEnvironmentVariable("PwshPath", "[$productDirectoryName]preview", "Process")
Copy link
Member

Choose a reason for hiding this comment

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

Add comment to highlight change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log
Projects
None yet
5 participants