-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Conversation
0fa29a2
to
7c771e0
Compare
d3c6d97
to
f7dbda9
Compare
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 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"?> |
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.
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.)
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.
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.
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.
@stefansjfw
The PR #24221 implements a new GPO category where you can add your installer GPO. ;-)
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.
blocking user install using a GPO
Good point.
What about the GPO. Will this be a second PR?
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.
It's on TODO list, It'll probably go as a follow-up PR
// HKCU key is missing | ||
RegCloseKey(perUserKey); | ||
return InstallScope::PerMachine; |
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.
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?
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.
'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
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.
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:
- Admins expect that machine GPOs have priority over user GPOs. It the normal behavior in Windows, Office, Edge, ...
- 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. 😅
Sure. I'm looking forward to this |
Move per machine check to bootstrapper Move all defines to common.wxs Fix CI
f7dbda9
to
14f36cb
Compare
@snickler Maybe you wanna take a look at/review the scripts here :) |
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.
Need to revert the endpoints to production endpoints before merging.
Will do 😎 |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Jeremy Sinclair <[email protected]>
Co-authored-by: Jeremy Sinclair <[email protected]>
This reverts commit 34545da.
It messes up app ID for per-user installation which ends up breaking winget update of the per-user PT
@stefansjfw with Jaime OOF, should i remove his blocking review? |
Yes, please. |
This comment has been minimized.
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 |
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.
Perhaps we could introduce a for loop here for the Invoke-Expression -Command "$PSScriptRoot\generateFileList.ps1
?
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.
awesome stuff! :)
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.
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.
No worries, it's in progress. Expect PR soon |
Woo hoo! |
* 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]>
Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed