-
Notifications
You must be signed in to change notification settings - Fork 45
Enable 6.2 CI and enable Windows CI #215
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
base: main
Are you sure you want to change the base?
Conversation
506733d
to
51124e4
Compare
51124e4
to
1221dc5
Compare
/// | ||
/// Signals are standardized messages sent to a running program to trigger specific behavior, such as quitting or error handling | ||
/// | ||
/// - Important: Unix signals are only functional on platforms supporting signals. |
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.
Should we instead compile out these unix signals on Windows for now completely, so even the ServiceGroup doesn't have those parameters in its initializers? We can later add support for the Windows-specific termination handling using the right terminology, and add more initializers then.
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.
In my opinion, no. I intentionally didn't compile them out because that makes writing cross platform Swift apps with ServiceLifecycle
really hard to maintain since users now need to #if os(Windows)
. IMO, the best place forward is to define event handlers here a well and add an event handler parameter to the ServiceGroup
but that can be done in a follow up 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.
Once we make this build on Windows, "UnixSignal" becomes API on Windows. I don't love that and would prefer we compile it out and only deliberately add a potential Windows-specific or a cross-platform API (called e.g. "service event") that uses the right platform-specific API under the hood. But landing this is a one-way door we can't remove for the rest of ServiceLifecycle 2.x, and we don't really benefit from it.
I think it's fine for those writing cross-platform services to handle this in their app for now, and we can introduce a cross-platform event API later.
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.
Again I disagree. Forcing users to write conditional code based on platform is a horrible pattern. I think it is totally fine for UnixSignal
to be an API on Windows that just isn't doing anything with it being documented. Packages that have different surface API based on platform is a huge pain that we often encounter ourselves e.g. Foundation
vs FoundationEssentials
.
For a low-level library like swift-system
that might be fine but for a higher level library like ServiceLifecycle
I think we should strive to keep the APIs aligned across platforms.
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.
So what's the plan when event handling is supported on Windows in Service Lifecycle? What will that look like in the ServiceGroup initializer?
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.
We just expand the API. We have two possibilities:
- Adding a new
WindowsConsoleEvent
struct and two new properties to the group configuration i.e.gracefulShutdownConsoleEvents
andcancellationConsoleEvents
. - Trying to come up with a cross platform representation like
TerminationEvent
that covers both unix signals and console events.
I am currently thinking that the former, is the better way forward since I think it is pretty hard to come up with a cross platform representation. swift-subprocess
is doing something similar.
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.
Would WindowsConsoleEvent
be a symbol that's present on macOS and Linux?
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.
Have you considered moving unix signals behind a trait. Trait would be default enabled on all platforms where Swift Service Lifecycle currently builds. Could default to disabled on Windows? Could possibly even annotate them as unavailable?
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.
Would WindowsConsoleEvent be a symbol that's present on macOS and Linux?
Yes. Same argument applies the other way around. Somebody wanting to write a cross platform service that uses ServiceGroup
should not be forced to write #if os()
code.
Trait would be default enabled on all platforms where Swift Service Lifecycle currently builds.
Traits cannot be enabled based on platform. They are orthogonal concepts.
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.
Yes. Same argument applies the other way around. Somebody wanting to write a cross platform service that uses ServiceGroup should not be forced to write #if os() code.
Fair enough, while I don't agree with the tradeoff here, it is a reasonable position and I'll defer to you here.
1221dc5
to
308ec23
Compare
308ec23
to
056c40b
Compare
056c40b
to
13ffa92
Compare
No description provided.