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

Public Ocluster_worker library #151

Closed
wants to merge 2 commits into from
Closed

Conversation

art-w
Copy link
Contributor

@art-w art-w commented Nov 29, 2021

For current-bench and sandmark, we need to experiment with a different kind of worker that build images, and then run them with custom settings to ensure that the benchmarks are stable. This PR exposes the internal library Ocluster_worker to help us extend the existing build behaviour.

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?

@art-w
Copy link
Contributor Author

art-w commented Oct 4, 2022

I've rebased the PR as current-bench still depends on this to implement its custom benchmarking workers: they are roughly a simplified version of the standard worker, but we need to enable some runtime settings to stabilize the benchmarks (which isolated cpu cores to use, a tmpfs, etc)

I respent a few hours trying not to remove the obuilder git submodule, because I understand it is very convenient... but nothing worked within dune. Some alternatives that would work though:

  • Functorizing the worker with obuilder signature, such that we can choose a non-vendored version later, but it's clearly super invasive.
  • Inserting a remove in the *.opam files to get rid of the vendored version when people install it:
build: [
  ["dune" "subst"] {dev}
  ["rm" "-Rf" "obuilder"]       <-----
  ["dune" "build" "-p" ...]
]

(I don't know how to keep the dune-project generation of the opam files with the latest)
Let me know if you have some other ideas or preferences :)

@art-w art-w force-pushed the public-worker branch 3 times, most recently from 65a9555 to caee589 Compare October 24, 2022 10:53
@MisterDA
Copy link
Contributor

Circling back on this after a long while, as I'm going to cut a release of ocluster soon.
Do you really need to track the development branch of ocluster, either by pinning or sub-moduling? If not, what if I simply expose the library as part of ocluster, or even through a new package if needed, and you simply depend on ocluster.0.2.1 and link with the library? When installed through opam, ocluster will depend on the opam package, and the submodule won't be pulled.
cc @ElectreAAS

@MisterDA MisterDA closed this Feb 21, 2023
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