Skip to content

Provide opam during build#4387

Closed
lefessan wants to merge 3 commits intoocaml:masterfrom
lefessan:z-2020-10-19-provide-opam-during-build
Closed

Provide opam during build#4387
lefessan wants to merge 3 commits intoocaml:masterfrom
lefessan:z-2020-10-19-provide-opam-during-build

Conversation

@lefessan
Copy link
Copy Markdown
Contributor

@lefessan lefessan commented Oct 11, 2020

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.

AltGr
AltGr requested changes Oct 12, 2020
View reviewed changes
Copy link
Copy Markdown
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?

@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
Copy Markdown
Contributor Author

lefessan commented Oct 12, 2020

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).

AltGr
AltGr reviewed Oct 12, 2020
View reviewed changes
Copy link
Copy Markdown
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.

@lefessan @AltGr
Update src/format/opamPath.ml
84bb121 
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 the 2.1.0~beta3 milestone Oct 12, 2020
rjbou
rjbou reviewed Oct 12, 2020
View reviewed changes
@lefessan @rjbou
Update master_changes.md
b691df4 
Co-authored-by: R. Boujbel <rjbou@ocamlpro.com>
@rjbou rjbou mentioned this pull request Oct 21, 2020
Merged
@rjbou
Copy link
Copy Markdown
Collaborator

rjbou commented Oct 27, 2020

Closing as #4402 merged. Thanks for the PR!

@rjbou rjbou closed this Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AltGr AltGr AltGr requested changes

@rjbou rjbou rjbou left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.1.0~beta4

Development

Successfully merging this pull request may close these issues.

3 participants

@lefessan @rjbou @AltGr