Make sure MSI build works when not preview #7752
Conversation
TravisEz13
requested review from
adityapatwardhan,
anmenaga,
dantraMSFT and
daxian-dbw
as code owners
| <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)"/> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The variable is set to empty (meaning to delete the variable) in the non-preview case and the build fails
|
@TravisEz13 Can you please explain briefly? To me it looks like this is a stylistic change only and not a fix |
|
@bergmeister The variable is set to empty (meaning to delete the variable) in the non-preview case and the build fails |
|
used admin to squash when windows CI was finished as this did not affect other platforms |
The variable was set to empty (meaning to delete the variable) in the non-preview case and the build fails. The fix avoids setting the variable to empty
|
@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. |
| else | ||
| { | ||
| [Environment]::SetEnvironmentVariable("PwshPath", 'preview', "Process") | ||
| [Environment]::SetEnvironmentVariable("PwshPath", "[$productDirectoryName]preview", "Process") |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
This variable was already there and only used for this purpose. I don't think I'm getting the meaning of your statement.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
[$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")There was a problem hiding this comment.
I'm fine with that change for 6.2.
4 participants
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