-
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
proj: add v9.4.0, drop old versions #21512
Conversation
valgur
commented
Nov 30, 2023
•
edited
Loading
edited
- Resolves PROJ 9.4.0 #23482.
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.
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.
Sorry I don't see the point of this modification of patches. I prefer to have patches per version instead of an external cmake file injected to some magic line in upstream CMakeLists. It's hard to reason about CMake logic with this kind of external cmake file. Patches are better when fixes can be submitted upstream, because it doesn't require any modification in conanfile when it's fixed upstream. With your pattern, it's worse for maintenance, because we will have to remove all this logic in conanfile conditionally once fixed upstream... |
PROJ will use SQLite::SQLite3 target in next release. that is when the patches can be removed. |
This comment has been minimized.
This comment has been minimized.
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.
Thanks!
Hi @valgur, thanks for your contribution! |
PROJ 9.3.0 and older set
cmake_minimum_required(VERSION 3.9)
which in turn sets CMP0077 to OLD by default. This line ensures that
tc.variables[] works as expected in all cases.
|
This line is very likely useless (or if it is planned to drop this logic
in all recipes, it should live in conan client code I guess, but again it's
useless for properly formed CMakeLists since conan toolchain translates
tc.variables to CACHE variables, therefore CMP0077 doesn't matter).
You are misunderstanding the issue being fixed by CMP0077 in some important
ways. When it’s set to OLD, all set(CACHE) and option() commands in
CMakeLists.txt will overwrite the value previously set by CMakeToolchain,
causing them to be ineffective. There is no such thing as a properly-formed
CMakeLists.txt in this case - only the value of this policy matters.
Believe me, this is a very real issue or at least a potential pitfall in
many recipes. I encourage you to test it yourself, if you’re having doubts.
I have been adding it for all recipes where CMake policy version is
configured to 3.11 or older, just in case. Even if these projects might not
be setting CACHE vars in the current version, they easily can switch to
using them in later versions, which is very easy to miss.
I agree that this would be better handled by the Conan client, but I
suppose setting it by default for all projects might cause unexpected
regressions in unrelated places, making it a bit risky. And even then it
would take a while for the change to become effective for CCI.
…On Thu, Jun 6, 2024 at 21:40 SpaceIm ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In recipes/proj/all/conanfile.py
<#21512 (comment)>
:
> @@ -109,6 +110,7 @@ def generate(self):
# Workaround for: conan-io/conan#13560
libdirs_host = [l for dependency in self.dependencies.host.values() for l in dependency.cpp_info.aggregated_components().libdirs]
tc.variables["CMAKE_BUILD_RPATH"] = ";".join(libdirs_host)
+ tc.cache_variables["CMAKE_POLICY_DEFAULT_CMP0077"] = "NEW"
Looking at conancenter history, I see this policy injected more & more
quite randomly without rational. It's quite misleading.
—
Reply to this email directly, view it on GitHub
<#21512 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANSD4C3IRCNPGG2HWBDGLDZGCUKHAVCNFSM6AAAAABABGW4K2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBSHEZDEMJYGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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.
LGTM
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.
LGTM
Conan v1 pipeline ✔️All green in build 17 (
Conan v2 pipeline ✔️
All green in build 16 (
|