Different behavior of version comparison comparing "GNU sort"

[sylvestre]

Feel free to wontfix my request because I do get that it is nitpicking :)

I am considering your lib to replace some of the logic in the rust coreutils
https://github.com/uutils/coreutils/blob/main/src/uucore/src/lib/mods/version_cmp.rs#L57

I noticed that GNU sort version a bit differently than version-compare:

$ echo "
5.4.0
5.04.0"| sort --stable --sort=version
5.4.0
5.04.0

With this crate, the following code is failing:

use version_compare::{compare, compare_to, Cmp, Version};

fn main() {
    let a = "string start 5.04.0 end of str";
    let b = "string start 5.4.0 end of str";
    assert_eq!(a > b, true);
}

as the code considers that 5.04.0 is smaller than 5.4.0

[timvisee]

I seem to get a different result with GNU's sort:

$ echo "
5.4.0
5.04.0"| sort --stable --sort=version
5.04.0
5.4.0

$ sort --version
sort (GNU coreutils) 8.32
...

which would make version-compare's output correct.

Is GNU's version ordering defined somewhere?

[sylvestre]

Looks they changed their behavior
I can reproduce what you have with coreutils 8.28 (on ubuntu bionic)

my initial test was with a Debian testing with coreutils 9.1

[timvisee]

I see!

The tricky part is that this crate is supposed to be general purpose. Meaning that, right now, it can't handle every exception across every system in version comparison logic. I must also be careful with adding exceptions as this would greatly increase complexity. I did have plans to rework this crate to make it modular in an attempt to provide something that can adapt to any version system, I never actually did this though (yet). For now, I generally go with "what seems most logical".

I am happy though to add an exception for this since I consider GNU's utils to be a big enough thing, and this is a rather small change anyway. This crate has a concept of Manifest which allows some configuration. A flag like gnu_ordering: bool can be added in:

version-compare/src/manifest.rs

Lines 26 to 35 in 9a965b4

	#[derive(Debug, Clone, PartialEq, Eq, Default)]
	pub struct Manifest {
	/// The maximum depth of a version number.
	///
	/// This specifies the maximum number of parts. There is no limit if `None` is set.
	pub max_depth: Option<usize>,
	/// Whether to ignore text parts in version strings.
	pub ignore_text: bool,
	}

What do you think of that?

Do you happen to know what the reasoning is behind this change by GNU?

[sylvestre]

That would be great!

I don't know why it changed on the GNU side

This PR shows why I reported this bug:
uutils/coreutils#4325

I am planning to use version-compare instead of our own implementation once this issue is fixed

[timvisee]

@sylvestre

I've implemented some basic logic for GNU number ordering, which can optionally be enabled through a Manifest.

This is part of revision 548bd90. Test it out by patching your Cargo.toml:

# Cargo.toml
[patch.crates-io]
version-compare = { git = "https://github.com/timvisee/version-compare", rev = "548bd90" }

Since this isn't default behavior it must be enabled explicitly using a Manifest.

use version_compare::{Version, Manifest};

fn main() {
    let mut gnu_manifest = Manifest::default();
    gnu_manifest.gnu_ordering = true;
    
    let a = Version::from_manifest("1.3", &gnu_manifest).unwrap();
    let b = Version::from_manifest("1.04", &gnu_manifest).unwrap();
    let c = Version::from_manifest("1.4", &gnu_manifest).unwrap();
    
    assert!(a < b && b < c);
}

I wanted to do the minimum API changes possible so it might be a little bit clunky to work with, sorry for that.

This makes sure GNU ordering is used properly, but I don't know if there are other cases that differ.

Let me know if this works as expected!

[sylvestre]

I tried to use it but I can't because of rust-lang/cargo#11523
(we use workspaces)

[timvisee]

Auch! Replacing the dependency directly, instead of patching, also works.