fix: Use CMake 3.12+ FindPython to get Python components #1906
Conversation
cmake/XRootDSummary.cmake
Outdated
| @@ -12,7 +12,7 @@ component_status( TESTS BUILD_TESTS CPPUNIT_FOUND ) | |||
| component_status( HTTP BUILD_HTTP OPENSSL_FOUND ) | |||
| component_status( TPC BUILD_TPC CURL_FOUND ) | |||
| component_status( MACAROONS BUILD_MACAROONS MACAROONS_FOUND ) | |||
| component_status( PYTHON BUILD_PYTHON PYTHON_FOUND ) | |||
| component_status( PYTHON BUILD_PYTHON Python_FOUND ) | |||
There was a problem hiding this comment.
This should probably match the other check, i.e.
Python_Interpreter_FOUND AND Python_Development_FOUND
There was a problem hiding this comment.
@amadio I updated this from Python_FOUND to Python_Interpreter_FOUND AND Python_Development_FOUND as you suggested though note from https://cmake.org/cmake/help/v3.12/module/FindPython.html#result-variables that Python_FOUND is a boolean of if
System has the Python requested components
and so would be more robust to changes in the associated find_package command.
matthewfeickert
force-pushed
the
fix/use-modern-find-package
branch
from
4df838c
to
40bb469
Compare
|
I have very limited experience doing anything with errors from rpm-fedora CI logs:I guess the real issue is xrootd/src/XrdCms/XrdCmsRRQ.cc Line 370 in ca73565 though I'm not sure if there's a way to change these build defaults to turn off Line 4 in ca73565 and so I assume you don't care. |
|
the problem comes from usage of |
Yeah, that was clear. Sorry I should be more specific. As |
|
Sorry @matthewfeickert i spoke to soon without proper digging .. |
|
So, i can reproduce this with the exact commands from workflow, but the really weird thing is that both the error location and what i think is the actual flag that enable errors on warnings ( |
|
Nice! Thanks for catching all this already @adriansev. With permalinks for posterity:
xrootd/packaging/rhel/xrootd.spec.in Lines 511 to 512 in ca73565 xrootd/packaging/rhel/xrootd.spec.in Lines 551 to 552 in ca73565
xrootd/src/XrdCms/XrdCmsRRQ.cc Line 363 in ca73565
|
matthewfeickert
force-pushed
the
fix/use-modern-find-package
branch
from
40bb469
to
c31ffee
Compare
|
@adriansev Can you also give this PR a test run through your workflows? |
|
@matthewfeickert yes, i can confirm the successful execution of rpm-fedora workflow. Thanks a lot! |
|
This is all good, especially if it fixes #1474. However, if we're going to move CMake minimum requirements up, I'd rather push it to 3.16 instead of 3.12, because there are important fixes in the new BTW, please add "Fixes: #1474" somewhere in your last commit message so the issue gets automatically closed when this is merged. Thanks. |
matthewfeickert
force-pushed
the
fix/use-modern-find-package
branch
from
c31ffee
to
b93fe3b
Compare
matthewfeickert
changed the title
@amadio Sounds great and done! 👍 I've rebased and pushed these changes.
I've been doing this already in the PR body so that things are cleanly linked back rather than from some commit that might never actually make it into a deploy branch. "Resolves" is in the word list of keywords that can close Issues (c.f. https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) so in the PR body when I start it with
it sets up the close on merge just as "fixes" would do:
|
|
I didn't notice the "Resolves #1474" in the description of the PR before, that will do just fine, no problem. |
| @@ -76,7 +76,7 @@ jobs: | |||
| -DCMAKE_CXX_STANDARD=17 \ | |||
| -DCMAKE_BUILD_TYPE=RelWithDebInfo \ | |||
| -DCMAKE_INSTALL_PREFIX=/usr \ | |||
| -DPYTHON_EXECUTABLE=$(command -v python3) \ | |||
| -DPython_EXECUTABLE=$(command -v python3) \ | |||
There was a problem hiding this comment.
I think these could actually be removed, as CMake will find /usr/bin/python3 just fine, but I can do this later.
* Require CMake 3.16 at minimum to allow use of modern find_package from CMake 3.12+ as well as fixes in FindPython from CMake 3.16+ for future planned work. - c.f. https://cmake.org/cmake/help/v3.16/command/find_package.html * Set range of valid CMake versions to allow for more modern CMakes to be used if available. - c.f. https://cliutils.gitlab.io/modern-cmake/chapters/basics.html
* Use FindPython from CMake 3.12+ which relies on find_package advancements to select the components that are required. Select the 'Interpreter' and 'Development' components to find the required executables and files. - c.f. https://cmake.org/cmake/help/v3.16/module/FindPython.html * Modern FindPython is capitalization sensitive and will set the following variables that are used throughout the project. * Python_FOUND * Python_Interpreter_FOUND * Python_EXECUTABLE * Python_Development_FOUND - c.f. https://cmake.org/cmake/help/v3.12/module/FindPython.html#result-variables
matthewfeickert
force-pushed
the
fix/use-modern-find-package
branch
from
b93fe3b
to
980f63b
Compare
|
Sorry, final force-with-lease push was just to match the commit naming scheme with prefixing them with |
|
Not a big deal, the CI can handle it, I'm doing many such force pushes myself on the musl PR. In any case, thanks for following the conventions. |
|
@raymondEhlers Quick note that, as this didn't make it into the |
|
This is a new feature, so it will go into 5.6 (there is a change in behavior relative to the previous module). |
There is no change in functionality beyond a bug fix. There is a change in the minimum required CMake version, but that doesn't change the functionality from a user's perspective and realistically isn't affecting any distributions as the CI is passing with no additional changes. I'm not sure what you think the feature is. |
Thanks for the fix and for the heads up! |
Resolves #1474
Python_FOUNDPython_Interpreter_FOUNDPython_EXECUTABLEPython_Development_FOUNDDemonstration of fixing Issue 1474: