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.

Sign up for GitHub

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

Jump to bottom

New package variable: opamfile #4402

Merged
merged 3 commits into from Oct 27, 2020
Merged

New package variable: opamfile #4402

merged 3 commits into from Oct 27, 2020

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Oct 21, 2020

superseeds #4387

@rjbou rjbou added this to the 2.1.0~beta3 milestone Oct 21, 2020
@rjbou rjbou added this to PR in Progress in Opam 2.1.x via automation Oct 21, 2020
AltGr
AltGr reviewed Oct 21, 2020
View reviewed changes
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! Some concerns about the generality of implementation, and maybe we could find a better name ? (-loc doesn't sound straightforward to me... Why not just opamfile or opam-file ?)

src/state/opamPackageVar.ml
in
OpamFile.OPAM.get_metadata_dir ~repos_roots opam
|> OpamStd.Option.map (fun d ->
OpamFilename.Op.(d//"opam")
Copy link
Member

Choose a reason for hiding this comment

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

Hm, does this work for pinned packages ? Can't the file be named differently?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for pinned packages, it's stored in the overlay dir, in the file opam, and from repo, its the package dir with name opam.

Copy link
Member

Choose a reason for hiding this comment

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

aah right, it'll never get back to the original srcdir, we always have a mirror, got it! 👍
maybe worth adding a comment ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's always there, in the current run of opam (repo dir are tmp ones), yes. In fact, it's the code of OpamRepositoryState.get_root, that is not reachable from OpamPackageVar.
+1 for the comment

@rjbou
Copy link
Collaborator Author

rjbou commented Oct 21, 2020

ok for me to rename it. opamfile can be misleading with the content of the opam file... first I thought about opamfile-path

@lefessan
Copy link
Contributor

I think opamfile is ok, nobody would expect the content of the file as an argument to a hook script, no ?

@rjbou rjbou moved this from PR in Progress to PR Finalised in Opam 2.1.x Oct 26, 2020
@rjbou rjbou changed the title New package variable: opamfile-loc New package variable: opamfile Oct 26, 2020
@rjbou rjbou merged commit 15e9a7a into ocaml:master Oct 27, 2020
Opam 2.1.x automation moved this from PR Finalised to Done Oct 27, 2020
@rjbou rjbou mentioned this pull request Oct 27, 2020
Closed
@dra27 dra27 mentioned this pull request Apr 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AltGr AltGr AltGr left review comments

Assignees
No one assigned
Labels
None yet
Projects
Opam 2.1.x
  
Done
Milestone
2.1.0~beta4
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rjbou @lefessan @AltGr