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 build system updates #1823

Merged
merged 10 commits into from
Nov 11, 2022
Merged

CMake build system updates #1823

merged 10 commits into from
Nov 11, 2022

Conversation

amadio
Copy link
Member

@amadio amadio commented Nov 8, 2022

These are mostly fixes for warnings with newer versions of CMake. I'm also picking the fix for #1804 to apply onto master.

This does not seem to be necessary. Eventually CMake will remove
the old behavior, so it's better to switch to the new behavior.
See "cmake --help-policy CMP0054" for information on this policy.
CMake generates a warning when a find module like Find<package>.cmake
uses something other than <package> (case-sensitive) in the call to
find_package_handle_standard_args.
XRootD's FindOpenSSL.cmake fails when OpenSSL is not found with the
following error:

CMake Error at cmake/FindOpenSSL.cmake:15 (file):
  file STRINGS file
  "/tmp/xrootd/xrootd/OPENSSL_INCLUDE_DIR-NOTFOUND/openssl/opensslv.h" cannot
  be read.
Call Stack (most recent call first):
  cmake/XRootDFindLibs.cmake:44 (find_package)
  CMakeLists.txt:26 (include)

CMake Error at cmake/FindOpenSSL.cmake:16 (string):
  string sub-command REGEX, mode REPLACE needs at least 6 arguments total to
  command.
Call Stack (most recent call first):
  cmake/XRootDFindLibs.cmake:44 (find_package)
  CMakeLists.txt:26 (include)

Since FindOpenSSL.cmake is already provided upstream by CMake 3.1, we
can use that instead and drop our custom module.
The egrep command has been deprecated since 2007. Recently
grep started to warn about it, so replace egrep usage with
grep -E. See release notes of grep at the link below for
more information.

https://savannah.gnu.org/forum/forum.php?forum_id=10227
@amadio amadio self-assigned this Nov 8, 2022
Attempting to upgrade it results in the error below.

  Complete output from command python setup.py egg_info:
  Traceback (most recent call last):
    File "<string>", line 1, in <module>
    File "/tmp/pip-build-b24Ztv/wheel/setup.py", line 1
      from __future__ import annotations
  SyntaxError: future feature annotations is not defined
When not setting CMAKE_{C,CXX}_COMPILER explicitly, the default
ones (cc and c++) are picked up, but are not properly setup to
find headers, which causes build failures of the Python bindings.
@amadio amadio force-pushed the cmake branch 2 times, most recently from 710c0c3 to fa7a2e1 Compare November 9, 2022 17:13
This is required as by default the installed Python doesn't know about
XRootD which was installed into /usr/local/lib/python3.X. Using a local
installation we can work around it. Note that we cannot set --user in
PIP_OPTIONS as it conflicts with --prefix, which is added by the build
system in bindings/python/CMakeLists.txt.
cmake/XRootDFindLibs.cmake Show resolved Hide resolved
cmake/FindOpenSSL.cmake Show resolved Hide resolved
cmake/FindOpenSSL.cmake Show resolved Hide resolved
cmake/FindOpenSSL.cmake Show resolved Hide resolved
cmake/FindOpenSSL.cmake Show resolved Hide resolved
cmake/FindOpenSSL.cmake Show resolved Hide resolved
cmake/FindOpenSSL.cmake Show resolved Hide resolved
cmake/FindOpenSSL.cmake Show resolved Hide resolved
This ensures that the right function is used with both shared and static
libraries, as CMake's check_symbol_exists() doesn't work with static
libraries. The release notes for OpenSSL 1.1.0 mention the deprecation
of the old function and recommends using the new TLS_method(). More
information at https://www.openssl.org/news/cl111.txt.
@amadio
Copy link
Member Author

amadio commented Nov 11, 2022

Turned out that removing the #ifdef altogether was a bit too optimistic, so I replaced it with a versioned #ifdef instead to keep it working with both shared and static libraries of OpenSSL.

@simonmichal We could remove the #ifdef and keep using only the older SSLv23_method name, as that's a macro that defines it as TLS_method anyway in newer OpenSSL (and why we didn't see any problems when not adding -DHAVE_TLS before. Let me know if you prefer this, I did the #ifdef to ensure the current version is future-proof. We can drop the #ifdef when we move to require a newer version of OpenSSL (i.e. when CentOS 7 support is dropped).

@simonmichal
Copy link
Contributor

Look's good, again thanks for the PR :-)

@simonmichal simonmichal merged commit 4cd7be5 into xrootd:master Nov 11, 2022
@amadio amadio deleted the cmake branch November 11, 2022 13:31
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

2 participants