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.

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

Release ocluster-worker and associated library as a standalone package #217

Closed
wants to merge 1 commit into from

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Feb 21, 2023

This PR attempts at replacing #151.
I'm introducing a new package, ocluster-worker, which contains the public ocluster-worker binary and public ocluster-worker library (the previous PR only exposed the library in this package).

However there is an issue with the obuilder git submodule that causes linker errors when trying to use the resulting ocluster-worker library: It gets compiled with the pinned obuilder, but it still requires a dependency on the publicly released obuilder (causing a conflict, see ocaml/dune#3911). That's why the second commit removes the submodule and replaces it with an opam pin. Is there an alternative solution to this issue that you would prefer?

The ocluster-worker package will now depend on the correct released version of the obuilder package: in this mode, obuilder is not vendored as a submodule. If projects using the ocluster-worker library just depend on the ocluster-worker package and link with the library, I think this solves your problem.

The main issue we've been having with vendoring projects as submodules is when the same project gets vendored multiple times. Unless I'm wrong, that's not what's happening with your use case.

I'm not entirely convinced we need a new package either: we could just expose the ocluster-worker library as part of the ocluster package. It also seemed more logical to add the ocluster-worker binary to the ocluster-worker package.

I've also made the ocluster package depend on ocluster-worker as a kind of meta-package.

cc @ElectreAAS @art-w

EDIT: we've confirmed offline the current-bench project just needs the ocluster-worker library exposed. Do we want to expose it as part of the ocluster package, or in a new package?

@MisterDA MisterDA force-pushed the ocluster-worker-standalone branch from 23e8d0b to b334134 Compare February 21, 2023 15:38
Copy link
Member

@tmcgilchrist tmcgilchrist left a comment

Choose a reason for hiding this comment

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

I think the library for ocluster-worker is the interesting part and would be in favour of releasing the tools to build a worker. I can’t see ocluster-worker executable being useful in general.

Comment on lines +20 to +21
"capnp-rpc-lwt"
"cohttp-lwt-unix"
Copy link
Member

Choose a reason for hiding this comment

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

These need lower bounds I think.

@MisterDA
Copy link
Contributor Author

Thanks!

@MisterDA MisterDA closed this Feb 22, 2023
@MisterDA MisterDA deleted the ocluster-worker-standalone branch February 22, 2023 10:48
MisterDA added a commit to MisterDA/opam-repository that referenced this pull request Mar 2, 2023
…uster (0.2.1)

CHANGES:

- Expose the ocluster-worker library in the ocluster-worker package
  (@MisterDA @art-w, ocurrent/ocluster#219 ocurrent/ocluster#217 ocurrent/ocluster#151, reviewed by @tmcgilchrist)
- Remove corrupted repositories from the cache (@kit-ty-kate ocurrent/ocluster#216, reviewed by @talex5)
- Allow workers to report additional prometheus metrics (@patricoferris ocurrent/ocluster#210, reviewed by @tmcgilchrist, @MisterDA)
- Smother Cap'n Proto and TLS debug logs (@MisterDA ocurrent/ocluster#213, reviewed by @talex5)
- Added command line option to set obuilder health check period (@mtelvers ocurrent/ocluster#214, reviewed by @tmcgilchrist)
- Conditionally compile macos user_temp fetcher (@tmcgilchrist ocurrent/ocluster#209, reviewed by @MisterDA, @mtelvers)
- Make rsync-mode mandatory when using rsync store (@tmcgilchrist ocurrent/ocluster#202, reviewed by @MisterDA)
- Windows service bugfixes (@MisterDA ocurrent/ocluster#200, reviewed by @tmcgilchrist)
- Fix build and opam metadata (@MisterDA @tmcgilchrist ocurrent/ocluster#199 ocurrent/ocluster#203)
MisterDA added a commit to MisterDA/opam-repository that referenced this pull request Mar 3, 2023
…uster (0.2.1)

CHANGES:

- Expose the ocluster-worker library in the ocluster-worker package
  (@MisterDA @art-w, ocurrent/ocluster#219 ocurrent/ocluster#217 ocurrent/ocluster#151, reviewed by @tmcgilchrist)
- Remove corrupted repositories from the cache (@kit-ty-kate ocurrent/ocluster#216, reviewed by @talex5)
- Allow workers to report additional prometheus metrics (@patricoferris ocurrent/ocluster#210, reviewed by @tmcgilchrist, @MisterDA)
- Smother Cap'n Proto and TLS debug logs (@MisterDA ocurrent/ocluster#213, reviewed by @talex5)
- Added command line option to set obuilder health check period (@mtelvers ocurrent/ocluster#214, reviewed by @tmcgilchrist)
- Conditionally compile macos user_temp fetcher (@tmcgilchrist ocurrent/ocluster#209, reviewed by @MisterDA, @mtelvers)
- Make rsync-mode mandatory when using rsync store (@tmcgilchrist ocurrent/ocluster#202, reviewed by @MisterDA)
- Windows service bugfixes (@MisterDA ocurrent/ocluster#200, reviewed by @tmcgilchrist)
- Fix build and opam metadata (@MisterDA @tmcgilchrist ocurrent/ocluster#199 ocurrent/ocluster#203)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants