Skip to content

Make sure MSI build works when not preview #7752

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

Merged
TravisEz13 merged 1 commit into from
Sep 10, 2018
Merged

Make sure MSI build works when not preview #7752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion assets/Product.wxs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@
<!-- add ourselves to %PATH% so pwsh.exe can be started from Windows PowerShell or cmd.exe -->
<Component Id="SetPath" Guid="{9dbb7763-7baf-48e7-b025-3bdedcb0632f}" KeyPath="yes">
<Condition>ADD_PATH=1</Condition>
<Environment Id="PATH" Action="set" Name="PATH" Part="last" Permanent="no" System="yes" Value="[$(var.ProductDirectoryName)]$(var.PwshPath)"/>
<Environment Id="PATH" Action="set" Name="PATH" Part="last" Permanent="no" System="yes" Value="$(var.PwshPath)"/>
Copy link
Member

@daxian-dbw daxian-dbw Sep 10, 2018

Choose a reason for hiding this comment

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

Just curious on why we are making these changes. It looks to me this change doesn't change the behavior -- Value="[$(var.ProductDirectoryName)]$(var.PwshPath)" basically does the same before the change, isn't it?

Copy link
Member Author

@TravisEz13 TravisEz13 Sep 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The variable is set to empty (meaning to delete the variable) in the non-preview case and the build fails

</Component>
<!-- Explorer context menu with 2 submenus to open PowerShell normally or as an Administator.
See https://blogs.msdn.microsoft.com/andrew_richards/2017/03/01/enhancing-the-open-command-prompt-here-shift-right-click-context-menu-experience/ for details -->
Expand Down
4 changes: 2 additions & 2 deletions tools/packaging/packaging.psm1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2521,14 +2521,14 @@ function New-MSIPackage
[Environment]::SetEnvironmentVariable("ProductVersionWithName", $productVersionWithName, "Process")
if (!$isPreview)
{
[Environment]::SetEnvironmentVariable("PwshPath", '', "Process")
[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")
}
else
{
[Environment]::SetEnvironmentVariable("PwshPath", 'preview', "Process")
[Environment]::SetEnvironmentVariable("PwshPath", "[$productDirectoryName]preview", "Process")
Copy link
Contributor

@bergmeister bergmeister Sep 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@TravisEz13 Non-blocking comment: Would it not be cleaner if a the variable was declared directly here to not convolute the script with WiX syntax?

Copy link
Member Author

@TravisEz13 TravisEz13 Sep 10, 2018

Choose a reason for hiding this comment

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

This variable was already there and only used for this purpose. I don't think I'm getting the meaning of your statement.

Copy link
Member Author

@TravisEz13 TravisEz13 Sep 10, 2018

Choose a reason for hiding this comment

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

I do think there is probably a cleaner way of doing this. Like refering to the preview directory by name, but the goal here was to make a quick change that would not require a lot of re-testing for the 6.1.0 release.

Copy link
Contributor

@bergmeister bergmeister Sep 10, 2018

Choose a reason for hiding this comment

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

[$productDirectoryName]preview is WiX syntax that we inject into the WiX file. The build script should be generic and know as little about WiX as possible to reduce coupling if installer technology changed for example or WiX 4 brings breaking changes in syntax. How about this:

[Environment]::SetEnvironmentVariable("PwshPath", (Join-Path $productDirectoryName 'preview'), "Process")

Copy link
Member Author

@TravisEz13 TravisEz13 Sep 11, 2018

Choose a reason for hiding this comment

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

I'm fine with that change for 6.2.

[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")
Expand Down