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

CMake: avoid imbalanced cmake_policy push/pop if TIFF or CURL dependency cannot be found (fixes #3696) #3697

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

rouault
Copy link
Member

@rouault rouault commented Apr 4, 2023

No description provided.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 5, 2023

Given that CMP0012 came with 2.8.0, I wonder if is this should be carried around at all. It is needed in one of the following cases:

  • Not calling cmake_minimum_required() before project().
  • Actually setting a minimum version less than 2.8.0.
  • Actually setting CMP0012 to OLD.

The first case is a user error. For the other cases, it needs to be decided: supported or not?

When building package which require a lower version but don't explicitly set CMP0012 (second case), I can use -DCMAKE_POLICY_DEFAULT_CMP<NNNN>=NEW.
https://cmake.org/cmake/help/v3.20/variable/CMAKE_POLICY_DEFAULT_CMPNNNN.html

@rouault
Copy link
Member Author

rouault commented Apr 5, 2023

Given that CMP0012 came with 2.8.0, I wonder if is this should be carried around at all.

yeah, I wonder why I introduced that at the beginning, but I assume I hit some error in some config otherwise I wouldn't have never put that, but I don't recall the situation. Hopefully the new code should be safe, even if mostly unnecessary.

@kloczek
Copy link

kloczek commented Apr 10, 2023

I think that it would be good to pysh new release only because thay fix 🤔

@rouault
Copy link
Member Author

rouault commented Apr 10, 2023

The fix here just avoids the somewhat misleading error message about imbalanced cmake_policy. But the real fix is that users need to make sure PROJ dependencies are available. That said, seeing that, I'm a bit ambivalent about the effect of the original change done in #3374 that brought the need for that new requirement, which is useless if libtiff or libcurl are dynamic libraries. But I'm not sure if there's an elegant way of doing that. For Unix, testing for .so or .dylib extensions should be OK. For Windows, the extension is always .lib for both static or dynamic import lib.

@kloczek
Copy link

kloczek commented Apr 10, 2023

Generally speaking this is why I hate cmake dependencies because cmake uses procedural description instead declarative one like pkgconfig.
IMO as OSGeos projests are provifing .pc files all cmake chedks of external libraries PROj and other peojects should be rewtited by rrplace use cmake modules by single lines pkg_search_module() calls.
With that it would be possible to remove tons of (IMO completly useless) cmake code .. all to be KISS pronciple comppliant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants