Add an admin command to compare 2 package versions

[mbarbin]

Add a new CLI opam admin compare-versions to compare package versions for sanity checks.

This command has 2 modes:

1. By default - user interactive.

In this mode, we provide two versions, and opam outputs a user-friendly expression with the result of the comparison, spelled out. For example:

$ opam admin compare-versions 0.0.9 0.0.10
 0.0.9 < 0.0.10

The command exits 0 regardless of the result of the comparison.

2. For use in scripts - if needed. Adding the argument --assert=OP.

In this mode, the output is suppressed (the command is silent). The result of the command will be encoded in the exit code, to be used in bash conditionals:

exit 0: the comparison holds exit 1: it doesn't

For example:

$ opam admin compare-versions 0.0.9 0.0.10 --assert='<'
[0]

$ opam admin compare-versions 0.0.9 0.0.10 --assert='>='
[1]

Original notes

This is meant for quick sanity checks for example.

• Added some basic tests to cover the command. The tests I added do not duplicate the tests for the actual comparison function, rather that meant to cover the basic cases encountered by the command.

• [x] Please update master_changes.md file with your changes.

This is following a discussion with @kit-ty-kate in #6118.

Note to reviewers:

I wasn't too sure where to expose the new command. It's not really an admin command, but I couldn't find another suitable section.

This is my first PR on opam and I am not familiar with the code base, as well as some dependencies (e.g. cmdliner). Please feel free to request for as many changes as you like. Thank you!

[avsm]

I recommend following the design of dpkg --compare-versions instead, where the op can also be specified. The return value is then a simple true (0) or false (non-zero).

https://manpages.ubuntu.com/manpages/oracular/en/man1/dpkg.1.html

      --compare-versions ver1 op ver2
           Compare version numbers, where op is a binary operator.  dpkg returns true (0) if the
           specified condition is satisfied, and false (1) otherwise.  There are two groups of
           operators, which differ in how they treat an empty ver1 or ver2.  These treat an empty
           version as earlier than any version: lt le eq ne ge gt.  These treat an empty version
           as later than any version: lt-nl le-nl ge-nl gt-nl.  These are provided only for
           compatibility with control file syntax: < << <= = >= >> >.  The < and > operators are
           obsolete and should not be used, due to confusing semantics.  To illustrate: 0.1 < 0.1
           evaluates to true.

[mbarbin]

Hello @avsm so nice to have you join the conversation!

Thank you for pointing out that command, I'll have a look. At quick glance, this can suggest that this new opam command could be named compare-versions (and the "-package-" middle part doesn't add much).

About the design I'd be curious what usage do you have in mind. I sort of see the appeal to re-using existing designs, but in this case, I feel:

1. dpkg sounds like it is bound by some historical deprecation constraints, which would be nice not to be tied to in opam from the get-go, unless there are applicable reasons
2. dpkg offers an interface which I believe, due to the 3 cases of the expected results, requires at least 2 calls to determine the outcome with no ambiguity, if the result of your query returns false (for example, if you are surprised that a strict-less-than doesn't hold, you still don't know whether you have an equal, or strictly-greater-than). Having the command return a result equivalent to an ordering.i just seems more simple, doesn't it? As a mere mortal, it's also friendly to be shown the plain ordering expression spelled out (I can tell it is going to take me some time to completely mentally digest that --compare-versions spec !).

The part about empty versions of dpkg just sounds very confusing to me. Is this due to a command parsing technicality, or are there really support for empty package versions out there? Does opam support empty package versions too?

[mbarbin]

Adding a note about empty versions:

The part about empty versions of dpkg just sounds very confusing to me. Is this due to a command parsing technicality, or are there really support for empty package versions out there? Does opam support empty package versions too?

As of the current tip of this PR:

$ ./_build/default/src/client/opamMain.exe admin compare-package-versions '' '1'
opam admin: VERSION argument: Package version can't be empty
Usage: opam admin compare-package-versions [OPTION]… VERSION VERSION
Try 'opam admin compare-package-versions --help' or 'opam admin --help' for more information.

So if we wanted to support empty version, some more changes would be required.

[avsm]

I have no strong opinion, but outputting shell operators like > or < is usually the worst possible interface to a shell interface where escaping is a dark art at the best of times ;-)

Two other options that are more "unix-like":

• unix commands usually exit with 0 for success and non-zero for inequality, so you could exit with -1 or 1 to indicate the results of the comparison
• you could take a list of versions on the command line argument and return them in ascending sorted order on stdout. That plays well with xargs/IFS style manipulations from shell scripts.

[mbarbin]

@avsm Thanks a lot for this input, this is really helpful. I am going to explore ideas base on your suggestions, reverting the PR as a draft for now.

[kit-ty-kate]

I don't personally see much value in a ""unix-style"" command. If people wanted that it'd be easier for them to just create a dedicated program using the opam-core library. opam admin commands are not the kind of command meant to be used as part of a script in this way. The current state of the PR looked fine by me

[avsm]

opam admin commands are not the kind of command meant to be used as part of a script in this way.

How are they meant to be used, then? The majority of invocations of these commands are part of a sequence of shell commands -- be they interactive, Dockerfiles, obuilder outputs, or whatever else. opam even documents EXIT STATUS specifically for each subcommand to reflect this.

[mbarbin]

@kit-ty-kate I think there could be a middle ground: if we add an optional --assert (<,>,=,...) command parameter to the state of this RP, with the understanding that when supplied, it affects the exit code accordingly, I think we combine the benefits of the approaches. Do you like it?

How about the name 'compare-versions' vs 'compare-package-versions' ? I have renamed locally, let me know if I should push this commit or not.

[dbuenzli]

Not following the discussion but just note that exit (-1) is going to be 255 in your shell.

[kit-ty-kate]

How are they meant to be used, then?

As mentioned in https://github.com/ocaml/opam/issues/6118 this would be used as part of a double-check for packages unfamiliar with opam/debian version ordering, so always interactively.

What use-case would there be in an automated context?

[avsm]

I use something similar in my personal opam repo (full of my homebrew shell scripts) to find the latest package revision from a directory to choose which one to install (just from looking at the opam files; there is no initialised opam here for various reasons yet and so no solver). And more generally, I've used almost every command exposed by opam at some point in a shell script while doing something-or-other with the repositories -- it's just how CLIs are used!

I'm not saying there's anything wrong with the design proposed in the original PR, except for a preference to not output known-shell-fragments (but even that's probably fine these days). But the ability to sort a list of package names+versions would definitely be something I find useful -- I've written OpamVersionCompare fragments for that several times in different contexts. Thinking about @dbuenzli's comment a bit more -- we never have a meaningful case where two versions are the same opam versions but lexically different, do we? So the ordering logic just needs to sort in ascending order, and then equal versions would be distinguishable by the fact they're the same string. No need for exit codes then.

[kit-ty-kate]

we never have a meaningful case where two versions are the same opam versions but lexically different, do we?

we do. For example: 1.00 = 1.0

[avsm]

Bah, good example! I thought those were different, but what I was thinking of was the common mistake of "1.0.0" <> "1.0" in package formula. So checking for equality is significant too, so it's not enough to just output a sorted list of package versions.

[mbarbin]

I added a few commit to explore having a mode where the command can exit with 0 or 1, based on the OP input from @avsm. This is simply to add more context to the conversation. #6128 This let you preview also what the change looks like with the shorter name compare-versions.

[mbarbin]

I updated the PR based on the previous comments and will proceed to re-opening for another round of review. Thanks!

[mbarbin]

Hello. I have rebased the PR.

I wanted to add a note about the addition of the --assert flag. After considering @avsm's input, I later imagined a reasonable use case, which I am including below.

This use case involves a setup using a release.sh script to release opam packages. I could imagine adding a new section to perform a sanity check to prevent backward releases, like this:

#!/bin/sh -e

# Some initial steps to extract the versions
OPAM='./_build/default/src/client/opamMain.exe'
PREVIOUS_VERSION=$1
NEW_VERSION=$2
IS_POINT_RELEASE=${3:-false} # This variable indicates if we are doing a point release
# ...

echo "Checking version ordering ..."

if [ "$IS_POINT_RELEASE" = false ]; then
  if ! $OPAM admin compare-versions $PREVIOUS_VERSION --assert '<' $NEW_VERSION; then
    echo "Unexpected version ordering - aborting release"
    exit 1
  fi
fi

echo "Carrying on with the release script ..."
# ...

[mbarbin]

Thanks a lot @kit-ty-kate for the review and for pointing me to the existing comparison operators. I updated as you suggested.

[dra27]

Slightly late to the party, but especially given the subsequent perfectly reasonable example:

  if ! $OPAM admin compare-versions $PREVIOUS_VERSION --assert '<' $NEW_VERSION; then

I'm left wondering why we've ended up with the --assert and not just gone with the 2 argument and 3 argument versions @avsm originally proposed, especially given that we have also ended up using operators that do need quoting anyway.

(I'd prefer to have the feature than not, though!)

[kit-ty-kate]

I'm ok with having both a 2 arguments and a 3 arguments version of that command

[mbarbin]

Hi @dra27 and thank you for joining the party!

I may have missed the specific proposal you are referring to. To make sure I understand it, in this proposal, the arguments are all positional, and depending on the number of arguments (2 or 3), you select the mode? Such as:

$ opam admin compare-versions 0.0.9 0.0.10
 0.0.9 < 0.0.10

$ opam admin compare-versions 0.0.9 <= 0.0.10
[0]

$ opam admin compare-versions 0.0.9 >= 0.0.10
[1]

Like this?

I would have to check a bit more what this entails to in cmdliner. My vague impression was that positional argument must be typed "statically" according to their position in the command line, but perhaps parsing them all as strings and deferring until the command handler should allow to make something like this work. For this reason, I tend to prefer the named --assert version. What do you think?

If you are thinking about something else entirely, please let me know!

[mbarbin]

@kit-ty-kate I am attempting to rebase this PR and am encountering a build issue, related to vendored opam-0install-cudf lib:

How do you update vendored libs in the opam repository? Is this something I have to do locally as well (such as a git submodule sync or smth), or are the updates committed and pushed to the repo?

File "src/solver/opamBuiltin0install.ml", lines 138-139, characters 4-42:
138 | ....Opam_0install_cudf.create
139 |       ~prefer_oldest ~handle_avoid_version..................
Error: The function Opam_0install_cudf.create has type
         ?prefer_oldest:bool ->
         constraints:(Cudf_types.pkgname *
                      (Cudf_types.relop * Cudf_types.version))
                     list ->
         Cudf.universe -> Opam_0install_cudf.t
       It is applied to too many arguments
File "src/solver/opamBuiltin0install.ml", line 139, characters 22-42:
139 |       ~prefer_oldest ~handle_avoid_version ~prefer_installed
                            ^^^^^^^^^^^^^^^^^^^^
  This extra argument is not expected.
make: *** [Makefile:138: build-opam] Error 1       

[mbarbin]

@dra27 thinking a bit more about the modes, I should add that it didn't even occur to me (until now) that we could design a syntax that makes use of the infix operators placed in postfix position. I'm inclined to think that it decreases somewhat the readability, but that may just be a personal preference.

That is, I think I prefer:

$ opam admin compare-versions 0.0.9 --assert '<=' 0.0.10

over:

$ opam admin compare-versions 0.0.9 0.0.10 <=

But I can probably get over it if that's what you're thinking about ! :-)

[kit-ty-kate]

@kit-ty-kate I am attempting to rebase this PR and am encountering a build issue, related to vendored opam-0install-cudf lib:

How do you update vendored libs in the opam repository? Is this something I have to do locally as well (such as a git submodule sync or smth), or are the updates committed and pushed to the repo?

It all depends on your setup. If you've used opam to install the dependencies then opam update && opam upgrade opam-0install-cudf will do. If you've used the vendored method (aka. ./configure --with-vendored-deps or make cold) then doing another ./configure --with-vendored-deps or make cold (depending on what you used) would fix your issue as well

[kit-ty-kate]

over:

$ opam admin compare-versions 0.0.9 0.0.10 <=

what about:

opam admin compare-versions 0.0.9 '<=' 0.0.10

[dra27]

Definitely not proposing having them in postfix position, no! As in https://github.com/ocaml/opam/pull/6124#issuecomment-2258285695 and https://github.com/ocaml/opam/pull/6124#issuecomment-2329149715, with the operator as an infix.

I think cmdliner can just give a list of positional arguments or at least regard one of three as optional - so still having the operator-less version you originally proposed (displaying the result) but also having the scripted version

[mbarbin]

I think cmdliner can just give a list of positional arguments or at least regard one of three as optional - so still having the operator-less version you originally proposed (displaying the result) but also having the scripted version

That sounds interesting to me. I thought this would be rejected (doesn't it open the door for specs that quickly become ambiguous?). I want to explore this a bit more, in particular see what happens when you "squeeze" optional positional args in the middle of required positional args. I created https://github.com/mbarbin/cmdlang/issues/6 to track.

Short of that:

1. matching on the number of args equates to the power of a monad, so that's not standard I suppose.

match%bind Arg.count_positional with
| 2 ->
  let%map v1 = pos 0 version
  and v2 = pos 1 version in
  `Compute (v1, v2)
| 3 ->
  let%map v1 = pos 0 version
  and op = pos 1 operator
  and v2 = pos 2 version in
  `Assert (v1, op, v2)

2. Doing something more dynamic with a pos_all and parsing arguments in the body of the command is going to cut down on the ability for cmdliner to generate a precise usage page automatically. It is not obvious to me how to estimate the overall gain then, compared to --assert.

As a side note, selecting the mode of a multi-mode commands with a named arguments feels more natural to me rather than by the presence/absence of positional arguments, but I don't think this argument is tenable, as there are probably countless counter-examples in the Unix literature. Plus, @dra27 's input seem to convey the opposite preference, at least in this instance, so I guess I'm happy not to account for my personal preference here.

I'm sort of stuck there. Is there another combinator I am not thinking of from cmdliner?

[kit-ty-kate]

I'm sort of stuck there. Is there another combinator I am not thinking of from cmdliner?

Isn't that possible with 3 Cmdliner.Arg.pos, where the first and third one are given to Cmdliner.Arg.required and the second is given to Cmdliner.Arg.value ?

[mbarbin]

Isn't that possible with 3 Cmdliner.Arg.pos, where the first and third one are given to Cmdliner.Arg.required and the second is given to Cmdliner.Arg.value ?

That's what I want to explore a bit more. I am used to hitting this limitation when attempting this sort of things.

Let me play a bit with it and I'll get back to you. Thanks for your help!

[kit-ty-kate]

Mmh, yeah it looks like cmdliner doesn't support that given positional arguments are absolute and do not change depending on optional parameters

let test arg1 arg2 arg3 =
  Printf.printf "arg1 = %s\narg2 = %s\narg3 = %s\n"
    arg1 (match arg2 with None -> "None" | Some x -> "Some "^x) arg3

module Arg = Cmdliner.Arg
module Cmd = Cmdliner.Cmd
module Term = Cmdliner.Term

let arg1 = Arg.(required & pos 0 (some string) None & info [])
let arg2 = Arg.(value & pos 1 (some string) None & info [])
let arg3 = Arg.(required & pos 2 (some string) None & info [])

let cmd =
  let info = Cmd.info "test" ~version:"dev" in
  Cmd.v info Term.(const test $ arg1 $ arg2 $ arg3)

let main () = Stdlib.exit (Cmd.eval cmd)
let () = main ()

$ ./a.out a b c
arg1 = a
arg2 = Some b
arg3 = c
$ ./a.out a c
test: a required argument is missing
Usage: test [OPTION]… ARG [ARG] ARG
Try 'test --help' for more information.

let's keep --assert. I'll open a ticket in cmdliner in case upstream wants to do have variable positional arguments support in the future if it's ever useful for anyone else.

[dbuenzli]

Honestly I think you are all a bit overthinking that :-) Simply ask for three positional arguments make the last one optional, resolve that in code with a proper match and write the SYNOPSIS section yourself.