Skip to content

RFC: allow recursive lookup of opam files for opam pin .#3174

Closed
hnrgrgr wants to merge 5 commits intoocaml:masterfrom
hnrgrgr:recursive_pin
Closed

RFC: allow recursive lookup of opam files for opam pin .#3174
hnrgrgr wants to merge 5 commits intoocaml:masterfrom
hnrgrgr:recursive_pin

Conversation

@hnrgrgr
Copy link
Copy Markdown
Contributor

@hnrgrgr hnrgrgr commented Jan 14, 2018

This prototype allows opam pin . to pin opam files found in subdirectories.

Rational

When a source tree contains multiple package definitions, the current usage is to define every opam file in the project root. But jbuilder is also able to handle opam file in subdirectories (typically in a vendors subdirectory). It might useful to add the same flexibility in opam.

Note: running a script that individually pin all subpackages, e.g. https://gitlab.com/tezos/tezos/blob/master/scripts/opam-pin.sh, would not allow to use --kind=git for subdirectories.

Proposed implementation

This prototype contains two patches:

  • The first patch extends OpamFile.URL to add a subpath field. When building such a package, the source will be fetched as usual, but then only the 'subpath' of the fetched source will be copied to the 'build_dir'. This field is only useful "VCS" backends, and not for the 'rsync' backend.

  • The second patch implements a recursive lookup of opam files. Opam files found in subdirectories are pinned with a correct 'subpath'.

User interface

With this prototype implementation there is only two ways for using the new 'subpath' field: either commit the 'subpath' field in the opam repository, or use opam pin ..

A more complete CLI might add:

  • opam pin add tezos-base git+https://gitlab.com/tezos/tezos --subpath lib_base

  • opam pin . --rec if we do not want to activate the recursive lookup by default

  • opam update -u . only update/upgrade sources for development packages found in subdirectories

  • opam upgrade . --installed only upgrade installed package (the current behaviour is to propose installation of all the sub package)

Note

After a thought, we might implement the 'recursive' lookup without relying on the new subpath field but this way we might more easily ignore updates that do not modify the 'subpath' of a given package.

@hnrgrgr
Add a field subpath in a package url.
78e04dc 
The semantics of this fields is to restrict the files copied from the
directory `sources/<package>` to the directory `build/<package>`.

A later patch might allow the `rsync` backend to only synchronize in
`sources` the `subpath`. Another patch might allow a `VCS` backend not
to rebuild a pinned package when the 'sources' were modified, but only
outside of the 'subpath'.
@AltGr
Copy link
Copy Markdown
Member

AltGr commented Jan 15, 2018

Thanks! This sounds great :)
I think that the subpath URL field is a good extension, and may prove useful (even for archives). Note that detection of changes would remain contrained by the whole archive/VC directory, but it's not really a problem.

For the recursive pin, I have a few questions and ideas of corner cases:

  • maybe make it explicit, e.g. opam pin DIR -r ? I am not sure exactly of the cases of conflict, but for example, when I debug a dependency of my main project, jbuilder makes it convenient to link (or copy) that project's source within the main project source tree. In this case I don't want opam to pin it. (Keep in mind that, while in theory opam uses the files as version-controlled, the lookup for opam files happens on the on-disk file tree with opam pin DIR)
  • this seems to be for many packages in a single VC repo. Do you handle the cases where multiple projects are in multiple ones ? E.g. my-projects/projectA.git/, my-projects/projectB.git/ and opam pin my-projects ? In this case we should pin projectA and projectB without using subpath, is that handled ?

this way we might more easily ignore updates that do not modify the 'subpath' of a given package.

Not sure. For VC, we could add commands to check for changes in a specific dir, but that would be involved (the update command atm returns the changed or not status for the whole repo). And for archives, we rely on the archive hash, so it can't be done.

In any case, it's too late for merging this into 2.0.0, but I'm looking forward to merging it after the release :). Thanks!

@AltGr
Copy link
Copy Markdown
Member

AltGr commented Jan 15, 2018

Also, for subpath: did you make sure to review all places where the source is also used ? For example, the opam source command, the execution of the remove scripts, etc.
We should also be careful where we look for updates of the metadata on opam update.

@AltGr
Copy link
Copy Markdown
Member

AltGr commented Jan 16, 2018

Something that crossed my mind, for opam install DIR, there is also handling for unpinning, e.g., if you had foo pinned to . and the foo.opam file was removed, opam will sync by unpinning it. That should be updated as well, to handle recursivity. We should in general be careful about how this interacts with opam install|upgrade|remove DIR.

@hnrgrgr
Implements opam pin/lint/install --rec .
335a624 
When a source tree contains multiple package definitions, the current
usage is to define every opam file in the project root. But `jbuilder`
is also able to handle opam file in subdirectories (typically in a
`vendors` subdirectory). It might useful to add the same flexibility
in opam.
@hnrgrgr
Proper handling of subpath in opam upgrade/reinstall/remove <dir>
c7994f6 
All those commands are based on `OpamAuxCommand.resolve_local_pinned`.
The command now apply not only to all pinned packages to the exact
path `<dir>`, but to package where `<path>/<subpath>' is below
'<dir>'.
@hnrgrgr
Improve VCS.is_dirty when subpath is defined.
9b8e7f2
@hnrgrgr
Implements opam upgrade --installed .
d5bd0f4
@ghost ghost force-pushed the recursive_pin branch from 479056a to d5bd0f4 Compare January 20, 2018 23:29
@hnrgrgr
Copy link
Copy Markdown
Contributor Author

hnrgrgr commented Jan 20, 2018

I just updated the PR:

  • added an explicit option --rec/-r to opam pin/install/lint
  • proper handling of subpath in opam upgrade/remove/reinstall

This seems to be for many packages in a single VC repo. Do you handle the cases where multiple projects are in multiple ones ? E.g. my-projects/projectA.git/, my-projects/projectB.git/ and opam pin my-projects ? In this case we should pin projectA and projectB without using subpath, is that handled ?

Currently opam pin --rec my-projects would lookup recursively for all opam files, but it would pin them with the rsync backend. And, it should be the same for opam pin --rec my-projects/projectA.git/sub/dir.

In both cases, it would make sense to detect the VC repo, I will have a look.

Also, for subpath: did you make sure to review all places where the source is also used ?

I believe my review was exhaustive, at least for what I have been able to track in the sources. From a user perspective, I tested pin/unpin/install/upgrade/reinstall/remove/source.

Something that crossed my mind, for opam install DIR, there is also handling for unpinning, e.g., if you had foo pinned to . and the foo.opam file was removed, opam will sync by unpinning it. That should be updated as well, to handle recursivity.

It looks like unpinning in opam install DIR works with this PR.

We should in general be careful about how this interacts with opam install|upgrade|remove DIR.

I made a small amendment in how this PR interacts with opam upgrade/reinstall/remove DIR. Now opam will not only look for pinned packages for which the pinned path is exactly DIR, but also for which DIR is a prefix of path/subpath. This allows a scenario like:

I also added a flag --installed to opam upgrade DIR. With this flag, opam will not try to install not installed packages, but only to upgrade the previously installed packages. That might be useful in non-interactive scripts.

@AltGr
Copy link
Copy Markdown
Member

AltGr commented Jan 24, 2018

This looks great, thanks! Only thing missing, as you point out, could be a little more control on how/when subpath gets used when using pin . --recursive:

  • if in a single git repo, and foo/bar/bar.opam is found, pin bar to . using git with subpath=foo/bar
  • if however foo is a different repo (but not foo/bar), pin bar to foo using git, with subpath=foo ?

but what if . is not git-controlled, contains an opam file, but there are git-controlled subdirs containing opam files ? I am thinking the sanest would be never to mix different kinds of pinnings on a single opam pin DIR [--rec] command. Locate all opam files, then get the different targets (possibly with git+subpath, if target doesn't contain a .git directory but one of the directories between . and target does), and fail if they're not all of the same kind.
What do you think ?

Also, what do you do on opam pin DIR --rec --subpath foo, is that an error ?

NB: in opam list, we use --recursive (which cmdliner allows to shorten to --rec). It'd be better to keep the same name.

Anyway, I am pin-pointing possible corner cases, but this is good! I'll be glad to merge after 2.0.0.

@AltGr AltGr added this to the 2.1 milestone Feb 2, 2018
@AltGr AltGr mentioned this pull request Jul 27, 2018
Closed
@rjbou rjbou mentioned this pull request Aug 8, 2018
Merged
@AltGr
Copy link
Copy Markdown
Member

AltGr commented Apr 24, 2019

Superseded by #3499

@AltGr AltGr closed this Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.1.0~alpha

Development

Successfully merging this pull request may close these issues.

2 participants

@hnrgrgr @AltGr