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

Vendorable #436

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Vendorable #436

wants to merge 13 commits into from

Conversation

lubegasimon
Copy link

The goal of this PR is to modify dune-release in favour of ocaml-platform to not to rely on external binaries, rather use libraries.

  • It adds dune_release_bin library and as a result, the entry point (main.ml) to dune-release is moved to bin/bin (which is a new directory).
  • It also adds opam_lint_with_cmd which is an exact implementation of the current the current opam_lint.
  • It implements a global ref, opam_lint_impl whose contents get changed (here as of now).
  • The current opam_lintfunction is changed to reference the contents of opam_lint_impl thereby leaving the current behaviour of dune-release unaffected.

@NathanReb
Copy link
Contributor

Thanks for your contribution!

Getting rid of external binary dependencies is indeed something that we would like to do in dune-release and have discussed in recent dev meetings. Some parts are easier to deal with, like opam, some others slightly more involved like curl but I'm confident we'll get there.

dune-release already depends on opam libraries so I think it would make much more sense to replace the external opam lint invocation by calls to the opam libraries to run the lint in-process. This would be a much more satisfying solution than introducing global refs. I'm happy to help you with this and provide guidance if needed!

Cutting out a library to expose CLI functionality is alright by me and is somehting we already do in a bunch of tools like opam-monorepo. I would much prefer if we were to stick to the conventions we use there which is:

  • have a cli/ folder with a dune-release.cli library
  • have a bin/ folder with the main executable called dune_release.ml

It would also probably make sense to rename the library dune-release.lib but we can do that separately and as part of the 2.0.0 release.

It also seems from the diff in the tests that your branch needs rebasing.

Finally I wish to emphasize something you are probably already aware of but making those library public doesn't mean we ensure their stability and we'd advise that ocaml-platform locks the dune-release dependency to prevent random breakage on new release of the tool!

@lubegasimon
Copy link
Author

Thank you for the insights, and I am happy to improve the pull request!

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.

None yet

2 participants