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

pkg: remove context arguments #9343

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Dec 1, 2023

We remove the --all-contexts and --context arguments from various package commands. Their usage in the tests has been replaced with the equivalent --workspace arguments.

They are replaced with positional lock directory arguments as suggested in #9324. It's quite natural for all our package commands to use this term, so I have put it in Pkg_common and simplified how we select lock directories in various commands.

@rgrinberg
Copy link
Member

Instead of a --workspace argument, it can be just a position argument pointing to the desired lock directory.

@rgrinberg rgrinberg requested a review from gridbugs December 1, 2023 06:34
@Alizter Alizter marked this pull request as draft December 1, 2023 14:07
@Alizter
Copy link
Collaborator Author

Alizter commented Dec 1, 2023

I originally missed #9324 so I haven't implemented the positional arguments. I'll mark this as draft and finish implementing it.

@Alizter Alizter force-pushed the ps/branch/pkg__remove_context_arguments branch from 121be2e to 7c0cb03 Compare December 5, 2023 18:31
@Alizter Alizter force-pushed the ps/branch/pkg__remove_context_arguments branch from 7c0cb03 to b6a9ea2 Compare December 5, 2023 23:20
bin/pkg/validate_lock_dir.ml Outdated Show resolved Hide resolved
@Alizter Alizter force-pushed the ps/branch/pkg__remove_context_arguments branch 4 times, most recently from d8c2279 to 4c16676 Compare December 7, 2023 04:44
@Alizter Alizter marked this pull request as ready for review December 7, 2023 04:44
@Alizter Alizter force-pushed the ps/branch/pkg__remove_context_arguments branch from 4c16676 to 7243252 Compare December 7, 2023 04:45
@Alizter
Copy link
Collaborator Author

Alizter commented Dec 7, 2023

@gridbugs This is now ready to be reviewed. Let me know if you have any questions.

@gridbugs
Copy link
Collaborator

gridbugs commented Dec 8, 2023

So it looks like the default behaviour of commands like dune pkg lock is to run for all lockdirs now, but you can pass lockdirs as anonymous arguments to have it run for just those lockdirs. I find that a but surprising as I thought we had a concept of a default lockdir which would be the target of commands unless otherwise specified. Since most other languages only allow a single lockfile per project, I think most users will expect that unless otherwise specified, commands that operate on lockdirs will only operate on a single lockdir. Plus considering command line tools in general when presenting a choice between a subset of options to users it's pretty rare for the default to be choosing everything. Usually if no elements are specified on the command line it's either an error or there's a conservative default behaviour.

My preference is for it to operate on the default lockdir when no lockdirs are specified and to allow anonymous args overriding which lockdirs are operated on, and add a flag --all which causes it to run on all lockdirs.

@Alizter Alizter force-pushed the ps/branch/pkg__remove_context_arguments branch from 7243252 to e0aeae2 Compare December 9, 2023 04:20
@Alizter
Copy link
Collaborator Author

Alizter commented Dec 9, 2023

My motivation for defaulting to all commands is that dune build will build targets in all contexts. Granted, we aren't coupling contexts to lock dirs any longer, so this might be misleading, but I think it is something that would be more intuitive for users.

@gridbugs Would you agree that the default behaviour of positional arguments is something we can decide on at a later time? We can follow up in an issue and discuss the pros and cons on either behaviour. I don't think it should prevent this PR from being accepted however.

@gridbugs
Copy link
Collaborator

My motivation for defaulting to all commands is that dune build will build targets in all contexts. Granted, we aren't coupling contexts to lock dirs any longer, so this might be misleading, but I think it is something that would be more intuitive for users.

I don't think the comparison to the way dune build treats contexts is apt since the main focus of that command is the target(s) being built. A more analogous comparison would be the way dune build treats targets, falling back to the @default alias when no targets are specified and having the user opt-in to building all targets via the @all alias.

@gridbugs Would you agree that the default behaviour of positional arguments is something we can decide on at a later time? We can follow up in an issue and discuss the pros and cons on either behaviour. I don't think it should prevent this PR from being accepted however.

The same could be said about any feedback regarding UX. Is there something in particular about this behaviour that merits merging it now and deferring further discussion? Are people blocked on this PR?

bin/pkg/pkg_common.ml Outdated Show resolved Hide resolved
@Alizter
Copy link
Collaborator Author

Alizter commented Dec 11, 2023

I'll change the default behaviour to dune.lock, but the disadvantages are:

  • dune.lock might not even be used by any context
  • It's harder to select all lock files (we will have to add an --all argument complicating the CLI further).

@Alizter Alizter force-pushed the ps/branch/pkg__remove_context_arguments branch 2 times, most recently from f0c717e to 2d203fc Compare December 11, 2023 18:35
@Alizter
Copy link
Collaborator Author

Alizter commented Dec 11, 2023

@gridbugs I've changed it so that it defaults to dune.lock and added a --all argument. This required some changes to dune describe pkg lock since we need to load the workspace in order to find all the lock directories to support --all.

@Alizter Alizter requested a review from gridbugs December 11, 2023 18:36
Copy link
Collaborator

@gridbugs gridbugs left a comment

Choose a reason for hiding this comment

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

Thanks @Alizter

Signed-off-by: Ali Caglayan <[email protected]>
@Alizter Alizter force-pushed the ps/branch/pkg__remove_context_arguments branch from 2d203fc to b92a1d4 Compare December 12, 2023 03:55
@rgrinberg rgrinberg merged commit c3e559a into ocaml:main Dec 12, 2023
24 checks passed
@Alizter Alizter deleted the ps/branch/pkg__remove_context_arguments branch December 12, 2023 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify CLI for handling lock directories
3 participants