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

patch CMake's UnixPaths.cmake script if --sysroot is set #2248

Merged
merged 6 commits into from
Jul 6, 2023

Conversation

boegel
Copy link
Member

@boegel boegel commented Nov 22, 2020

Alternative to #2247, which isn't working out well...

@bartoldeman Is this more or less what you had in mind?

Copy link
Contributor

@bartoldeman bartoldeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try out this block tomorrow morning and see if it's working ok.

@@ -95,6 +130,7 @@ def configure_step(self):
self.log.info("Found sysroot '%s', adding it to $CMAKE_PREFIX_PATH and $CMAKE_LIBRARY_PATH", sysroot)
cmake_prefix_path.append(sysroot)
cmake_library_path.append(os.path.join(sysroot, 'usr', 'lib'))
cmake_library_path.append(os.path.join(sysroot, 'usr', 'lib64'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this chunk is still needed at all: if CMake is patched itself do you still need those environment variables set?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I believe this chunk should be removed since these directories are already searched via the compiled-in CMAKE_SYSTEM_PREFIX_PATH.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should actually never be required. CMake already reroots paths when the SYSPATH or FIND_ROOT_PATH is passed

if sysroot:
# prepend custom sysroot to all hardcoded paths like /lib and /usr
# see also patch applied by Gentoo:
# https://gitweb.gentoo.org/repo/gentoo.git/tree/dev-util/cmake/files/cmake-3.14.0_rc3-prefix-dirs.patch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many small differences versus this patch. Those may not matter though but we should carefully check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked and all the small differences look inconsequential. Here '-' is the Gentoo-patched cmake and '+' the easyblock patch:

 list(APPEND CMAKE_SYSTEM_PREFIX_PATH
   # Standard
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/local /cvmfs/soft.computecanada.ca/gentoo/2020/usr /cvmfs/soft.computecanada.ca/gentoo/2020/
+  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/local /cvmfs/soft.computecanada.ca/gentoo/2020/usr /cvmfs/soft.computecanada.ca/gentoo/2020

this is just a slash

 # Non "standard" but common install prefixes
 list(APPEND CMAKE_SYSTEM_PREFIX_PATH
-  /usr/X11R6
-  /usr/pkg
-  /opt
+  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/X11R6
+  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/pkg
+  /cvmfs/soft.computecanada.ca/gentoo/2020/opt

these don't exist in the prefix, and most likely /opt/bin, /opt/include aren't used either so this looks harmless.

-list(APPEND CMAKE_SYSTEM_LIBRARY_PATH
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/lib//gcc
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/lib/
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib64
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/libx32
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib32
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib
-  /cvmfs/soft.computecanada.ca/gentoo/2020/lib
-  )
-
-list(APPEND CMAKE_SYSTEM_PROGRAM_PATH
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/bin
-  /cvmfs/soft.computecanada.ca/gentoo/2020/bin
+list(APPEND CMAKE_SYSTEM_LIBRARY_PATH /cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib64 /cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib /cvmfs/soft.computecanada.ca/gentoo/2020/lib64 /cvmfs/soft.computecanada.ca/gentoo/2020/lib
+  # X11
+ 
   )

The GCC paths are Gentoo GCC so not relevant to EB GCC.
The X11 path is a historical artifact, it's no longer relevant.
Appending to CMAKE_SYSTEM_PROGRAM_PATH on the Gentoo side looks silly since this defaults to CMAKE_SYSTEM_PREFIX_PATH entries suffixed with /bin already.

 list(APPEND CMAKE_PLATFORM_IMPLICIT_LINK_DIRECTORIES
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/lib//gcc
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/lib/
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib64
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/libx32
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib32
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib
-  /cvmfs/soft.computecanada.ca/gentoo/2020/lib
+  /cvmfs/soft.computecanada.ca/gentoo/2020/lib /cvmfs/soft.computecanada.ca/gentoo/2020/lib32 /cvmfs/soft.computecanada.ca/gentoo/2020/lib64 /cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib /cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib32 /cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib64
   )

same story as above for GCC

@@ -92,11 +77,11 @@
 # parsing the implicit directory information from compiler output.
 set(_CMAKE_C_IMPLICIT_INCLUDE_DIRECTORIES_INIT
   ${CMAKE_C_IMPLICIT_INCLUDE_DIRECTORIES}
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/include
+  "${_cmake_sysroot_compile}/usr/include"
   )
 set(_CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES_INIT
   ${CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES}
-  /cvmfs/soft.computecanada.ca/gentoo/2020/usr/include
+  "${_cmake_sysroot_compile}/usr/include"
   )
 set(_CMAKE_CUDA_IMPLICIT_INCLUDE_DIRECTORIES_INIT
   ${CMAKE_CUDA_IMPLICIT_INCLUDE_DIRECTORIES}

not sure why Gentoo patches this. The implicit include directories are ok when they are initialized from the compiler output at CMake-compile-time, so we should not worry about this bit.

@Flamefire
Copy link
Contributor

I don't think this is a good idea. Using sysroot from a toolchain file sounds much much better.
Counter example: Some software does find_library(FOO foo.so PATHS /usr/foo)
This won't be caught be the patch approach but by the sysroot approach
Better without a toolchain file (if --sysroot is not really required):
CMAKE_FIND_ROOT_PATH with CMAKE_FIND_ROOT_PATH_MODE_LIBRARY and the other 2

@bartoldeman
Copy link
Contributor

I may have misunderstood your point here @Flamefire but the question is here if find_library(FOO foo.so PATHS /usr/foo) should look in $EPREFIX/usr/foo or in /usr/foo.

In the EESSI (or CC CVMFS) Gentoo Prefix context the paths are absolute and not auto-prepended with any prefix, i.e. the output of pkg-config and friends includes the prefix, and if you then prepend the prefix to that you get a double prefix.

@Flamefire
Copy link
Contributor

@bartoldeman IIUC the whole point of sysroot is that you use $EPREFIX and not /usr/foo, is this correct?

The CMake find_* commands have logic which will prepend the CMAKE_FIND_ROOT_PATH (or CMAKE_SYSROOT) to ALL paths passed to the find_* command (explicit and implicit via e.g. CMAKE_PREFIX_PATH) And via some variables you can control if the non-prefixed paths should be searched too. This is useful so that find_library(FOO foo.so PATHS /usr/foo) searches $EPREFIX/usr/foo.

I can see that failing though as e.g. modules set CMAKE_PREFIX_PATH and you don't want to search prepended paths here but you want (only) them for the paths passed to PATHS (above) and for CMAKE_SYSTEM_PREFIX_PATH (autogenerated)

This is tough... I guess then this is the right approach to get it right most of the time. Make sure to test this thoroughly with --debug-find

@bartoldeman
Copy link
Contributor

I think we are conflating two uses of sysroot here, one as used in cross-compilation strategies and one used as a prefix in a Gentoo Prefix installation.

I'll do some more research and get back on this.

@boegel
Copy link
Member Author

boegel commented May 31, 2023

@bartoldeman I would like to get back to this, and get it merged ASAP.

Maybe we should set up a quick call to discuss what needs to change here?

We've been using this easyblock in EESSI for a while now, and haven't seen any problems with CMake itself, or stuff that involves CMake in the installation procedure...

Copy link
Contributor

@bartoldeman bartoldeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rereading my comments it's ok, but I would still remove the whole chunk from line 129 to line 136 since it looks superfluous.

sysroot = ...
if sysroot:
    ...

…d $CMAKE_LIBRARY_PATH in configure step of CMake easyblock - no longer needed due to sysroot-aware patching of Modules/Platform/UnixPaths.cmake
@boegel
Copy link
Member Author

boegel commented Jul 6, 2023

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS CMake-3.20.1-GCCcore-10.3.0.eb
  • SUCCESS CMake-3.22.1-GCCcore-11.2.0.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
node3129.skitty.os - Linux Debian GNU/Linux 11 (bullseye), x86_64, Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz (skylake_avx512), Python 3.11.4
See https://gist.github.com/boegel/4af87dbe3320d0100362d626e95f6d65 for a full test report.

edit: tested in EESSI build environment, using --sysroot

@boegel
Copy link
Member Author

boegel commented Jul 6, 2023

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS CMake-3.20.1-GCCcore-10.3.0.eb
  • SUCCESS CMake-3.22.1-GCCcore-11.2.0.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
node3129.skitty.os - Linux RHEL 8.6, x86_64, Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz (skylake_avx512), Python 3.6.8
See https://gist.github.com/boegel/23839eedcef7b0a650c5664fa8ae7134 for a full test report.

Copy link
Contributor

@bartoldeman bartoldeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bartoldeman bartoldeman merged commit fddeab7 into easybuilders:develop Jul 6, 2023
@boegel boegel deleted the patch_cmake_sysroot branch July 6, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix EESSI Related to EESSI project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants