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

Add an admin command to compare 2 package versions #6124

Closed
wants to merge 12 commits into from

Conversation

mbarbin
Copy link
Contributor

@mbarbin mbarbin commented Jul 30, 2024

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.

  1. 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.

  • 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!

@mbarbin mbarbin force-pushed the compare-package-versions branch 2 times, most recently from 64eb403 to a9049b1 Compare July 30, 2024 11:54
@avsm
Copy link
Member

avsm commented Jul 30, 2024

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
Copy link
Contributor Author

mbarbin commented Jul 30, 2024

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
Copy link
Contributor Author

mbarbin commented Jul 30, 2024

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.

@rjbou rjbou self-requested a review July 30, 2024 14:11
@avsm
Copy link
Member

avsm commented Jul 30, 2024

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 mbarbin marked this pull request as draft July 30, 2024 15:07
@mbarbin
Copy link
Contributor Author

mbarbin commented Jul 30, 2024

@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
Copy link
Member

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
Copy link
Member

avsm commented Jul 30, 2024

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
Copy link
Contributor Author

mbarbin commented Jul 30, 2024

@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
Copy link
Contributor

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

@kit-ty-kate
Copy link
Member

How are they meant to be used, then?

As mentioned in #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
Copy link
Member

avsm commented Jul 30, 2024

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
Copy link
Member

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
Copy link
Member

avsm commented Jul 30, 2024

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
Copy link
Contributor Author

mbarbin commented Jul 30, 2024

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
Copy link
Contributor Author

mbarbin commented Aug 5, 2024

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

@mbarbin mbarbin marked this pull request as ready for review August 5, 2024 13:12
@mbarbin
Copy link
Contributor Author

mbarbin commented Aug 12, 2024

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 ..."
# ...

Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

otherwise that looks good to me

src/client/opamAdminCommand.ml Outdated Show resolved Hide resolved
src/client/opamAdminCommand.ml Outdated Show resolved Hide resolved
@mbarbin
Copy link
Contributor Author

mbarbin commented Aug 13, 2024

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

@kit-ty-kate kit-ty-kate added this to the 2.3.0~alpha milestone Aug 13, 2024
@mbarbin
Copy link
Contributor Author

mbarbin commented Sep 12, 2024

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 mbarbin/cmdlang#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)
  1. 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
Copy link
Member

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
Copy link
Contributor Author

mbarbin commented Sep 12, 2024

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
Copy link
Member

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
Copy link
Contributor

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.

@mbarbin
Copy link
Contributor Author

mbarbin commented Sep 12, 2024

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.

Thanks for the input @dbuenzli!

Personally, I wouldn't know how to defend doing that, versus keeping withing a well supported use case and keeping the --assert flag. This sort of feels arbitrary to me.

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.

My inclination is for cmdliner to align with the core.command limitation and cleanly reject these kinds of specs. I also don't intend to support this use case in cmdlang and I doubt climate will support it either.

@dbuenzli
Copy link
Contributor

Also I often find it that when people try to do subtle things with cli, it's often not in the interest of the end-users.

In this particular case, why not simply always require the operator ? I don't think having a default operator brings much here except that I will have to remember it.

@dbuenzli
Copy link
Contributor

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.

I closed it. The problem is that in general this leads to ambiguous parses. There are already enough of these to confuse users, let's not add more.

@mbarbin
Copy link
Contributor Author

mbarbin commented Sep 12, 2024

In this particular case, why not simply always require the operator ? I don't think having a default operator brings much here except that I will have to remember it.

Are you proposing to add a syntax for a special "don't know" operator? Like:

$ opam admin compare-versions 0.0.9 '?' 0.0.10
0.0.9 < 0.0.10

@dbuenzli
Copy link
Contributor

Are you proposing to add a syntax for a special "don't know" operator? Like:

Rather that you don't need this mode of operation.

The operators <, =, <=, with exit 0 if the assertion holds and 1 if it does not is all you need.

@mbarbin
Copy link
Contributor Author

mbarbin commented Sep 12, 2024

The operators <, =, <=, with exit 0 if the assertion holds and 1 if it does not is all you need.

If the command also prints on stderr the corrected inequality, spelled out, in case of an error, I'd be OK with this. This is very close to my initial motivating use case.

$ opam admin compare-versions 0.33.0-preview.1 '<' 0.33.0
0.33.0-preview.1 > 0.33.0
[1]

@mbarbin
Copy link
Contributor Author

mbarbin commented Sep 12, 2024

@kit-ty-kate what do you think? I pushed a preview here (keeping the existing operators). Edit: subsumed by #6196

@dbuenzli
Copy link
Contributor

If the command also prints on stderr the corrected inequality, spelled out, in case of an error, I'd be OK with this.

I would find this a rather confusing cli. Why not simply output on stdout true or false which a --quiet will suppress.

@mbarbin
Copy link
Contributor Author

mbarbin commented Sep 12, 2024

As a reminder, I was trying to introduce a command that allows comparing versions from the command line, for which you are not sure a priori of the result. There are cases where the result is surprising, including cases of version equality for version that are syntactically different.

During the course of this conversation, I have tried avoiding designs that forces me to run the command multiple times, trying to "guess" what the correct result is each time, rather than being able to run a command once, and have the result nicely displayed on my screen. Any design that returns a response with 1 bit of information will suffer from the characteristic that I am trying to avoid, given that the answer I am looking for doesn't fit on 1 bit (Ordering.t having 3 variants).

The true|false version is no different from the silent exit {0|1} design on that regard.

I am starting to loose faith that I'll be able to provide a proposal that pleases everybody. I'm sorry if I have used too much of the reviewers time and patience, this really wasn't my intention to begin with.

@dbuenzli
Copy link
Contributor

As a reminder, I was trying to introduce a command that allows comparing versions from the command line, for which you are not sure a priori of the result

Mmh sorry I misunderstood what you were trying to provide because I inferred it from the command name you proposed, namely compare-versions. Personally I think the command name you proposed is not a good name for this.

I would rather suggest order-versions. I think having both is fine and useful. Basically:

admin compare-versions V0 OP V1  # Assert the equation both on stdout and with exit
admin order-versions V0 V1       # Output V0 OP V1 with OP such that the equation holds

Note that order-versions should never fail (unless V0 and V1 can't be ordered or parsed into versions).

@mbarbin
Copy link
Contributor Author

mbarbin commented Sep 12, 2024

Thank you for this reply.

Personally I think the command name you proposed is not a good name for this.

I suppose it depends on the perspective you adopt. See for example:

$ dune/otherlibs/stdune/src$ grep -RI 'val compare :'
unit.mli:val compare : t -> t -> Ordering.t
list.mli:val compare : 'a t -> 'a t -> compare:('a -> 'a -> Ordering.t) -> Ordering.t
env.mli:  val compare : t -> t -> Ordering.t
loc0.mli:val compare : t -> t -> Ordering.t
filename.mli:val compare : t -> t -> Ordering.t
path_intf.ml:  val compare : 'w t -> 'w t -> Ordering.t
bool.mli:val compare : t -> t -> Ordering.t
sexp.mli:val compare : t -> t -> Ordering.t
char.mli:val compare : t -> t -> Ordering.t
signal.mli:val compare : t -> t -> Ordering.t
string.mli:val compare : t -> t -> Ordering.t
ansi_color.ml:  val compare : t -> t -> Ordering.t
ansi_color.ml:  val compare : t -> t -> Ordering.t
user_message.mli:  val compare : t -> t -> Ordering.t
user_message.mli:val compare : t -> t -> Ordering.t
comparator.mli:  val compare : t -> t -> Ordering.t
set_intf.ml:  val compare : t -> t -> Ordering.t
ansi_color.mli:  val compare : t -> t -> Ordering.t
int.mli:val compare : t -> t -> Ordering.t
option.mli:val compare : ('a -> 'a -> Ordering.t) -> 'a t -> 'a t -> Ordering.t
bit_set.mli:  val compare : t -> t -> Ordering.t
path.ml:  val compare : t -> t -> Ordering.t
float.mli:val compare : t -> t -> Ordering.t
comparator.ml:  val compare : t -> t -> Ordering.t
lexbuf.mli:  val compare : t -> t -> Ordering.t
id.ml:  val compare : t -> t -> Ordering.t
id.mli:  val compare : t -> t -> Ordering.t
loc.mli:val compare : t -> t -> Ordering.t
poly.mli:val compare : 'a -> 'a -> Ordering.t
map_intf.ml:  val compare : 'a t -> 'a t -> compare:('a -> 'a -> Ordering.t) -> Ordering.t

See also base todo item.

I also understand the perspective of using a new terminology for ordering elements.

admin compare-versions V0 OP V1  # Assert the equation both on stdout and with exit
admin order-versions V0 V1       # Output V0 OP V1 with OP such that the equation holds

Note that order-versions should never fail (unless V0 and V1 can't be ordered or parsed into versions).

That makes sense to me. I agree this PR as it is now suffers from trying to fit two conflicting views into a single command. The behavior you describe for order-versions I think is close to what was there in the early history of this PR (e.g. 3b5cb56). Thank you!

@dbuenzli
Copy link
Contributor

I suppose it depends on the perspective you adopt

Sure, I'm not attached to the names I suggested, I just needed another name to make the point about not cramming the two behaviours in the same command.

@mbarbin
Copy link
Contributor Author

mbarbin commented Sep 13, 2024

I created a new draft trying to account for the various feedback received here. Please see : #6196

Thanks!

@mbarbin
Copy link
Contributor Author

mbarbin commented Sep 14, 2024

If we prefer non-quoted operators, there's also the possibility of using the operator names as flags (rather than "--assert") and being back to having 1 single command. I created a version based on in #6197 as an additional data point.

I'll have some time to look again into this PR and the end of next week. Thanks a lot!

@kit-ty-kate
Copy link
Member

Looking at all 3 PRs i think i prefer #6197 (opam admin compare-versions V1 [--eq|--lt|...] V2) so far, but we'll talk about it at the next meeting on monday so it's not a final answer or anything yet

@mbarbin
Copy link
Contributor Author

mbarbin commented Sep 25, 2024

Looking at all 3 PRs i think i prefer #6197

In retrospect that's the one I prefer as well so far. Thank you for following up and letting me know about your meeting. I'll be ready to help moving things forward as you see fit. Thanks a lot!

@kit-ty-kate
Copy link
Member

kit-ty-kate commented Sep 30, 2024

Dev meeting note: after discussion, #6197 looks preferable for all 3 of us (Raja, David, Kate)

@mbarbin
Copy link
Contributor Author

mbarbin commented Sep 30, 2024

Dev meeting note: after discussion, #6197 looks preferable for all 3 of us (Raja, David, Kate)

Great! I'll close the other ones, and do a little pass at it, so we can do a first round of review. Thank you!

@mbarbin mbarbin closed this Sep 30, 2024
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.

5 participants