Skip to content
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.

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

[installer] Support per-user installation #24087

Merged
merged 21 commits into from
Mar 31, 2023
Merged

Conversation

stefansjfw
Copy link
Collaborator

@stefansjfw stefansjfw commented Feb 13, 2023

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

  • build both installers
  • try installing both versions of PT
    • ensure that per-machine version is installed in Program files by default and registry entries are in HKLM
    • ensure that per-user version is installed in user's app data folder by default and registry entries are in HKCU
    • try installing per-user version over per-machine version
      • ensure that error message that per-machine version is already installed is shown
    • try installing per-machine version over per-user version
      • ensure it works

@stefansjfw stefansjfw marked this pull request as draft February 13, 2023 15:19
@stefansjfw stefansjfw changed the title [installer] Support per-user installation [draft][installer] Support per-user installation Feb 13, 2023
@stefansjfw stefansjfw marked this pull request as ready for review February 13, 2023 15:35
@microsoft microsoft deleted a comment from azure-pipelines bot Feb 13, 2023
@stefansjfw stefansjfw marked this pull request as draft February 13, 2023 15:37
@microsoft microsoft deleted a comment from azure-pipelines bot Feb 13, 2023
@stefansjfw stefansjfw force-pushed the stefan/per_user_instal branch 2 times, most recently from d3c6d97 to f7dbda9 Compare February 13, 2023 17:19
Copy link
Collaborator

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

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

I know this is WIP. But I want to write my thoughts in an early state because they could be code changing.

<?endif?>

<?if $(var.PerUser) = "true"?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is setting this? Installer parameter? For enterprise usage I think two things are required: Allow forcing machine install and blocking user install using a GPO. (Cc: @jaimecbernardo for the GPO.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Allow forcing machine install

This is a build argument. There will be 2 separate installers. Like for VSCode.

blocking user install using a GPO

Good point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@stefansjfw
The PR #24221 implements a new GPO category where you can add your installer GPO. ;-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

blocking user install using a GPO

Good point.

What about the GPO. Will this be a second PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's on TODO list, It'll probably go as a follow-up PR

Comment on lines +79 to +81
// HKCU key is missing
RegCloseKey(perUserKey);
return InstallScope::PerMachine;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What makes more sense?
'No HKCU'=Machine or 'HKLM exist'=Machine. What happens if both are installed because a user tries to go around company restrictions for update cycles?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

'HKLM exist'=Machine

This is not possible as we need to be backward compatible with the versions that doesn't have this reg entry.

What makes more sense?
'No HKCU'=Machine or 'HKLM exist'=Machine. What happens if both are installed because a user tries to go around company restrictions for update cycles?

This is yet to be decided, i.e. what to do in this scenario

Copy link
Collaborator

@htcfreek htcfreek Feb 20, 2023

Choose a reason for hiding this comment

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

What makes more sense?
'No HKCU'=Machine or 'HKLM exist'=Machine. What happens if both are installed because a user tries to go around company restrictions for update cycles?

This is yet to be decided, i.e. what to do in this scenario

Fyi:

  1. Admins expect that machine GPOs have priority over user GPOs. It the normal behavior in Windows, Office, Edge, ...
  2. If a user has no admin permission he can't manipulate user and machine policies. Neither using gpedit.msc, nor changing the Registry values.

And maybe we should simply block installing both at the same time. This makes no sense and I am sure it will make trouble. 😅

@stefansjfw
Copy link
Collaborator Author

I know this is WIP. But I want to write my thoughts in an early state because they could be code changing.

Sure. I'm looking forward to this

Move per machine check to bootstrapper
Move all defines to common.wxs
Fix CI
@stefansjfw stefansjfw changed the title [draft][installer] Support per-user installation [installer] Support per-user installation Feb 22, 2023
@stefansjfw stefansjfw marked this pull request as ready for review February 22, 2023 10:07
@stefansjfw
Copy link
Collaborator Author

@snickler Maybe you wanna take a look at/review the scripts here :)

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

Need to revert the endpoints to production endpoints before merging.

src/common/updating/updating.cpp Outdated Show resolved Hide resolved
@snickler
Copy link
Collaborator

@snickler Maybe you wanna take a look at/review the scripts here :)

Will do 😎

@github-actions

This comment has been minimized.

It messes up app ID for per-user installation which ends up breaking winget update
of the per-user PT
@crutkas
Copy link
Member

crutkas commented Mar 20, 2023

@stefansjfw with Jaime OOF, should i remove his blocking review?

@stefansjfw
Copy link
Collaborator Author

@stefansjfw with Jaime OOF, should i remove his blocking review?

Yes, please.

@github-actions

This comment has been minimized.

Invoke-Expression -Command "$PSScriptRoot\generateFileComponents.ps1 -fileListName ""MonacoCustomLanguagesFiles"" -wxsFilePath $PSScriptRoot\FileExplorerPreview.wxs -regroot $registryroot"

#FileLocksmith
#TODO: There are multiple .deps.json files, make sure it works as expected
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we could introduce a for loop here for the Invoke-Expression -Command "$PSScriptRoot\generateFileList.ps1?

Copy link
Collaborator

@yuyoyuppe yuyoyuppe left a comment

Choose a reason for hiding this comment

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

awesome stuff! :)

Copy link
Collaborator

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

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

Please add a "block per user install" before releasing this change. Otherwise IT departments may get problems with already deployed (SCCM, ...) per machine install, I think.

The gpo should block exe and extracted msi from installing as per user. Having it only as machine policy is enough here.

@stefansjfw
Copy link
Collaborator Author

Please add a "block per user install" before releasing this change.

No worries, it's in progress. Expect PR soon

@stefansjfw stefansjfw merged commit 870f8e3 into main Mar 31, 2023
@snickler
Copy link
Collaborator

Woo hoo!

@crutkas crutkas deleted the stefan/per_user_instal branch May 1, 2023 23:51
BLM16 pushed a commit to BLM16/PowerToys that referenced this pull request Jun 22, 2023
* Add per user installer

* Separate upgrade codes for per machine and per user installation
Move per machine check to bootstrapper
Move all defines to common.wxs
Fix CI

* Update installer/PowerToysSetup/generateFileList.ps1

Co-authored-by: Jeremy Sinclair <[email protected]>

* Update installer/PowerToysSetup/generateAllFileComponents.ps1

Co-authored-by: Jeremy Sinclair <[email protected]>

* Update installer/PowerToysSetup/generateFileList.ps1

Co-authored-by: Jeremy Sinclair <[email protected]>

* expect.txt

* Revert "Update installer/PowerToysSetup/generateFileList.ps1"

This reverts commit 34545da.

* Update release CI to build both installers

* Revert bundle name change

It messes up app ID for per-user installation which ends up breaking winget update
of the per-user PT

* spellcheck

* Fix bad merge

* Add RegistryPreview

* Include backup_restore_settings.json

* Revert testing endpoint change

---------

Co-authored-by: Jeremy Sinclair <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants