Cli versioning #4385
Cli versioning #4385
Conversation
|
Wow, pretty heavy infrastructure work... Seems pretty clean given the scope :)
|
If a flag is introduced in 3.0, deprecated in 3.1 and removed in 3.2 :
Some notes on the implementation:
+1 for reftests!!! |
src/client/opamArg.ml
Outdated
| let cli2_0 = OpamCLIVersion.of_string "2.0" | ||
| let cli2_1 = OpamCLIVersion.of_string "2.1" | ||
| let cli2_3 = OpamCLIVersion.of_string "2.3" | ||
| let cli3_0 = OpamCLIVersion.of_string "3.0" | ||
| let valid_cli_legacy = Valid_since cli2_0 |
There was a problem hiding this comment.
These are dropped here, but maybe there is a better place to put them. I thought about directly in OpamCLIVersion, but it would be quite more verbose to write/read.
|
Finally looking properly at this - many thanks for taking the heavy lifting of this task list! A few things are not working quite as I envisaged in the spec, but I expect this to be tweaking, rather than a wholesale problem! The idea with the CLI versioning is that we only deprecate features, rather than commands. So, taking
edit: add checkbox |
No mention of removal if help called with 2.0 (can be changed).
As it is a message in the code, and not in option management, a specific behavior need to be called when the message is displayed. The problem here is that we don't have the information about the requested cli version in all modules, only on opam-client & those depending on ones. |
|
There is some options that was removed in 2.0 betas and reintroduced in 2.0 (cf. #3685, for non erroneous message). In cli versioning it means that it was introduced in 2.0 and removed in 2.0, that errors. It is possible to remove them completely in 2.1, but some behaviour can't be reintroduced as it was not even present in 2.0. It concerns |
There was a problem hiding this comment.
This looks basically wonderful - some notes and comments on the API, but there's nothing which looks major:
- For
--no-aspcud,--just-file(old behaviour),--no-autoinstall,--alias-of, the behaviour has changed from "ignore the flag" to "syntax error", so I'd keepOpamArg.deprecated_optionfor 2.0 so that it's ignored in OPAMCLI=2.0 (for compatibility) and is an error in opam 2.1+ - I don't follow the need for the polymorphic variant on the content field in
OpamArg.Mk, unless there's a weird interaction with Cmdliner that I'm not yet seeing? OPAMCLI=2.0 opam optionshould display an error,OPAMCLI opam option --globaldoes, I guess it's just some threading of cli version for the default behaviour of a new command?- Given that all the uses of, say,
OpamArg.print_short_flaghave had to be updated to includecli, I'd makevaliditynon-optional just to be really sure that if it gets added elsewhere that adding the version isn't forgotten. - It's not this PR which has done it, but we've lost the description for the opam admin command somewhere along the line:
COMMANDS
admin
clean
Cleans up opam caches
|
Are there still some un-pushed commits? You've ticked "--no-aspcud, --just-file (old behaviour), --no-autoinstall, --alias-of", for example, but those CLI options aren't yet restored? Similarly for |
|
All comments that are marked as resolved but not present in the PR. I'll post a response to the main post when I'll finish. The pushed commits are only from suggestions comments. |
|
Thanks for the detailed review!
I kept just messages printing to not reintroduce the function. For
In fact, there is no need of a polymorphic variant, a simple variant is sufficient. I'm not against having a variant instead of polymorphic variant, it's just a new type to declare.
Not yet implemented, it needs another mechanism (this one handles only options & flags). But with all this all done, it should be not so hard to add.
Agree on it, updated accordingly.
It was removed with changes in 97c2252 |
|
Commands added! |
| | None -> { validity with content = `Valid content} | ||
| | Some _ -> { validity with content = `Removed content} | ||
|
|
||
| let cli_from valid = { valid ; removed = None; content = () } |
There was a problem hiding this comment.
This can also be implemented for a more simple use as
val cli_from: string -> valitidy
let cli_from version =
try
let valid = OpamCLIVersion.of_string version in
if OpamCLIVersion.is_supported valid then
{ valid; removed = None; content = () }
else
OpamConsole.error_and_exit `Internal_error "not supported"
with Failure _ -> OpamConsole.error_and_exit `Internal_error "not a valid cli"
but it will require to define newer version as supported (for example when we know that we want to deprecate an option in a future opam cli version), and we loose a strong check at compile time replaced by a runtime failure.
Oops - duly amending from " |
There was a problem hiding this comment.
A few minor nits (all as code suggestions). Why did the restored --no-aspcud (opam-install) and --alias-of/--no-autoinstall (opam-switch) end up referencing the 2.2 cli? I was expecting for those to be CLI errors in 2.1. It's just 2.0 that wants to display the message that they're being ignored (if, for example, there are still scripts using --no-ascpud - it's OK that they need to switch to using OPAMCLI=2.0, but that's the only change 2.0 scripts should have to have added).
The implementation for subcommands looks great (and is working as I anticipated too), thanks.
A few doc string updates, but otherwise I think this is ready to go in - thanks again for the considerable amount of work on it!
|
The AppVeyor failure can be ignored - the Ubuntu test failure appears to be because of... CLI versioning 😊 Apart from that, I think is ready to go in? |
* All `mk_*' function are enclosed in a `Mk' module, that handles internally cli validity * A `validty' can be retrieved via `cli_from' for new options, and `cli_between' for removed one * The validity is an argument to `mk_*' functions, to chek internally their validity regarding the given cli
update reftests
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
Implementing CLI versioning, handling of the different CLIs from spec.
It is only handled for subcommands and flags, not principal commands. A note is added in the manpage depending of flag validity and requested cli. When a not valid flag is used, a message is also printed to the user with cli indication, etc.
See discussion below for more precisions
Summary of new
[N]and deprecated[D]options in 2.1 (diff of manpages from 2.0.7 & master).opam commands
lock[N]option[N]opam globals options
--cli[N]package build options
--assume-depexts[N]--locked(behaviour change)--lock-suffix[N]--no-depexts[N]--unlock-base[D]--update-invariant[N]opam admin add-hashes
--packages[N]opam config
set[D]unset[D]set-global[D]unset-global[D]var[D]exec[D]opam env
--check[N]opam info/show
--all-versions[N]--just-file[N]--sort[N]--file[D]opam install
--check[N]--depext-only[N]--ignore-conflicts[N]opam pin
scan[N]--normalise[N]--with-version[N]opam switch
invariant[N]set-invariant[N]set-base[D]install[D]--force[N]--formula[N]--freeze[N]--no-action[N]opam unpin
--normalise[N]--with-version[N]opam upgrade
--installed[N]opam var
--global[N]