GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Make sure MSI build works when not preview by TravisEz13 · Pull Request #7752 · PowerShell/PowerShell

Conversation

@TravisEz13

@TravisEz13 Travis Plunk (TravisEz13) commented Sep 10, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

PR Summary

Make sure MSI build works when not preview

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • User-facing Documentation needed
      • Issue filed - Issue link:
  • Testing - New and feature
    • Not Applicable or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@TravisEz13
fix issue with PwshPath
60f17e0
@TravisEz13
Travis Plunk (TravisEz13) requested review from Aditya Patwardhan (adityapatwardhan), Andrew (anmenaga), Dan Travison (dantraMSFT) and Dongbo Wang (daxian-dbw) as code owners September 10, 2018 21:11
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Sep 10, 2018
View reviewed changes
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Sep 10, 2018
View reviewed changes
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Sep 10, 2018
View reviewed changes
Comment thread assets/Product.wxs
<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)"/>

@daxian-dbw Dongbo Wang (daxian-dbw) Sep 10, 2018

Copy link
Copy Markdown
Member

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TravisEz13 Travis Plunk (TravisEz13) Sep 10, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bergmeister

Christoph Bergmeister (bergmeister) commented Sep 10, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Travis Plunk (@TravisEz13) Can you please explain briefly? To me it looks like this is a stylistic change only and not a fix
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TravisEz13

Travis Plunk (TravisEz13) commented Sep 10, 2018

Copy link
Copy Markdown
Member Author

Christoph Bergmeister (@bergmeister) The variable is set to empty (meaning to delete the variable) in the non-preview case and the build fails
bergmeister reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TravisEz13
Travis Plunk (TravisEz13) merged commit 172a8a0 into PowerShell:master Sep 10, 2018
@TravisEz13

Travis Plunk (TravisEz13) commented Sep 10, 2018

Copy link
Copy Markdown
Member Author

used admin to squash when windows CI was finished as this did not affect other platforms
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Travis Plunk (TravisEz13) added a commit that referenced this pull request Sep 10, 2018
@TravisEz13
Make sure MSI build works when not preview (#7752)
15a3beb 
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
@TravisEz13 Travis Plunk (TravisEz13) added this to the v6.1.0 milestone Sep 10, 2018
@TravisEz13
Travis Plunk (TravisEz13) deleted the Fix_msi_for_6_1 branch September 10, 2018 21:37
@TravisEz13

Travis Plunk (TravisEz13) commented Sep 10, 2018

Copy link
Copy Markdown
Member Author

Christoph Bergmeister (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bergmeister
Christoph Bergmeister (bergmeister) reviewed Sep 10, 2018
View reviewed changes
Comment thread tools/packaging/packaging.psm1
else
{
[Environment]::SetEnvironmentVariable("PwshPath", 'preview', "Process")
[Environment]::SetEnvironmentVariable("PwshPath", "[$productDirectoryName]preview", "Process")

@bergmeister Christoph Bergmeister (bergmeister) Sep 10, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Travis Plunk (@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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TravisEz13 Travis Plunk (TravisEz13) Sep 10, 2018

Copy link
Copy Markdown
Member Author

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 variable was already there and only used for this purpose. I don't think I'm getting the meaning of your statement.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TravisEz13 Travis Plunk (TravisEz13) Sep 10, 2018

Copy link
Copy Markdown
Member Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bergmeister reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@bergmeister Christoph Bergmeister (bergmeister) Sep 10, 2018

Copy link
Copy Markdown
Contributor

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.

[$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")

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TravisEz13 Travis Plunk (TravisEz13) Sep 11, 2018

Copy link
Copy Markdown
Member Author

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.

I'm fine with that change for 6.2.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bergmeister reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@TravisEz13
Make sure MSI build works when not preview (PowerShell#7752)
45f5ac6 
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
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@TravisEz13
Make sure MSI build works when not preview (PowerShell#7752)
7c79370 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
@anmenaga Andrew (anmenaga) Awaiting requested review from anmenaga
@dantraMSFT Dan Travison (dantraMSFT) Awaiting requested review from dantraMSFT
+1 more reviewer
@bergmeister Christoph Bergmeister (bergmeister) bergmeister left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v6.1.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@TravisEz13 @bergmeister @daxian-dbw @adityapatwardhan
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.