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

CMake improvements #8

Merged
merged 6 commits into from
Sep 29, 2016
Merged

CMake improvements #8

merged 6 commits into from
Sep 29, 2016

Conversation

jschueller
Copy link
Contributor

No description provided.

@m-reuter
Copy link
Owner

m-reuter commented Sep 27, 2016

Thanks. About the examples: There is nothing to build here without examples. The library consists only of headers. We may want to put everything except the install target inside the IF EXAMPLES block? There is no need to check for libraries if EXAMPLES is off.

Also about the install rule for headers. I think we also need the .h files from all the other include directories under examples/matrices example/matprod and examples/areig?

@jschueller
Copy link
Contributor Author

  • Ok for the examples I changed the strategy : they're built by "make examples" and not by default.
  • Aren't the headers in examples/ specific to the examples ?

@m-reuter
Copy link
Owner

The example headers contain some nice utility (e.g. the class Solution to print results, several simple matrices for testing, wrapper functions etc.). I think it makes sense to make them available via install. (Can you do that)? Then the travis file needs to be changed to "make examples" and maybe we want to run "make install" also for testing on travis.
Finally, I plan to switch to explicitly listing each file in CMakeLists.txt instead of the globbing everwhere. That way cmake will know when to re-run (if a file name gets changed or file added/removed). That does not need to be part of this merge, though.

@jschueller
Copy link
Contributor Author

jschueller commented Sep 28, 2016

I updated the Travis file.
You mean install all the headers ?

@m-reuter
Copy link
Owner

m-reuter commented Sep 28, 2016

Yes, would be great.

@jschueller
Copy link
Contributor Author

Ok, done (Travis updated too).

@m-reuter
Copy link
Owner

m-reuter commented Sep 28, 2016

For some reason, the travis mac build still fails. Probably some of the mac specific fortran libraries are not added to the example target?
Also, some systems (like my centos) don't need and don't have the quadmath library, so we should not fail if not found.

@m-reuter
Copy link
Owner

Oh, also about the install naming. At some point we decided to call the project arpackpp, should we install to that dir instead of ++ ? Or do you know of existing dependencies that expect arpack++ ?

@jschueller
Copy link
Contributor Author

I'll try to fix that tomorrow.
About the naming, I mimic Debian which provide them under /usr/include/arpack++:
https://packages.debian.org/jessie/amd64/libarpack++2-dev/filelist
But that doesn't mean it cannot change, names that contains '+' are not always allowed.

@m-reuter
Copy link
Owner

This should fix it:
SET (GFORTRAN_LIB ${GCC_EXT_LIB})
should be
SET (GFORTRAN_LIB ${GFORTRAN_LIB} ${QUADMATH_LIB} ${GCC_EXT_LIB})
thats where the fortran libs get dropped.
Also moving the block with
MESSAGE(STATUS "quadmath is required but could not be found") SET(ABORT_CONFIG TRUE)
back into the apple block should fix old centos. It will show as not found but still continue.

Naming: I'd prefer arpackpp. That will make it clear that this is not the old arpack++ library.

@jschueller jschueller force-pushed the cmake branch 2 times, most recently from 0b2b58d to 5657289 Compare September 29, 2016 06:13
@jschueller jschueller force-pushed the cmake branch 2 times, most recently from 6b30810 to 7a05834 Compare September 29, 2016 06:39
@jschueller
Copy link
Contributor Author

Ok, fixed.
I also installed to include/arpackpp instead of include/arpack++ then.

@m-reuter m-reuter merged commit 2db30c8 into m-reuter:master Sep 29, 2016
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.

2 participants