-
Notifications
You must be signed in to change notification settings - Fork 133
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
Add windows service functionality #344 #567
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #567 +/- ##
=======================================
Coverage 78.26% 78.27%
=======================================
Files 75 75
Lines 5835 5896 +61
=======================================
+ Hits 4567 4615 +48
- Misses 1268 1281 +13 ☔ View full report in Codecov by Sentry. |
… always gets seen
Ok, I think it's good to go for review now. The service name and service description are up for debate. I can change them to whatever you want. Currently they are:
|
I've also fixed Since I can nuke the service out of this PR if you want. The only benefit of the service over |
Keep it in, I think that's the main reason why many people wanted to have the service in the first place :) Code looks clean on well-documented on the first glance! Much appreciated. The Windows portion of Pueue needed a bit of love since quite some time. |
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.
First up, thanks again :)
Overall clean code structure, well documented and readable code.
The system setup was certainly more involved than I expected, but I guess it also gives the service itself a lot more control over itself.
A few remarks, mostly nitpicks and call for more documentation
Disclaimer:
I really didn't read up on the internals on how Windows services and their privileges work. I only read through the code, which I obviously didn't always fully understand because of that. So I'll just trust you and hope that you'll be available if anything breaks in this part 😁
To keep it simple, some api calls require the calling process has certain privileges (which the windows docs state which one). As this is running as SYSTEM, the privileges for this are probably already granted by default. This is just me being explicit for safety's sake (but even if it's not granted by default, this grants it, so we're all good). I don't anticipate there's any way the privileges can even be broken in real world usage. The call is pretty much guaranteed to succeed. But yeah, if there's any issue reports, feel free to ping me and I'm glad to lend a hand. :) One example: pueue/pueue/src/daemon/service.rs Line 385 in 9d30ba5
https://learn.microsoft.com/en-us/windows/win32/api/wtsapi32/nf-wtsapi32-wtsqueryusertoken |
- Turns out `arguments` does not need extra buffer space - We can use the Free trait for HANDLE drop - And made the dirty code checking more concise (added some comment to make it clear though)
- Added more docs. - Reordered some code. - Remove noop stop() just to ensure no race condition can occur.
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.
Boi, that's what I call thorough documentation :D
Thanks a lot, whoever will take a look at this in the future will have all the resources they need to understand what's going on :) Much appreciated!
I've no further remarks, lets merge this!
This is an initial implementation of a Windows service for pueued. #344
This is a draft and I would like to have some testing/feedback on it before it gets merged.How to use:
The following subcommands now exist:
pueued service install
pueued service uninstall
pueued service start
pueued service stop
pueued service run
(internal only)In an admin terminal:
pueued service install
pueued service start
. It's set to autostart, so it will start up with the system, though that can obviously be adjusted to manual if the user wants. After the initial install, the service is not started automatically, so you must start it yourself the first time.pueued service uninstall
This should also respect it when users log out and other users log in.
The service process itself runs as SYSTEM. It spawns a new
pueued
daemon process as the currently logged in user.What I want tested:
Todo / possible bug:
-d
to actually detach in windows using process creation flags-d
launching visible shell windowsPossible additional features:
-d
mode check if windows service exists, if so, start and run the service instead of the daemon, otherwise, fallback to forking daemon mode.Checklist
Please make sure the PR adheres to this project's standards:
CHANGELOG.md
.cargo clippy
andcargo fmt
. The CI will fail otherwise anyway.-d
mode, but using the service will properly fix it)