Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upMake sure MSI build works when not preview #7752
Conversation
TravisEz13
requested review from
adityapatwardhan,
anmenaga,
dantraMSFT and
daxian-dbw
as
code owners
Sep 10, 2018
adityapatwardhan
approved these changes
Sep 10, 2018
daxian-dbw
approved these changes
Sep 10, 2018
daxian-dbw
reviewed
Sep 10, 2018
| @@ -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)"/> | |||
This comment has been minimized.
This comment has been minimized.
daxian-dbw
Sep 10, 2018
Member
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?
This comment has been minimized.
This comment has been minimized.
TravisEz13
Sep 10, 2018
•
Author
Member
The variable is set to empty (meaning to delete the variable) in the non-preview case and the build fails
This comment has been minimized.
This comment has been minimized.
|
@TravisEz13 Can you please explain briefly? To me it looks like this is a stylistic change only and not a fix |
This comment has been minimized.
This comment has been minimized.
|
@bergmeister The variable is set to empty (meaning to delete the variable) in the non-preview case and the build fails |
TravisEz13
merged commit 172a8a0
into
PowerShell:master
Sep 10, 2018
7 checks passed
This comment has been minimized.
This comment has been minimized.
|
used admin to squash when windows CI was finished as this did not affect other platforms |
TravisEz13
added a commit
that referenced
this pull request
Sep 10, 2018
TravisEz13
added this to the v6.1.0 milestone
Sep 10, 2018
TravisEz13
deleted the
TravisEz13:Fix_msi_for_6_1
branch
Sep 10, 2018
This comment has been minimized.
This comment has been minimized.
|
@bergmeister feel free to continue the discussion. This was blocking the 6.1 builds so I merged so it's easier to take for the build. |
bergmeister
reviewed
Sep 10, 2018
| [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") |
This comment has been minimized.
This comment has been minimized.
bergmeister
Sep 10, 2018
•
Contributor
@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?
This comment has been minimized.
This comment has been minimized.
TravisEz13
Sep 10, 2018
Author
Member
This variable was already there and only used for this purpose. I don't think I'm getting the meaning of your statement.
This comment has been minimized.
This comment has been minimized.
TravisEz13
Sep 10, 2018
Author
Member
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.
This comment has been minimized.
This comment has been minimized.
bergmeister
Sep 10, 2018
Contributor
[$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")
TravisEz13 commentedSep 10, 2018
•
edited
PR Summary
Make sure MSI build works when not preview
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests