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

Provide opam during build #4387

Closed

Conversation

lefessan
Copy link
Contributor

With the latest version of opam, opam-bin fails because an internal call (to opam show to get the opam file corresponding to the built package) fails because of the global lock (it happens only when creating a switch).

This PR makes opam create an opam description file in the build directory called $OPAMROOT/$SWITCH/.opam-switch/build/$PACKAGE.opam that can be used by hooks, and opam-bin in particular, avoiding the recursive call to opam.

Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

Thanks!

A few notes:

  • please avoid hardcoding specific paths at several places, it's not maintainable; we use the OpamPath module to centralise such definitions. Especially here since it's expected as an interface with plugins.
  • maybe a cleaner interface would be to provide the opam file location as a variable to the hook.
  • with a variable, since we should already have the opam file stored somewhere, maybe we could even provide its location directly and avoid the extra file write?

src/client/opamSolution.ml Outdated Show resolved Hide resolved
@lefessan lefessan force-pushed the z-2020-10-19-provide-opam-during-build branch from da2df97 to 9090304 Compare October 12, 2020 08:57
@lefessan
Copy link
Contributor Author

I have done the fix using OpamPath.
For the hook variable, it was not clear to me how to find the location of the opam description file from the place where hook variables are defined.
Moreover, using such a variable means creating an incompatibility between the current and new versions of opam, since the plugin would have to put a hook description that would depend on the version of opam. From that point of view, I would prefer the env. variable or the (current with this PR) implicit file location (opam-bin checks for the existence of the file, and calls opam if the file does not exist, which is compatible with new and old opam versions).

Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

Suggested changes mostly for style, but there is a reason for not computing the path relative to build_dir, for cases where e.g. --inplace-build was used and build_dir can be outside of the opam-controlled dirs.

src/format/opamPath.ml Outdated Show resolved Hide resolved
src/format/opamPath.mli Outdated Show resolved Hide resolved
src/client/opamAction.ml Outdated Show resolved Hide resolved
src/client/opamSolution.ml Outdated Show resolved Hide resolved
Co-authored-by: Louis Gesbert <louis.gesbert@ocamlpro.com>
@lefessan lefessan force-pushed the z-2020-10-19-provide-opam-during-build branch from 4814e6b to 84bb121 Compare October 12, 2020 14:23
@rjbou rjbou added this to PR in Progress in Opam 2.1.x via automation Oct 12, 2020
@rjbou rjbou added this to the 2.1.0~beta3 milestone Oct 12, 2020
master_changes.md Outdated Show resolved Hide resolved
Co-authored-by: R. Boujbel <rjbou@ocamlpro.com>
@rjbou
Copy link
Collaborator

rjbou commented Oct 27, 2020

Closing as #4402 merged. Thanks for the PR!

@rjbou rjbou closed this Oct 27, 2020
Opam 2.1.x automation moved this from PR in Progress to Done Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Opam 2.1.x
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants