conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.17k stars 974 forks source link

[bug] Unexpected gtest pattern match #14228

Open jsallay opened 1 year ago

jsallay commented 1 year ago

Environment details

Steps to reproduce

Run conan install --tool-requires "gtest/[>=1.13.0]"

This matches against the gtest/cci.20210126 package even though this is older than 1.13.0. It looks like pattern matching works strictly on alphabetical order. I believe that this means that any version that starts with a letter will always be considered newer than any semver versioned package.

I know that you can't magically know that 1.13.0 is newer than the other version, but maybe the >= could only apply to semver style version names (or at least not mixing the search between versions that start with numbers vs letters)

Logs

conan install --tool-requires "gtest/[>=1.13.0]"

======== Input profiles ========
Profile host:
[settings]
arch=x86_64
build_type=Release
compiler=gcc
compiler.cppstd=gnu17
compiler.libcxx=libstdc++11
compiler.version=11
os=Linux
os.distro=Ubuntu22.04
[conf]
tools.system.package_manager:mode=install
tools.system.package_manager:sudo=True
tools.build:jobs=6

Profile build:
[settings]
arch=x86_64
build_type=Release
compiler=gcc
compiler.cppstd=gnu17
compiler.libcxx=libstdc++11
compiler.version=11
os=Linux
os.distro=Ubuntu22.04
[conf]
tools.system.package_manager:mode=install
tools.system.package_manager:sudo=True
tools.build:jobs=6

======== Computing dependency graph ========
Graph root
    cli
Build requirements
    gtest/cci.20210126#dafbdf84b58cd687075ace7314651c1a - Cache
Resolved version ranges
    gtest/[>=1.13.0]: gtest/cci.20210126

======== Computing necessary packages ========
gtest/cci.20210126: Checking 9 compatible configurations:
gtest/cci.20210126: '56a52efa35140eb5207ec825a0bb891b949cb615': compiler.cppstd=11
gtest/cci.20210126: '5b58b26bab3b604200b3120eac772627b2d8c6b9': compiler.cppstd=gnu11
gtest/cci.20210126: 'd85bc90be65eb77509ff219bf6fa4d97cfbb5c39': compiler.cppstd=14
gtest/cci.20210126: '068a098979e0fbce80ada5dd1bef5f4ba13d93ef': compiler.cppstd=gnu14
gtest/cci.20210126: '5005402964adccc23074a5ec72c621050a153327': compiler.cppstd=17
memsharded commented 1 year ago

Hi @jsallay

thanks very much for your report.

I think we need to discuss this behavior, if a bug or a feature. As Conan version range resolution is actually a kind of extension over semver this behavior of comparing letters with numbers is not defined in semver. Conan allows it, following alphanumeric ordering (then letters go after numbers, they are considered newer), and that seems consistent and good. Think for example of versions like 1.1.1, 1.1.9, 1.1.a, I think most people would expect that sequence to be correctly ordered and the a version come after the 1-9 ones.

I know that you can't magically know that 1.13.0 is newer than the other version, but maybe the >= could only apply to semver style version names (or at least not mixing the search between versions that start with numbers vs letters)

I am afraid this is not possible, one of the reasons the Conan semver extensions do compare letters with numbers is because it is necessary to do so in many cases that versions exists with letters, like openssl, that were simply ignored in Conan 1.X. Discarding comparisons that not comply strictly with standard semver gets us back to the initial problems.

Probably the right approach is to be able to explicitly exclude cci.... versions? Why not just specifying it explicitly with test_requires("gtest/[>=1.0 <cci]")

jsallay commented 1 year ago

The workaround is sufficient for my needs. Another potential solution would be to remove the cci version from conan center, since it is fairly old. I don't know if you guys have any sort of roll off policy for old packages.

I'll leave the issue open since you said I think we need to discuss this behavior, if a bug or a feature. Feel free to close it is you decide that it's not worth fixing.

memsharded commented 1 year ago

Oh, yes, the cci.xxx version has been removed from the conan-center-index repo some time ago. But the binaries and recipes in ConanCenter actual server are never removed (at least so far, this might change in the future. Most likely we will do a freeze when moving to 2.0-only support in ConanCenter, and that will drop old cci.xxx versions of gtest)

gsantner commented 3 months ago

Hello @memsharded , thanks for your replies on this topic 😄 .

I'm currently adding a version upgrade check for my Conan dependencies using conan graph outdated conanfile.py / conan graph outdated --requires 'gtest/1.14.0'. Here always the cci.20210126 version is suggested as upgrade.

image

After discovering this bug also added suggested version range exclusion. However that excluision is ignored for the outdated check. Seems like it just checks package name and prints what it finds ordered by semver the most recent.

grafik

-> I understand that the upgrade check won't work if it considers the normal version range ("1.14.0") as that always results in the same version again. Is there any way to tell the outdated check to consider a version range which is (not) considered for upgrade? At the end Conan users would want to know when 1.14.1 / 1.15.0 is available to upgrade via Conan.
Removing the cci versions might be one approach but I think Conan needs some way to flag some versions so they are not considered for calculated version ranges (and only get downloaded if somebody specifies that version explicitly). gtest might be a popular case but this issue can just as well happen at any other package.

memsharded commented 3 months ago

Hi @gsantner

Thanks for reporting this. It is true, the issue is that it is impossible to know the right time order when mixing regular semver-like versions and cci.XXX versions.

There are several aspects impacted by this:

I'll check if it is possible to improve the conan graph outdated output.

memsharded commented 3 months ago

To clarify, the issue wouldn't be solved with something like:

conan graph outdated --requires="somepkg/[>1 <cci]"

affecting the output of the command, because even if we applied that filter for somepkg itself, it could have transitive dependencies, and those dependencies could have both semver and cci versions, and that would still potentially report the wrong version.

gsantner commented 3 months ago

Thanks, yes what you said is also what I meant to say albeit I wrote loads of text 😄 .

Is there any way to tell the outdated check to consider a version range which is (not) considered for upgrade?

Maybe a additional optional (default=None) parameter for the requires(), test_requires(), build_requires() and tool_requires() (https://docs.conan.io/2/reference/conanfile/methods/build_requirements.html) functions is a possibilty to put such restrictions

memsharded commented 3 months ago

I have checked this a bit. It is extremely complicated that Conan would be able to improve this, because it is an intrinsic limitation of the versioning approach, the defined cci versions in ConanCenter are not ordered when mixed with semver versions, this is something that cannot be easily overcome.

I am talking with the team to at least have a solution for the future, which could be outlined as:

memsharded commented 3 months ago

Maybe a additional flag for the requires(), test_requires(), build_requires() and tool_requires()

I am afraid this is not possible. The recipe is not even read for these cases, only the current version and the versions existing in the server can be taken into account. Even if the recipe could be read, it is not possible to modify recipes in the past to work retroactively, the conan graph outdated can resolve to older versions and older revisions under several conditions, like using lockfiles or explicitly pinned revisions.

The right solution to this issue is to propose a correct versioning approach, this would solve all the issues at once. It is true that it might take some time for ConanCenter to follow, and the legacy versions might be an issue as they will not be easily removed, but at least it won't happen for new cases.

gsantner commented 3 months ago

Thanks, just as additional thought:
When that exclusion would see light, you can also specify i.e. gtest/1.14.0 as require, and gtest/<2.0.0 as update hint..and then you get the info that 1.15.0 is available, also when 2.1.0 would be available too by now. So it's not only possible to filter out "bad versions" by this, but also to exclude newer major versions - while still having a specific version locked (if you specify <2.0.0 directly it would upgrade without users immediate knowledge at future builds).

memsharded commented 3 months ago

When that exclusion would see light, you can also specify i.e. gtest/1.14.0 as require, and gtest/<2.0.0 as update hint..and then you get the info that 1.15.0 is available,

We tried this with one of the first initial implementations of the conan graph outdated command, but it resulted in a bit too much complexity for the value. We might want to revisit this possibility again, but in our initial iterations we realized that the biggest value of this command is actually showing possible updates outside of the defined version ranges. Because checking possible updates within the ranges can be typically done with conan graph info --lockfile="" --update (making sure not to lock version), and that returns the new versions that can be used for an update. It is not as ergonomic as the conan graph outdated for information, but the updates within ranges are typically more integrated in the flow, while the thing that was really inconvenient was reporting the new versions outside of the defined ranges, and conan graph outdated focused on that use case.

The command is pretty new, we will most likely revisit it when it is more widely used and we see more repeated usage patterns and needs. At the moment it is a bit too early, as said above, it was a bit too much complexity for the value, so we will wait a bit until getting more experience and users feedback.

Thanks for the feedback!