easybuilders / easybuild-framework

EasyBuild is a software installation framework in Python that allows you to install software in a structured and robust way.
https://easybuild.io
GNU General Public License v2.0
152 stars 202 forks source link

Problem using CPATH in modulefiles overwriting system paths #3331

Open Flamefire opened 4 years ago

Flamefire commented 4 years ago

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 https://github.com/easybuilders/easybuild-easyblocks/pull/2047

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

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: https://github.com/spack/spack/issues/11555 and solved it by using C_INCLUDE_PATH, CPLUS_INCLUDE_PATH, INCLUDE instead: https://github.com/spack/spack/pull/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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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

SimonPinches commented 2 years ago

Setting CPATH is an on-going annoying problem for any modules that provide Fortran modules that are used in other software that is compiled using pkg-config to find their location. Any chance of getting a fix in the next release? E.g. By providing a way to enforce the equivalent of setting CLI option --filter-env-vars=CPATH within EB files themselves?

boegel commented 1 year ago

@SimonPinches Sorry for the radio silence on this.

There's no feature that allows to easily filter $CPATH from the build environment currently, but maybe you can get away for now by using unset CPATH && in prebuildopts (and co, if needed)?

We're discussing to switch away from $CPATH in EasyBuild v5.0...

SimonPinches commented 3 months ago

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 commented 3 months ago

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 commented 1 month ago

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 commented 1 month ago

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 #4645
  2. see ##4655
Flamefire commented 1 month ago

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 commented 1 month ago

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.