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

opam tree: Allow packages with a specific version, directories or local opam files, as input #5613

Merged
merged 5 commits into from
Aug 24, 2023

Conversation

kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Jul 24, 2023

Fixes #5493
Fixes #5287

Additional questions:

  • do we want to support --recursive and --subpath ?
  • do we want to support --with-version ?

my personal opinion for those is to say "we can merge this now, open an issue for those options and leave it for later"


TODO:

  • Add some reftests

@kit-ty-kate
Copy link
Member Author

Additional questions:

* do we want to support `--recursive` and `--subpath` ?

* do we want to support `--with-version` ?

thinking about it some more i think we should not support these option as their name are confusing and hits towards an action in opam-tree instead of in the underlying opam-pin action. I would advise for thinking about them again for 2.3.

@rjbou
Copy link
Collaborator

rjbou commented Aug 1, 2023

I think we should support --recursive && --subpath as they are supported by other commands that permit directory lookup for opam files (lint, upgrade, install, etc.).
For --with-version, it is a pin specific option, so we shouldn't support.

These command shortcuts, ie have access to opam install/lint . instead of opam pin . ; opam install/lint <pkgs>, ease a lot the usage, but complexify set of handled option. Plus usually it is only a subset of flags that are ported, so sometimes users expect to have such option but it is not available. It is not a simple subject.

Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

The global idea is great, it would simplify opam tree usage.
I've put some remarks on the code itself, some tiny optimisations, support of rec/subpath & tests.

src/client/opamTreeCommand.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Show resolved Hide resolved
src/client/opamTreeCommand.ml Outdated Show resolved Hide resolved
src/client/opamTreeCommand.ml Outdated Show resolved Hide resolved
tests/reftests/tree.test Show resolved Hide resolved
master_changes.md Show resolved Hide resolved
@kit-ty-kate kit-ty-kate force-pushed the opam-tree-pkg-dir branch 2 times, most recently from 8b604aa to b27a341 Compare August 15, 2023 19:23
@kit-ty-kate kit-ty-kate requested a review from rjbou August 15, 2023 19:24
@kit-ty-kate kit-ty-kate force-pushed the opam-tree-pkg-dir branch 2 times, most recently from d11d450 to beddf05 Compare August 16, 2023 20:04
master_changes.md Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
tests/reftests/tree.test Show resolved Hide resolved
src/client/opamTreeCommand.ml Outdated Show resolved Hide resolved
tests/reftests/tree.test Show resolved Hide resolved
src/client/opamTreeCommand.ml Outdated Show resolved Hide resolved
master_changes.md Show resolved Hide resolved
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

All good, thanks!

@rjbou rjbou added this to the 2.2.0~alpha3 milestone Aug 24, 2023
@kit-ty-kate kit-ty-kate merged commit 8a23fc0 into ocaml:master Aug 24, 2023
26 checks passed
@kit-ty-kate kit-ty-kate deleted the opam-tree-pkg-dir branch August 24, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opam tree <pkgname>.<version> isn't supported Add support for opam tree <dir>
2 participants