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

Problem using CPATH in modulefiles overwriting system paths #3331

Open
Flamefire opened this issue May 8, 2020 · 13 comments
Open

Problem using CPATH in modulefiles overwriting system paths #3331

Flamefire opened this issue May 8, 2020 · 13 comments
Assignees
Labels
change request EasyBuild-5.0-blocker Blocker for EasyBuild 5.0 EasyBuild-5.0 EasyBuild 5.0
Milestone

Comments

@Flamefire
Copy link
Contributor

TLDR: Stop setting CPATH in module files and set C_INCLUDE_PATH, CPLUS_INCLUDE_PATH, INCLUDE instead

This came up for ROOT and was work-arounded in easybuilders/easybuild-easyblocks#2047

Basically: the C/C++ preprocessor considers the paths in the following order:

  • -I
  • CPATH
  • -isystem
  • CPLUS_INCLUDE_PATH
  • buildins

Now projects adding paths via -isystem have those ignored if the same folder/file name exists in CPATH which happened for ROOT and LLVM resulting in taking an incompatible, EB installed LLVM over the included LLVM

Talked to a Spack guy about the CPATH issue: They had the same: spack/spack#11555 and solved it by using C_INCLUDE_PATH, CPLUS_INCLUDE_PATH, INCLUDE instead: spack/spack#14749
So I guess we can and should be doing the same. For existing modules we could simply move the contents of CPATH to the other 3 variables after loading a module
Obvious downside: 3 variables with duplicated content instead of 1.

From Bennet Fauber:
https://gcc.gnu.org/onlinedocs/gcc/Environment-Variables.html
Lists the variables for GNU Cs.

Some additional environment variables affect the behavior of the preprocessor.
CPATH
C_INCLUDE_PATH
CPLUS_INCLUDE_PATH
OBJC_INCLUDE_PATH
CPATH specifies a list of directories to be searched as if specified with -I, but after any paths given with -I options on the command line. This environment variable is used regardless of which language is being preprocessed.
The remaining environment variables apply only when preprocessing the particular language indicated. Each specifies a list of directories to be searched as if specified with -isystem, but after any paths given with -isystem options on the command line.

CPLUS_INCLUDE_PATH is particularly useful for --std=c++99 and what have you that are specific to C++. I am blanking on which it was, but one of the geographic libraries was using CFLAGS for that, and it wasn't working well....

@boegel
Copy link
Member

boegel commented Aug 27, 2020

@Flamefire Do you think there's a scenario where no longer setting $CPATH could cause trouble?
Incorrect paths being passed to -isystem (like /usr/include), for example?

I'm a bit reluctant to make a change like this in EasyBuild 4.x, it feels like giving up some control (having $CPATH win over -isystem can be seen as a good thing).

Maybe problems that pop up because of this should be dealt with on a per case basis, like we did for ROOT?

@bsteinb Any input on this (since you referenced this issue after hitting a problem at JSC related to this)?

@Flamefire
Copy link
Contributor Author

Incorrect paths being passed to -isystem (like /usr/include), for example?

having $CPATH win over -isystem can be seen as a good thing

Incorrect things are what we have to deal with anyway. If some software explicitly passes something to the compiler this should win. Us "injecting" unexpected stuff is what I would consider incorrect (which is the reason for this issue anyway)

Maybe problems that pop up because of this should be dealt with on a per case basis, like we did for ROOT?

I'd say this will be hard. Because most often it will "just work". I.e. the application requests some other path (e.g. a local directory) but the system/EB path is taken instead and it happens to work because the API is "compatible enough". But at ABI level it might break in silent ways. So we'll only notice problem when it breaks loudly at build time.

IMO we have some kind of assurance that this change works well: Because Spack already uses it.

@ocaisa
Copy link
Member

ocaisa commented Aug 27, 2020

To maintain backwards compatibility we can just introduce an option to choose one or the other approach, for v4 the default is to use CPATH, for v5 we switch the default (and for maintainers we can recommend to switch already so that we gain plenty of experience with the change)

@SimonPinches
Copy link

This is just to add a comment that we are also facing problems associated with the setting of CPATH in modules. The problem arises for Fortran codes and the use of pkg-config. New versions of pkg-config unhelpfully strip out paths they find in CPATH from what they return when run with --cflags. Since compilers like gfortran do not search CPATH for Fortran modules, they are now not found :-(

@SimonPinches
Copy link

Is there a way to do the equivalent of passing --filter-env-vars=CPATH on the CLI within an EB file?

@boegel boegel modified the milestones: 4.5.0, release after 4.5.0 Oct 13, 2021
@boegel boegel modified the milestones: 4.5.1, release after 4.5.1 Dec 7, 2021
@boegel boegel modified the milestones: 4.5.2, release after 4.5.2 Jan 14, 2022
@SimonPinches
Copy link

This is still an on-going problem for our Fortran codes. I've just had to manually strip the setting of CPATH from all our new installations of netCDF-Fortran because I forgot to install with --filter-env-vars=CPATH. This is a recurrent problem which I hope will be fixed in EasyBuild 5.0 or perhaps even sooner...

@Flamefire
Copy link
Contributor Author

To maintain backwards compatibility we can just introduce an option to choose one or the other approach, for v4 the default is to use CPATH, for v5 we switch the default (and for maintainers we can recommend to switch already so that we gain plenty of experience with the change)

This would be possible

Alternative for admins: module_write hook to replace or remove CPATH

@boegel
Copy link
Member

boegel commented Sep 9, 2024

This could be guarded behind an EasyBuild configuration option like --c-cxx-header-path, with CPATH as default, and INCLUDE_PATH as alternative which makes it update C_INCLUDE_PATH, CPLUS_INCLUDE_PATH and INCLUDE instead of CPATH.

@lexming
Copy link
Contributor

lexming commented Sep 18, 2024

Summary of maintainers meeting

There are 2 dimensions to this issue:

  1. what EB sets in the environment at build time: currently all toolchains add to CPPFLAGS the paths to all dependencies (1st order), which translates to -I/path/to/include options added to compilation command
  2. what modules set in the environment at load time: currently modules are generated with a CPATH variable, their include directory is appended to CPATH environment variable on load

This means that builds carried out by EB mainly rely on -I options set by CPPFLAGS on-the-fly, and the CPATH from the modules is only used for second-order dependencies (or if they point to some missing header anywhere else). On the other hand, users compiling stuff on their own (without EB) fully depend on CPATH set in the modules.

We will add options to change how EB behaves in both situations:

  1. see add option search-path-cpp-headers to control how EasyBuild sets paths to headers at build time #4645
  2. see #add option module-search-path-headers to control how modules set search paths to header files #4655

@Flamefire
Copy link
Contributor Author

This doesn't seem to fully solve the initial problem:

Unconditionally setting -I leads to the same problem of software not using the intended path added via -isystem because -I takes precedence. So maybe the default for the new option should be setting the path variables instead.

On the other hand, users compiling stuff on their own (without EB) fully depend on CPATH set in the modules.

Yes and the other variables should work the same way if we set them instead of CPATH, won't they?

@lexming
Copy link
Contributor

lexming commented Sep 19, 2024

We do not plan to change the default. This is not a widespread issue. Software needing this specific setup will be handled on a case-per-case basis through toolchainopts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change request EasyBuild-5.0-blocker Blocker for EasyBuild 5.0 EasyBuild-5.0 EasyBuild 5.0
Projects
Status: Breaking changes
Development

No branches or pull requests

5 participants