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

add option search-path-cpp-headers to control how EasyBuild sets paths to headers at build time #4645

Open
wants to merge 33 commits into
base: 5.0.x
Choose a base branch
from

Conversation

lexming
Copy link
Contributor

@lexming lexming commented Sep 18, 2024

Partial fix for #3331

EasyBuild (at build time) currently handles paths to headers of dependencies provided by EB through CPPFLAGS. The result is that the compilation command gets additional options of the form -I/path/to/include explicitly pointing the compiler to places with extra headers.

The GCC prepocessor also supports the following environment variables that define the search paths for headers, which have lower precedence than -I command line options:

  • CPATH
  • C_INCLUDE_PATH, CPLUS_INCLUDE_PATH, OBJC_INCLUDE_PATH

(see https://gcc.gnu.org/onlinedocs/cpp/Environment-Variables.html#Environment-Variables-1)

This PR adds a new build option and toolchain option called cpp-headers-search-path to control how EasyBuild will handle paths to headers at build time:

  • CPPFLAGS: current approach by setting CPPFLAGS and injecting -I/path/to/include to compilation command
  • CPATH: new approach by adding paths to include directories to CPATH environment variable
  • INCLUDE_PATHS: new approach by adding paths to include directories to C_INCLUDE_PATH, CPLUS_INCLUDE_PATH, OBJC_INCLUDE_PATH independently

Notes:

  • all these options and environment variables are also supported by LLVM and the Intel compilers.
  • option CPPFLAGS stays the default. So this PR does not change current behavior.
  • the toolchain option has precedence over the build option as it is more specific and (probably) easyconfigs setting this option will not work with any other setting.
  • FCC toolchain can now be drastically simplified by leveraging inheritance to the new Toolchain._add_dependency_cpp_headers() method (@migueldiascosta can you please check that this works on your side?)
  • replaced hardcoded settings for LDFLAGS and CPPFLAGS with new Toolchain methods in ACML toolchain
  • added unit test for cpp-headers-search-paths option

@lexming lexming changed the title add option cpp-headers-search-paths to set method to add search paths to CPP add option cpp-headers-search-paths to set method to control paths to headers at build time Sep 18, 2024
@lexming lexming changed the title add option cpp-headers-search-paths to set method to control paths to headers at build time add option cpp-headers-search-paths to control how EasyBuild sets paths to headers at build time Sep 18, 2024
@Flamefire
Copy link
Contributor

What about $INCLUDE for fortran modules? spack/spack#14749 (comment)

For Intel ifort the .mod file search path uses CPATH and INCLUDE

@lexming
Copy link
Contributor Author

lexming commented Sep 19, 2024

@Flamefire that's out of scope of this PR. This handles all supported options by CPP, which does not apply to Fortran. For Fortran we first need to check what is supported by the different Fortran preprocessors (gfortran for GNU and FPP for Intel) and probably it will need a different option in EB.

@lexming
Copy link
Contributor Author

lexming commented Sep 19, 2024

Actually, after checking the docs, it turns out that the Fortran compilers in both GNU and Intel do support the same environment variables defined by CPP. So this PR already applies to Fortran as well:

@Flamefire
Copy link
Contributor

Flamefire commented Sep 19, 2024

Ok, I see. Only potential issue I see is that setting $CPATH here might make Intel-Fortran pick things up it didn't before.
But I guess that was already the case as we put the same paths in CPATH in the module files, don't we?

Also: How are existing entries handled? I.e. if CPATH=/folder1:/folder2 and adding both here (again) would they be duplicated? Would the order get changed?

Edit: That was supposed to be a reply to #4645 (comment) ;-)

@lexming lexming changed the title add option cpp-headers-search-paths to control how EasyBuild sets paths to headers at build time add option search-path-cpp-headers to control how EasyBuild sets paths to headers at build time Sep 24, 2024
test/framework/toolchain.py Outdated Show resolved Hide resolved
easybuild/tools/toolchain/compiler.py Outdated Show resolved Hide resolved
easybuild/tools/toolchain/constants.py Show resolved Hide resolved
easybuild/tools/toolchain/toolchain.py Outdated Show resolved Hide resolved
easybuild/tools/toolchain/toolchain.py Outdated Show resolved Hide resolved
easybuild/tools/toolchain/toolchain.py Outdated Show resolved Hide resolved
test/framework/toolchain.py Outdated Show resolved Hide resolved
test/framework/toolchain.py Show resolved Hide resolved
lexming and others added 3 commits September 26, 2024 10:45
Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
Copy link
Contributor

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

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

Still some refinements of the new functions which I think are required to avoid subtile issues.

easybuild/tools/utilities.py Outdated Show resolved Hide resolved
easybuild/tools/utilities.py Outdated Show resolved Hide resolved
easybuild/tools/utilities.py Outdated Show resolved Hide resolved
easybuild/tools/utilities.py Outdated Show resolved Hide resolved
test/framework/utilities_test.py Outdated Show resolved Hide resolved
lexming and others added 6 commits October 2, 2024 13:10
Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
Copy link
Contributor

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

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

Looks good to me. One (very) minor suggestion where I'm not sure. See the comment.

easybuild/tools/utilities.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Nice-to-have
Development

Successfully merging this pull request may close these issues.

2 participants