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.

Sign up for GitHub

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

Jump to bottom

Windows support #128

Merged
merged 1 commit into from
Jul 21, 2021
Merged

Windows support #128

merged 1 commit into from
Jul 21, 2021

Conversation

MisterDA
Copy link
Contributor

This PR adds support for Windows.
The service interface uses subcommands (my current design, which I find simpler), but @dra27 prefers flags, but it's the end of the week and I've been fighting against cmdliner for too long, so here it is.
The Lwt_io bit is going to be fixed upstream.
Vendoring winvsc is required for bug fixes (a PR is opened).
ExtUnix dev branch has required fixes, so this requires extunix > 0.3.1 (unreleased at the time of writing).

@MisterDA MisterDA mentioned this pull request May 4, 2021
Closed
@MisterDA MisterDA force-pushed the windows branch 3 times, most recently from d62f32b to 2461a0a Compare May 11, 2021 14:40
@MisterDA MisterDA force-pushed the windows branch 5 times, most recently from 5bbde02 to 086c1a3 Compare May 19, 2021 11:18
@dra27
Copy link
Contributor

dra27 commented May 25, 2021

I think it's worth refactoring the changes in bin/ to minimise the different files between Unix and Windows - definitely worth having the same CLI for both, just with error messages if you try to register the service on Unix (it could in theory do something systemctl-y in future....!). Otherwise this is looking marvellous too 🙂

@MisterDA MisterDA force-pushed the windows branch 6 times, most recently from 9f9921a to 9110ebe Compare May 28, 2021 08:15
talex5
talex5 reviewed Jun 2, 2021
View reviewed changes
Copy link
Contributor

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Looks OK, but I'm not very keen on the --install/--remove thing. Can we skip that code on Linux, so the options don't even show up in the man-page, and the required arguments really are required? It's a shame to lose cmdliner's error reporting everywhere just to support one platform.

One way might be to check the first argument manually. If it's install or remove then do the Windows thing, otherwise parse the arguments with cmdliner.

Possibly we should change connect_addr to be a named option (as with ocluster-client) so it can't be confused with a command.

bin/scheduler.ml Outdated Show resolved Hide resolved
worker/process.ml Outdated Show resolved Hide resolved
@MisterDA
Copy link
Contributor Author

MisterDA commented Jun 3, 2021

Looks OK, but I'm not very keen on the --install/--remove thing. Can we skip that code on Linux, so the options don't even show up in the man-page, and the required arguments really are required? It's a shame to lose cmdliner's error reporting everywhere just to support one platform.

Agreed.

One way might be to check the first argument manually. If it's install or remove then do the Windows thing, otherwise parse the arguments with cmdliner.

I think this is basically using sub-commands on Windows...
I'll try to come up with something.

Possibly we should change connect_addr to be a named option (as with ocluster-client) so it can't be confused with a command.

I was bitten by this at least once, added this change.

@MisterDA MisterDA mentioned this pull request Jun 3, 2021
Merged
15 tasks
@MisterDA MisterDA force-pushed the windows branch 5 times, most recently from 671b8d8 to ae58df6 Compare June 18, 2021 15:40
@talex5 talex5 mentioned this pull request Jun 21, 2021
Merged
@MisterDA
Copy link
Contributor Author

Ok, I'll rebase this branch when Windows prerequisites are merged.

@MisterDA
Copy link
Contributor Author

Rebased.

talex5
talex5 reviewed Jun 24, 2021
View reviewed changes
Copy link
Contributor

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Looking good - needs rebasing though!

bin/worker.ml Outdated Show resolved Hide resolved
bin/winsvc_wrapper..ml Outdated Show resolved Hide resolved
bin/winsvc_wrapper..ml Outdated Show resolved Hide resolved
bin/winsvc_wrapper.winsvc.ml Outdated Show resolved Hide resolved
@MisterDA MisterDA force-pushed the windows branch 4 times, most recently from 1e3419e to be5d67a Compare June 30, 2021 14:11
This was referenced Jul 14, 2021
Merged
Merged
@talex5
Copy link
Contributor

talex5 commented Jul 15, 2021

I've merged bits of this in other PRs now. I think we're just waiting for savonet/ocaml-winsvc#1 to be merged before this can go in.

@MisterDA
Run the scheduler and worker as Windows services
7dbb219 
Wrap ocaml-winsvc for portability.
Add `install` and `remove` sub-commands to install and remove the
program as a Windows Service.
When running as a Windows Service, the program logs to the state
dir. Logs are retained between runs.
@talex5 talex5 merged commit 5bf8d46 into ocurrent:master Jul 21, 2021
@talex5
Copy link
Contributor

talex5 commented Jul 21, 2021

winsvc has been released now, so I've merged this - thanks!

@MisterDA MisterDA deleted the windows branch September 2, 2021 13:18
tmcgilchrist added a commit to tmcgilchrist/opam-repository that referenced this pull request Nov 9, 2022
@tmcgilchrist
[new release] ocluster, ocluster-api and current_ocluster (0.2)
b8873c9 
CHANGES:

- Update OBuilder to pull in Windows prereqs (@MisterDA ocurrent/ocluster#196, reviewed by @tmcgilchrist)
- Worker pool capacity metric (@mtelvers ocurrent/ocluster#195, reviewed by @talex5)
- Update Ocluster to be macOS capable (@patricoferris ocurrent/ocluster#152, reviewed by @tmcgilchrist)
- Use rsync-hardlink from OBuilder rsync store (@MisterDA ocurrent/ocluster#189, reviewed by @tmcgilchrist)
- Various Dune 3 fixes (@MisterDA ocurrent/ocluster#187, reviewed by @talex5)
- Switch from Debian to Ubuntu for Worker Builds (@mtelvers ocurrent/ocluster#185, reviewed by @dra27)
- Update to Prometheus 1.2 (@mtelvers ocurrent/ocluster#183, reviewed by @MisterDA)
- Default to GitLab merge-requests refspecs (@MisterDA ocurrent/ocluster#180, reviewed by @tmcgilchrist)
- GitLab: allow fetching merge-requests from remote origin (@MisterDA ocurrent/ocluster#175, reviewed by @tmcgilchrist)
- Updated Dockerfile* and .dockerignore (@mtelvers ocurrent/ocluster#178, reviewed by @tmcgilchrist and @MisterDA)
- Upgrade lwt to 5.5.0 (@maiste ocurrent/ocluster#171 ocurrent/ocluster#181, reviewed by @dra27)
- Added --terse option to ocluster-admin-show and added new command ocluster-admin-exec (@mtelvers ocurrent/ocluster#165, reviewed by @tmcgilchrist and @dra27)
- Support custom jobs, adds custom job specifications to the cluster API. (@patricoferris ocurrent/ocluster#156, reviewed by @talex5)
- Continuing the joy of submodule URL changes (@dra27 ocurrent/ocluster#164)
- Update ocaml/opam-repository SHA includes tar-unix.2.0.1 (@mtelvers ocurrent/ocluster#161, reviewed by @dra27)
- Run git submodule update when resetting the git clone (@MisterDA ocurrent/ocluster#163, reviewed by @tmcgilchrist)
- Cmdliner.1.1.0 support (@MisterDA ocurrent/ocluster#160)
- Explicitly set confirmation levels to allow for manually triggered jobs. (@tmcgilchrist ocurrent/ocluster#159, reviewed by @TheLortex)
- Merge Obuilder with rsync store (@MisterDA ocurrent/ocluster#155, reviewed by @talex5)
- Sqlite3 remove usage of "FALSE" for compatibility with older versions (@art-w ocurrent/ocluster#150, reviewed by @talex5)
- Revert adopting GNU tar format (@dra27 ocurrent/ocluster#148)
- Update obuilder to latest (@mtelvers ocurrent/ocluster#147)
- Fix deprecations in Fmt 0.8.10 (@tmcgilchrist ocurrent/ocluster#145)
- Lwt_unix.yield was deprecated in favor of Lwt.pause (@MisterDA ocurrent/ocluster#142, reviewed by @dra27)
- Remove runc build from Dockerfile.worker (@talex5 ocurrent/ocluster#140)
- Add Windows support (@MisterDA ocurrent/ocluster#128, reviewed by @dra27 and @talex5)
- Admin client: fix ref-counting on progress display (@talex5 ocurrent/ocluster#138)
- Add --verbose to README examples (@talex5 ocurrent/ocluster#137)
- Use --connect for the worker capability too (@talex5 ocurrent/ocluster#136)
- Make free-space check work on Windows (@talex5 ocurrent/ocluster#134)
- Support Fmt.cli and Logs.cli (@MisterDA ocurrent/ocluster#133, reviewed by @talex5)
- Windows support prerequisites (@talex5 and @MisterDA ocurrent/ocluster#132)
- Update to capnp-rpc 1.2 and fix connection handling (@talex5 ocurrent/ocluster#131)
- Improve reporting of connection errors (@talex5 ocurrent/ocluster#130)
- Depend on more current_* modules to build examples (@MisterDA ocurrent/ocluster#129, reviewed by @talex5)
- Add ca-certificates to Dockerfile (@talex5 ocurrent/ocluster#127)
- API to wait for a worker to drain, useful for scripts that need to wait for a worker to stop before continuing. (@talex5 ocurrent/ocluster#126)
- Allow pausing/unpausing/forgetting unconnected workers (@talex5 ocurrent/ocluster#125)
- Fix ref-leak when rejecting duplicate workers (@talex5 ocurrent/ocluster#124)
- Improve handling of a worker's active state (@talex5 ocurrent/ocluster#123)
- Report better error on duplicate worker registration (@talex5 ocurrent/ocluster#122)
- Switch pool tests to expect tests (@talex5 ocurrent/ocluster#121)
- Add timestamps to OBuilder logs (@talex5 ocurrent/ocluster#120)
- Fix opam constraint on digestif (@kit-ty-kate ocurrent/ocluster#118, reviewed by @talex5)
- Add support for secrets. Secrets are a way to transmit sensitive key-value pairs to workers, without having them displayed in any log files. (@TheLortex ocurrent/ocluster#116, reviewed by @talex5)
- Add optional label to build_obuilder (@TheLortex ocurrent/ocluster#113, reviewed by @talex5)
@tmcgilchrist tmcgilchrist mentioned this pull request Nov 9, 2022
Closed
@tmcgilchrist tmcgilchrist mentioned this pull request Nov 30, 2022
Merged
dinosaure pushed a commit to tmcgilchrist/opam-repository that referenced this pull request Dec 8, 2022
@tmcgilchrist @dinosaure
[new release] ocluster, ocluster-api and current_ocluster (0.2)
d27404f 
CHANGES:

- Update OBuilder to pull in Windows prereqs (@MisterDA ocurrent/ocluster#196, reviewed by @tmcgilchrist)
- Worker pool capacity metric (@mtelvers ocurrent/ocluster#195, reviewed by @talex5)
- Update Ocluster to be macOS capable (@patricoferris ocurrent/ocluster#152, reviewed by @tmcgilchrist)
- Use rsync-hardlink from OBuilder rsync store (@MisterDA ocurrent/ocluster#189, reviewed by @tmcgilchrist)
- Various Dune 3 fixes (@MisterDA ocurrent/ocluster#187, reviewed by @talex5)
- Switch from Debian to Ubuntu for Worker Builds (@mtelvers ocurrent/ocluster#185, reviewed by @dra27)
- Update to Prometheus 1.2 (@mtelvers ocurrent/ocluster#183, reviewed by @MisterDA)
- Default to GitLab merge-requests refspecs (@MisterDA ocurrent/ocluster#180, reviewed by @tmcgilchrist)
- GitLab: allow fetching merge-requests from remote origin (@MisterDA ocurrent/ocluster#175, reviewed by @tmcgilchrist)
- Updated Dockerfile* and .dockerignore (@mtelvers ocurrent/ocluster#178, reviewed by @tmcgilchrist and @MisterDA)
- Upgrade lwt to 5.5.0 (@maiste ocurrent/ocluster#171 ocurrent/ocluster#181, reviewed by @dra27)
- Added --terse option to ocluster-admin-show and added new command ocluster-admin-exec (@mtelvers ocurrent/ocluster#165, reviewed by @tmcgilchrist and @dra27)
- Support custom jobs, adds custom job specifications to the cluster API. (@patricoferris ocurrent/ocluster#156, reviewed by @talex5)
- Continuing the joy of submodule URL changes (@dra27 ocurrent/ocluster#164)
- Update ocaml/opam-repository SHA includes tar-unix.2.0.1 (@mtelvers ocurrent/ocluster#161, reviewed by @dra27)
- Run git submodule update when resetting the git clone (@MisterDA ocurrent/ocluster#163, reviewed by @tmcgilchrist)
- Cmdliner.1.1.0 support (@MisterDA ocurrent/ocluster#160)
- Explicitly set confirmation levels to allow for manually triggered jobs. (@tmcgilchrist ocurrent/ocluster#159, reviewed by @TheLortex)
- Merge Obuilder with rsync store (@MisterDA ocurrent/ocluster#155, reviewed by @talex5)
- Sqlite3 remove usage of "FALSE" for compatibility with older versions (@art-w ocurrent/ocluster#150, reviewed by @talex5)
- Revert adopting GNU tar format (@dra27 ocurrent/ocluster#148)
- Update obuilder to latest (@mtelvers ocurrent/ocluster#147)
- Fix deprecations in Fmt 0.8.10 (@tmcgilchrist ocurrent/ocluster#145)
- Lwt_unix.yield was deprecated in favor of Lwt.pause (@MisterDA ocurrent/ocluster#142, reviewed by @dra27)
- Remove runc build from Dockerfile.worker (@talex5 ocurrent/ocluster#140)
- Add Windows support (@MisterDA ocurrent/ocluster#128, reviewed by @dra27 and @talex5)
- Admin client: fix ref-counting on progress display (@talex5 ocurrent/ocluster#138)
- Add --verbose to README examples (@talex5 ocurrent/ocluster#137)
- Use --connect for the worker capability too (@talex5 ocurrent/ocluster#136)
- Make free-space check work on Windows (@talex5 ocurrent/ocluster#134)
- Support Fmt.cli and Logs.cli (@MisterDA ocurrent/ocluster#133, reviewed by @talex5)
- Windows support prerequisites (@talex5 and @MisterDA ocurrent/ocluster#132)
- Update to capnp-rpc 1.2 and fix connection handling (@talex5 ocurrent/ocluster#131)
- Improve reporting of connection errors (@talex5 ocurrent/ocluster#130)
- Depend on more current_* modules to build examples (@MisterDA ocurrent/ocluster#129, reviewed by @talex5)
- Add ca-certificates to Dockerfile (@talex5 ocurrent/ocluster#127)
- API to wait for a worker to drain, useful for scripts that need to wait for a worker to stop before continuing. (@talex5 ocurrent/ocluster#126)
- Allow pausing/unpausing/forgetting unconnected workers (@talex5 ocurrent/ocluster#125)
- Fix ref-leak when rejecting duplicate workers (@talex5 ocurrent/ocluster#124)
- Improve handling of a worker's active state (@talex5 ocurrent/ocluster#123)
- Report better error on duplicate worker registration (@talex5 ocurrent/ocluster#122)
- Switch pool tests to expect tests (@talex5 ocurrent/ocluster#121)
- Add timestamps to OBuilder logs (@talex5 ocurrent/ocluster#120)
- Fix opam constraint on digestif (@kit-ty-kate ocurrent/ocluster#118, reviewed by @talex5)
- Add support for secrets. Secrets are a way to transmit sensitive key-value pairs to workers, without having them displayed in any log files. (@TheLortex ocurrent/ocluster#116, reviewed by @talex5)
- Add optional label to build_obuilder (@TheLortex ocurrent/ocluster#113, reviewed by @talex5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dra27 dra27 dra27 left review comments

@talex5 talex5 talex5 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MisterDA @dra27 @talex5