-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
gdal: add v3.9.0, fix a CMake bug, bump deps #23233
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valgur It looks good, but I'd like to see any log using those options related to the bumped dependencies (the remaining ones defaulted to False). It's only to verify that we're not introducing any conflicts or raising different problems.
That's almost impossible at the number of dependencies this library supports. Until we have a method of bumping each dependency simultaneously across the whole CCI at least. Other than that, can do. |
@valgur I just tried it. I got some conflicts:
Anyway, applying this diff was enough to avoid it: diff --git a/recipes/gdal/post_3.5.0/conanfile.py b/recipes/gdal/post_3.5.0/conanfile.py
index 7418aa855f..9fb263c22d 100644
--- a/recipes/gdal/post_3.5.0/conanfile.py
+++ b/recipes/gdal/post_3.5.0/conanfile.py
@@ -207,10 +207,10 @@ class GdalConan(ConanFile):
if self.options.with_ecw:
self.requires("libecwj2/3.3")
if self.options.with_expat:
- self.requires("expat/2.6.2")
+ self.requires("expat/2.5.0")
if self.options.with_exr:
self.requires("openexr/3.2.3")
- self.requires("imath/3.1.10")
+ self.requires("imath/3.1.9")
if self.options.with_freexl:
self.requires("freexl/2.0.0")
if self.options.with_geos:
@@ -246,7 +246,7 @@ class GdalConan(ConanFile):
if self.options.with_libkml:
self.requires("libkml/1.3.0")
if self.options.with_lzma:
- self.requires("xz_utils/5.6.1")
+ self.requires("xz_utils/5.4.5")
if self.options.with_lz4:
self.requires("lz4/1.9.4")
if self.options.with_mongocxx:
@@ -295,7 +295,7 @@ class GdalConan(ConanFile):
if self.options.with_xerces:
self.requires("xerces-c/3.2.5")
if self.options.with_xml2:
- self.requires("libxml2/2.12.5")
+ self.requires("libxml2/2.12.4")
if self.options.with_zstd:
self.requires("zstd/1.5.5")
# Use of external shapelib is not recommended and is currently broken. Another related one was:
This one was caused by using |
@franramirez688 Also, That was with static libs. Some dependency broke the shared build pretty badly, though. Edit: this was caused by |
This comment has been minimized.
This comment has been minimized.
Hi @valgur - thanks for this PR. In recent weeks we have taken the decision to not merge PRs that bump version dependencies without a motivation for each bump. I'm really happy to hear that you are successfully using overrides to solve version conflicts. However, we keep receiving feedback from users - especially those who are new to Conan Center, conflicts become a problem and it becomes an obstacle. And then of course, the issues that arise in Conan Center CI when a build fails because of pre-existing conflicts that are unrelated to the PR being built. We have noticed in recent months that indiscriminately bumping dependencies tend to introduce conflicts, rather than fix them - and these are conflicts that can only be solved by other bump dependencies in other PRs. We want to minimise the amount of time any conflict is present with the default options, while we work on a more robust solution. The team is currently working on a number of strategies to address this moving forward:
That is, moving forward we want a conflict-free experience for most users (at least those who are using a consistent version set provided by us). This is why you may see the team not prioritising version conflicts, or asking them to be removed from a PR that has other fixes. Note however that we will consider version bumps if this fixes an existing conflict, new functionality from a given library is required, or there is a security issue where the new version proposed already has the required fix. As a note to contributors - given the high load of PRs that we need to review, and bearing any account that bumping dependency versions without justification is not currently prioritised (but by all means, we welcome the PR being open so that we eventually get to it), we would advise PRs that address other issues (e.g. adding a new version, fixing a bug, etc) to be done separately from bumping dependencies without a strong motivator. |
@jcar87 @franramirez688 For the more general point, I agree and I'm very grateful for the effort to stabilize things and move forward in a more systematic and efficient manner. The version conflicts are a major paint point on the consumer side. Probably even more than on CCI. I'll try to revert any non-critical dependency version bumps on my existing PRs and new ones. However, we still need a stable baseline state with consistent versions across CCI before we can sanely talk about mostly freezing dependency versions. To that end, please direct some of that energy towards and prioritize #23277. It should fix a large portion of version conflicts currently (and frequently) encountered on CCI. I think it's also in line with the likely future approach of handling version bumps by individual dependencies at once instead of doing it in arbitrary and inconsistent fashion across random PRs. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
expat 2.5.0 has some CVEs as mentioned in #23277
I opened #23508 which bumps expat in freexl to 2.6.0 which solves the vulnerable situation and limits conflicts with other recipes. |
@mayeut I would move the Expat versions to v2.6.2 as it fixes a CVE as well: |
ok, not reflected yet in repology or #23277 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@franramirez688 The version bumps part of the PR has been reverted and was not the most significant part of the PR anyway. It fixes some major bugs in the recipe, plus adds a newer version, so a second look (and hopefully a fix for the missing geotiff binary) would be welcome. |
Bumped deps that were previously broken anyway. Bumped vulnerable libarchive version.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 6 (
Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping Failure in build 6 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Could someone please fix the broken binaries. The recipe is broken and needs these CMake fixes to work reliably. Please unblock it. The other changes are trivial and largely irrelevant. The version bumps are motivated by new actually working versions becoming available after the recipes for them have been migrated to Conan 2.x. Feel free to revert any that you don't agree with. I don't really care. Also, the recipe still needs to work around a Conan generators bug via patching, which would also benefit from some attention at some point: |
Are the changes in a0480a2 motivated by a bug? I would avoid changes like these if that's not the case. |
Mostly to follow the CMake/Conan conventions: #25381 (comment) Also, give me a break, please. 😁 This PR, which has been open since March, proposes a fix for an actual major bug that currently breaks a significant portion of dependency handling in the CMake configure step. The |
I see - that was in response to the addition of |
The custom
find_package2()
failed to set non-capitalized dependency vars (e.g.PostgreSQL_LIBRARIES
) in a globally visible manner. Fixed this.