-
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
[GPO; Enterprise] Updater policies #24221
Conversation
@snickler, @jaimecbernardo
Do you have an idea why? |
I would say to try running the powershell to remove the generated directories and build again using msbuild. I will also try building to see if I can repro |
Is it auto generated? I can't find it in the folder structure. |
Yup it's generated. Oh, also nuke all Msbuild.exe and VBCSCompiler.exe instances before you try again |
Hmmm I'm unsure why this is failing to build properly. I want to say it could be something not matching with the changes that it's failing to create the generated file needed. I'll dig in and see what I can find. |
@htcfreek - Oh, I think I know what it is. It's likely because of UpdateUtils directly including the GPOWrapper.h. Since that GPOWrapper.g.h it's referencing is generated during the build of the GPOWrapper project, it's probably not going to be in existence at the time of it being referenced by the updater. There may be some compilation based things that have to do or maybe some ifdefs around including GPOWrapper.g.h? I'm not 100% sure. |
@snickler |
Ha, I think I fixed it. |
Nice. Can you tell me how or push the fix? |
Added relative path to Generated Files folder for GPOWrapper.g.h
I just pushed the change to it that worked locally for me. I added the Generated Files path to the include and that seemed to be fine |
Looks like the arm64 build completed successfully. I'm sure there's probably a BETTER way of doing this. I wonder if it would be better to add the Generated Files folder for GPOWrapper into the additional include directories for the runner project rather than hardcoding the directory name in GPOWrapper.h? @jaimecbernardo thoughts? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I have check the issue and PR history. It seems that we decide to not show the setting to normal users, because they should not enable it. We don't want this because the can't install the updates. (See also #2885.) |
@jaimecbernardo , @stefansjfw |
get => !AutoUpdatesDisabledOnDevBuild && !_autoDownloadUpdatesIsGpoDisabled; | ||
} | ||
|
||
// The settings card is hidden for users who are not a member of the Administrators group and in this case the GPO info should be hidden too. |
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.
note that this will be changed once per-user installer is out
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.
Okay. But then we need to update it after user install is out. I only improved code.
Currently it is hidden so gpo message should be too.
And btw, I think this will be related to the install mode. If PT is installed as admin a user still has no permissions.
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.
yeah, yeah, it was note to myself mostly :)
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.
lgtm
@stefansjfw |
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.
Settings card is not disabled when version is != 0.0.1. Also info bar not displayed. @htcfreek can you double-check that?
Edit: Bug found. |
Tested fully installed PT |
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.
Looks good now :)
Then you can merge it. I don't have the permissions. |
@@ -272,11 +274,20 @@ public bool IsAdmin | |||
} | |||
} | |||
|
|||
// Are we running a dev build? (Please note that we verify this in the code that gets the newest version from Github too.) |
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.
Thanks a lot for the contribution @htcfreek ! 🎉 |
* Implement GPO * Add GPOs in updater * Rename policy * fix * fix * Update GPOWrapper.h Added relative path to Generated Files folder for GPOWrapper.g.h * fix and inactivate PeriodicUpdateCheck gpo * Docs * GPO name change * Templates * Templates: Text changes * Templates: Text changes * Templates: Text changes * docs: spell fix * settings ui * fixes * fixes * fix gpo description * EOF fix * Fix include in UpdateUtils.cpp and remove build workaround * UI improvements * spell fixes * code improvements * Update README.md * Update PowerToys.adml * Update src/gpo/assets/PowerToys.admx * Remove forbidden pattern
Summary of the Pull Request
This PR adds the following three updater policies:
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
TESTED:
NOT TESTED (Not possible for me.):