Skip to content

Drop *_INCLUDE_PATH in prefix_inspections:include #21699

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.

Sign up for GitHub

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

Merged
haampie merged 3 commits into from
Feb 23, 2021
Merged

Drop *_INCLUDE_PATH in prefix_inspections:include #21699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c02495
Drop *_INCLUDE_PATH in prefix_inspections:include
haampie Feb 16, 2021
e8eb76c
Drop other variables that have the role of -I and -L, also drop INCLU…
haampie Feb 16, 2021
4ad975f
Fix tests: LIBRARY_PATH is not set anymore
haampie Feb 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions etc/spack/defaults/modules.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,6 @@ modules:
- MANPATH
share/aclocal:
- ACLOCAL_PATH
lib:
- LIBRARY_PATH
lib64:
- LIBRARY_PATH
include:
- C_INCLUDE_PATH
- CPLUS_INCLUDE_PATH
# The INCLUDE env variable specifies paths to look for
# .mod file for Intel Fortran compilers
- INCLUDE
Copy link
Copy Markdown
Contributor

@glennpj glennpj Feb 16, 2021

Choose a reason for hiding this comment

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

Should INCLUDE be kept for Fortran?

Copy link
Copy Markdown
Contributor

@RemiLacroix-IDRIS RemiLacroix-IDRIS Feb 16, 2021

Choose a reason for hiding this comment

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

it's Windows specific as far as I know.

Copy link
Copy Markdown
Contributor

@glennpj glennpj Feb 16, 2021

Choose a reason for hiding this comment

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

I quickly read through #19840 and #14749 because I remember this came up before. The Windows specificity was missed during the CPATH change but if I am understanding correctly, which may not be the case, INCLUDE is used for finding gfortran .mod files. If that is the case, then dropping INCLUDE would break gfortran module finding much like dropping CPATH broke it for ifort.

Copy link
Copy Markdown
Contributor

@RemiLacroix-IDRIS RemiLacroix-IDRIS Feb 16, 2021

Choose a reason for hiding this comment

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

I think gfortran uses INCLUDE, the documentation says it uses the same environment variables as gcc and I couldn't find INCLUDE in the list.

Copy link
Copy Markdown
Member

@adamjstewart adamjstewart Feb 16, 2021

Choose a reason for hiding this comment

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

it's Windows specific as far as I know.

P.S. Windows support is coming soon! So we should keep this even if it's only used on Windows.

Copy link
Copy Markdown
Contributor

@glennpj glennpj Feb 16, 2021

Choose a reason for hiding this comment

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

I think this is the relevant text from the gfortran man page:

-Idir
           These affect interpretation of the "INCLUDE" directive (as well as of the "#include" directive of the cpp
           preprocessor).

           Also note that the general behavior of -I and "INCLUDE" is pretty much the same as of -I with "#include"
           in the cpp preprocessor, with regard to looking for header.gcc files and other such things.

           This path is also used to search for .mod files when previously compiled modules are required by a "USE"
           statement.

Copy link
Copy Markdown
Contributor

@RemiLacroix-IDRIS RemiLacroix-IDRIS Feb 16, 2021

Choose a reason for hiding this comment

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

These affect interpretation of the "INCLUDE" directive

I think they mean the Fortran INCLUDE directive (https://docs.oracle.com/cd/E19957-01/805-4939/6j4m0vna1/index.html).

Copy link
Copy Markdown
Member Author

@haampie haampie Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that part applies to the include directive, not the environment variable. gfortran man pages do not mention an INCLUDE environment variable. ifort 19.1 man pages only mention CPATH (Linux* and macOS*) and GXX_INCLUDE (Linux*) but INCLUDE is never mentioned as an env variable that's used by the compiler, even though others for Windows are listed. And if this really was used on some older version of ifort on Windows, then for consistency across platforms we should probably drop it too?

lib/pkgconfig:
- PKG_CONFIG_PATH
lib64/pkgconfig:
Expand Down
1 change: 0 additions & 1 deletion share/spack/qa/setup-env-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ succeeds spack -m load b
fails spack -m load -l
# test a variable MacOS clears and one it doesn't for recursive loads
contains "export LD_LIBRARY_PATH=$(spack -m location -i a)/lib:$(spack -m location -i b)/lib" spack -m load --sh a
contains "export LIBRARY_PATH=$(spack -m location -i a)/lib:$(spack -m location -i b)/lib" spack -m load --sh a
succeeds spack -m load --only dependencies a
succeeds spack -m load --only package a
fails spack -m load d
Expand Down