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 — Fix x86 installer bug: it gets installed as an x64 component and has the same UpgradeCode as the x64 installer leading to an uninstallation of an x64 installation (and vice versa) by bergmeister · Pull Request #5812 · PowerShell/PowerShell

Conversation

@bergmeister

@bergmeister Christoph Bergmeister (bergmeister) commented Jan 7, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

PR Summary

Fix bug #5597: x86/x64 installer are uninstalling each other when installing either of them:

-Make x86 installer to be installed as a x86 component (-arch argument to candle.exe, which sets the sys.BUILDARCH variable)
-Make the UpgradeCode unique per platform

Replace var.ProductTargetArchitecture variable with sys.BUILDARCH use to have only 1 variable for the architecture
Additionally, the architecture was appended to the package name to be able to distinguish the installations.

Windows installer PRs usually got reviewed by Steve Lee (@SteveL-MSFT) in the past.

PR Checklist

Note: Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • User facing Documentation needed : Not sure? Package name now includes architecture...
    • Issue filed - Issue link: Can't install 32-bit/64-bit PowerShell together when using MSI installer #5597
  • Change is not breaking
  • NA Make sure you've added a new test if existing tests do not effectively test the code changed
    • [] NA Add [feature] if the change is significant or affectes feature tests
  • 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bergmeister
Fix x86/x64 installer uninstalling each other when installing either …
4f00b46 
…of them:

-Make x86 installer to be installed as a x86 component (-arch argument to candle.exe)
-Make the UpgradeCode unique per platform

Additionally, the architecture was appended to the package name to be able to distinguish the installations.
Simplified architecture variable use to have only 1 variable for it.
@bergmeister
Christoph Bergmeister (bergmeister) requested review from Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan) and Dongbo Wang (daxian-dbw) as code owners January 7, 2018 19:27
@iSazonov
Ilya (iSazonov) requested a review from Steve Lee (SteveL-MSFT) January 7, 2018 19:32
@bergmeister

Christoph Bergmeister (bergmeister) commented Jan 7, 2018

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) Joey Aiello (@joeyaiello) I am not sure if this bug was ever triaged but it seems quite severe to me and would cause problems in the future when trying to upgrade install the x86 version in 6.1 if this fix does not go into 6.0. On the other hand, I can understand that this is last minute (maybe building only the x86 installer using this fix would mitigate the risk?)...
All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Jan 7, 2018

Copy link
Copy Markdown
Collaborator

Should Explorer menu be fixed too?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov Ilya (iSazonov) self-assigned this Jan 7, 2018
@bergmeister

Christoph Bergmeister (bergmeister) commented Jan 7, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) : The explorer context menu goes into 6.1 (I already asked this question, the answer by Steve Lee (@SteveL-MSFT) is here). Steve Lee (@SteveL-MSFT) said here that it is not clear whether the current behaviour is right or wrong and therefore we should wait for feedback. If the current behaviour is bothering you please open an issue so that it can be tracked and discussed further.
All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Jan 8, 2018

Copy link
Copy Markdown
Collaborator

Christoph Bergmeister (@bergmeister) My request was that in the previous PR we added the menu and in this PR we add a second PowerShell version - what version will be called from this menu? I suppose that if we want this menu, then in this PR you have to add the menu for both versions.
All reactions

Sorry, something went wrong.

Uh oh!

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

Christoph Bergmeister (bergmeister) commented Jan 8, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) I see, you have a fair point that I did not consider the case of installing both x64 and x86 for the context menu.
But this bug fix itself has nothing to do with the context menu since it should have always worked to install both versions, hence the question to steve if this fix is needed for the release branch as well. At the moment the PR targets fixing the installer problem so that it is also easily possible to merge it into the release branch if needed/required. I shall open a 2nd WIP PR for the architecture specific context menu to separate the issues.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Jan 8, 2018

Copy link
Copy Markdown
Member

I think the number of users using both x86 and x64 are very minimal. Giving that where we are with 6.0.0, I think we should document this (Known Issues Joey Aiello (@joeyaiello)) and fix it for 6.1.0.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT Steve Lee (SteveL-MSFT) added the Documentation Needed in this repo Documentation is needed in this repo label Jan 8, 2018
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Jan 8, 2018
View reviewed changes
@TravisEz13

Travis Plunk (TravisEz13) commented Jan 8, 2018

Copy link
Copy Markdown
Member

I think we need to take this for GA to ensure that we can create servicing packages correctly without creating a complicated servicing wrapper.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Jan 8, 2018

Copy link
Copy Markdown
Member

I talked to Travis Plunk (@TravisEz13) offline, agree that servicing would be an issue so we should take this for GA.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT Steve Lee (SteveL-MSFT) added this to the 6.0.0-GA milestone Jan 8, 2018
@SteveL-MSFT Steve Lee (SteveL-MSFT) removed the Documentation Needed in this repo Documentation is needed in this repo label Jan 8, 2018
@TravisEz13

Travis Plunk (TravisEz13) commented Jan 8, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

filed: #5821
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TravisEz13
Travis Plunk (TravisEz13) approved these changes Jan 8, 2018
View reviewed changes
@bergmeister

Christoph Bergmeister (bergmeister) commented Jan 8, 2018

Copy link
Copy Markdown
Contributor Author

OK. I will prepare now a separate PR that targets the release branch to check for merge problems beforehand.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TravisEz13
Travis Plunk (TravisEz13) reviewed Jan 8, 2018
View reviewed changes
Comment thread assets/Product.wxs
<!-- Generate Your Own GUID for both ID and UpgradeCode attributes. -->
<!-- Note: UpgradeCode GUID MUST REMAIN SAME THROUGHOUT ALL VERSIONS -->
<!-- Otherwise, updates won't occur -->
<Product Id="$(var.ProductGuid)" Name="$(var.ProductSemanticVersionWithName)" Language="1033" Version="$(var.ProductVersion)" Manufacturer="Microsoft Corporation" UpgradeCode="{f7ba3e58-0be8-443b-ac91-f99dd1e7bd3b}">

@TravisEz13 Travis Plunk (TravisEz13) Jan 8, 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.

Can you add an comment that <Product Id is the product code?
https://stackoverflow.com/questions/10330534/in-wix-where-is-the-productcode-specified
http://wixtoolset.org/documentation/manual/v3/xsd/wix/product.html

Sorry, something went wrong.

Uh oh!

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

@bergmeister Christoph Bergmeister (bergmeister) Jan 8, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor 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.

Travis Plunk (@TravisEz13) : I created a separate PR for 6.0.0 here. The difference is that it is based on the 6.0.0. branch where I cherry picked the fixing commit (an auto-merge of whitespace was needed) so that local testing becomes easier. I have addressed your comment about adding the product code related comment.
This PR shall now as WIP and for 6.1.0.
For the record: the mentioned context menu issue applies only to 6.1 and is not related to this PR

Sorry, something went wrong.

Uh oh!

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

Travis Plunk (TravisEz13) commented Jan 8, 2018

Copy link
Copy Markdown
Member

Christoph Bergmeister (@bergmeister) I will take care of any merge conflicts with the release branch. Just get it working on this branch. I've finished my review. It looks good otherwise.
All reactions

Sorry, something went wrong.

Uh oh!

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

Christoph Bergmeister (bergmeister) commented Jan 8, 2018

Copy link
Copy Markdown
Contributor Author

OK. Whatever you prefer then.
All reactions

Sorry, something went wrong.

Uh oh!

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

Travis Plunk (TravisEz13) commented Jan 8, 2018

Copy link
Copy Markdown
Member

Can you get the comment added in the next few hours?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bergmeister
Add requested comment about that product code is product id.
b535a5d
TravisEz13
Travis Plunk (TravisEz13) approved these changes Jan 8, 2018
View reviewed changes
@TravisEz13 Travis Plunk (TravisEz13) mentioned this pull request Jan 8, 2018
6.0.0: Fix x86 installer bug: it gets installed as an x64 component and has the same UpgradeCode as the x64 installer leading to an uninstallation of an x64 installation (and vice versa) #5822
Closed
6 of 8 tasks
@TravisEz13

Travis Plunk (TravisEz13) commented Jan 8, 2018

Copy link
Copy Markdown
Member

Christoph Bergmeister (@bergmeister) Thanks
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.
@bergmeister

Christoph Bergmeister (bergmeister) commented Jan 8, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

No worries, my pleasure. I am glad I found it by coincidence (I guess the bug must have been in there since day one). I think the next time we have to ensure that issues get triaged better because if the impact was understood better from the reported issue then it would not have been so much last minute.
I have done some local testing if this PR got merged into the 6.0.0. branch and it seems fine as well.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TravisEz13
Travis Plunk (TravisEz13) merged commit f8a5fc1 into PowerShell:master Jan 8, 2018
@bergmeister

Christoph Bergmeister (bergmeister) commented Jan 8, 2018

Copy link
Copy Markdown
Contributor Author

I did some analysis and it showed that the bug about the upgrade code being constant and the -arch parameter being hardcoded to x64 was already present in the first commit here that added the WiX file and the MSI build script...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Travis Plunk (TravisEz13) pushed a commit to TravisEz13/PowerShell that referenced this pull request Jan 9, 2018
@bergmeister @TravisEz13
packaging: x86 package gets installed as an x64 component and has the…
adb5631 
… same UpgradeCode as the x64 installer leading to an uninstallation of an x64 installation (and vice versa) (PowerShell#5812)

Fix bug PowerShell#5597: x86/x64 installer are uninstalling each other when installing either of them:

-Make x86 installer to be installed as an x86 component (-arch argument to candle.exe, which sets the `sys.BUILDARCH` variable)
-Make the UpgradeCode unique per platform
-Replace `var.ProductTargetArchitecture` variable with sys.BUILDARCH use to have only 1 variable for the architecture
-Additionally, the architecture was appended to the package name to be able to distinguish the installations.

# Conflicts:
#	assets/Product.wxs
@TravisEz13 Travis Plunk (TravisEz13) mentioned this pull request Jan 9, 2018
Merge 6.0.0-ga PRs #5828
Merged
Travis Plunk (TravisEz13) pushed a commit that referenced this pull request Jan 9, 2018
@bergmeister @TravisEz13
packaging: x86 package gets installed as an x64 component and has the…
087d4ac 
… same UpgradeCode as the x64 installer leading to an uninstallation of an x64 installation (and vice versa) (#5812)

Fix bug #5597: x86/x64 installer are uninstalling each other when installing either of them:

-Make x86 installer to be installed as an x86 component (-arch argument to candle.exe, which sets the `sys.BUILDARCH` variable)
-Make the UpgradeCode unique per platform
-Replace `var.ProductTargetArchitecture` variable with sys.BUILDARCH use to have only 1 variable for the architecture
-Additionally, the architecture was appended to the package name to be able to distinguish the installations.

# Conflicts:
#	assets/Product.wxs
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@bergmeister @TravisEz13
packaging: x86 package gets installed as an x64 component and has the…
2d9e2c3 
… same UpgradeCode as the x64 installer leading to an uninstallation of an x64 installation (and vice versa) (PowerShell#5812)

Fix bug PowerShell#5597: x86/x64 installer are uninstalling each other when installing either of them:

-Make x86 installer to be installed as an x86 component (-arch argument to candle.exe, which sets the `sys.BUILDARCH` variable)
-Make the UpgradeCode unique per platform
-Replace `var.ProductTargetArchitecture` variable with sys.BUILDARCH use to have only 1 variable for the architecture
-Additionally, the architecture was appended to the package name to be able to distinguish the installations.
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@bergmeister @TravisEz13
packaging: x86 package gets installed as an x64 component and has the…
9244562 
… same UpgradeCode as the x64 installer leading to an uninstallation of an x64 installation (and vice versa) (PowerShell#5812)

Fix bug PowerShell#5597: x86/x64 installer are uninstalling each other when installing either of them:

-Make x86 installer to be installed as an x86 component (-arch argument to candle.exe, which sets the `sys.BUILDARCH` variable)
-Make the UpgradeCode unique per platform
-Replace `var.ProductTargetArchitecture` variable with sys.BUILDARCH use to have only 1 variable for the architecture
-Additionally, the architecture was appended to the package name to be able to distinguish the installations.

# Conflicts:
#	assets/Product.wxs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 Travis Plunk (TravisEz13) TravisEz13 approved these changes
@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Awaiting requested review from adityapatwardhan
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw

Uh oh!

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

Assignees

@iSazonov Ilya (iSazonov)

Labels

None yet

Projects

None yet

Milestone

6.0.0-GA

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

4 participants

@bergmeister @iSazonov @SteveL-MSFT @TravisEz13
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 01402520 — 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.