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

Fix find_headers to also look for C++ headers and Fortran modules #10798

Merged
merged 1 commit into from
Mar 9, 2019

Conversation

michaelkuhn
Copy link
Member

@michaelkuhn michaelkuhn commented Mar 5, 2019

This is likely caused by #10623. Currently, only C headers are considered, causing build failures for packages depending on, e.g., netcdf-fortran and xerces-c. Additionally, the regex used to look for the include path component did not consider word boundaries, causing false matches.

@alalazo
Copy link
Member

alalazo commented Mar 5, 2019

An alternative approach for the netcdf-fortran problem would be to extend the list of suffixes considered in find_headers.

I'd like that better, as there could be more Fortran packages suffering from this. Curious what @adamjstewart and @scheibelp think about that.

@scheibelp
Copy link
Member

@alalazo is this expected? I would think that the logic of #10623 would add <prefix>/include if any subdirectory in <prefix>/include contained a header file.

@michaelkuhn could you add the error message here (or point me to the relevant issue)?

@michaelkuhn
Copy link
Member Author

@michaelkuhn could you add the error message here (or point me to the relevant issue)?

Sorry, I did not capture the error output. esmf failed to build because the compiler could not find netcdf.mod and the xerces header. I then checked the build environment and saw that SPACK_INCLUDE_DIRS did not contain the prefix/include for these two packages. I can reproduce it if you want.

@alalazo
Copy link
Member

alalazo commented Mar 7, 2019

@scheibelp

is this expected? I would think that the logic of #10623 would add /include if any subdirectory in /include contained a header file.

It's expected in the sense that find_headers (not modified in #10623) will recognize as headers only *.h files, while as far as I get from the comment here the directory only contains Fortran *.mod files. My suggestion was to add *.mod to the list of files recognized as headers as this will possibly solve the same issue in other similar packages.

@scheibelp
Copy link
Member

It's expected in the sense that find_headers (not modified in #10623) will recognize as headers only *.h files, while as far as I get from the comment here the directory only contains Fortran *.mod files.

OK thanks I did not realize that. So

An alternative approach for the netcdf-fortran problem would be to extend the list of suffixes considered in find_headers.

I'd like that better, as there could be more Fortran packages suffering from this. Curious what @scheibelp think about that.

That sounds good

Currently, only C headers are considered, causing build failures for
packages depending on, e.g., netcdf-fortran and xerces-c. Additionally,
the regex used to look for the include path component did not consider
word boundaries, causing false matches.
@michaelkuhn
Copy link
Member Author

I have pushed an updated version that now makes both package-specific changes unnecessary. I missed that xerces-c ships C++ headers the first time around and have now updated find_headers to also look for .hpp headers.

@michaelkuhn michaelkuhn changed the title netcdf-fortran, xerces-c: Add headers property Fix find_headers to also look for C++ headers and Fortran modules Mar 8, 2019
@scheibelp scheibelp merged commit a1c91f3 into spack:develop Mar 9, 2019
@scheibelp
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants