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

Remove compiler ID from include directrory #55

Merged
merged 2 commits into from
Aug 3, 2022
Merged

Conversation

yurivict
Copy link
Contributor

@yurivict yurivict commented Aug 3, 2022

It adds the compiler ID to the include directory: include/mctc-lib/GNU-11.3.0/

This is very unconventional and unnecessary.

With this patch it now installs headers into include/mctc-lib which
aligns with the way how most projects install their headers.

@awvwgk
Copy link
Member

awvwgk commented Aug 3, 2022

The exact directory doesn't matter, since the user of the library should always query it with pkg-config. Putting the module files in just $CMAKE_INSTALL_INCLUDEDIR/$PROJECT_NAME will result in problems for projects which provide C headers in the same directory because you can now use the header with either $PROJECT_NAME/something.h or something.h.

@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #55 (8d84a92) into main (05caf6c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #55   +/-   ##
=======================================
  Coverage   69.75%   69.75%           
=======================================
  Files          64       64           
  Lines        8496     8496           
  Branches     2534     2534           
=======================================
  Hits         5926     5926           
  Misses        784      784           
  Partials     1786     1786           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@yurivict
Copy link
Contributor Author

yurivict commented Aug 3, 2022

Putting the module files in just $CMAKE_INSTALL_INCLUDEDIR/$PROJECT_NAME will result in problems for projects which provide C headers in the same directory because you can now use the header with either $PROJECT_NAME/something.h or something.h.

This doesn't cause any real problems because projects both would still resolve to the same header. Projects usually define how their headers are included: #include <project-name/header.h> or #include <header.h>.

@awvwgk
Copy link
Member

awvwgk commented Aug 3, 2022

Projects usually define how their headers are included: #include <project-name/header.h> or #include <header.h>.

If the latter becomes possible due to the module install location, it might get used by accident due to pkg-config adding the location to --cflags and result in unexpected breakage. $CMAKE_INSTALL_INCLUDEDIR/$PROJECT_NAME and $CMAKE_INSTALL_INCLUDEDIR are both not a good place to install Fortran module files.

I think Fedora is installing them in $CMAKE_INSTALL_LIBDIR/.../ including at least the build triple and finclude in the path, which might be an alternative.

@yurivict
Copy link
Contributor Author

yurivict commented Aug 3, 2022

But almost no other projects install headers this way and they are all fine.

What is special about mctc-lib that requires its headers to include compiler ID?

@awvwgk
Copy link
Member

awvwgk commented Aug 3, 2022

What is special about mctc-lib that requires its headers to include compiler ID?

It does not, it only requires to not install the module files in a directory which might contain C headers. This project does not have C headers, but other projects which use a similar structure have and would probably be the next to receive a similar patch.

If the compiler ID is the problem, any other subdirectory name can be used, the actual path doesn't matter. With the module files being a build time requirement I wonder why the compiler ID is causing problems in the first place?

But almost no other projects install headers this way and they are all fine.

Packages installing modulefiles into $CMAKE_INSTALL_INCLUDEDIR are usually broken when installed in a system prefix like /usr. NetCDF for example does this, making the pkg-config file unusable for Fortran projects, which is why they provide nc-config as a workaround.

@yurivict
Copy link
Contributor Author

yurivict commented Aug 3, 2022

If the compiler ID is the problem, any other subdirectory name can be used [...]

Fortran headers are already in a subdirectory. This is sufficient.

What other projects would install headers into include/mctc-lib? This is a dedicated directory for this project. Even if someone else would install C/C++ headers there - this would be totally fine.

@awvwgk
Copy link
Member

awvwgk commented Aug 3, 2022

We can split the differences here. I'm happy to accept this patch for installing modules in include/mctc-lib/modules.

It adds the compiler ID to the include directory: include/mctc-lib/GNU-11.3.0/

This is very unconventional and unnecessary.

With this patch it now installs headers into include/mctc-lib/modules.
@yurivict
Copy link
Contributor Author

yurivict commented Aug 3, 2022

Ok, I updated the PR.

@awvwgk awvwgk merged commit cb04ec8 into grimme-lab:main Aug 3, 2022
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.

None yet

2 participants