grimme-lab / mctc-lib

Modular computation tool chain library
https://grimme-lab.github.io/mctc-lib
Apache License 2.0
15 stars 17 forks source link

Remove compiler ID from include directrory #55

Closed yurivict closed 2 years ago

yurivict commented 2 years ago

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

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[bot] commented 2 years ago

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

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

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

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

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

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

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

yurivict commented 2 years ago

Ok, I updated the PR.