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 overhaul #41

Closed
wants to merge 10 commits into from
Closed

CMake overhaul #41

wants to merge 10 commits into from

Conversation

derpda
Copy link

@derpda derpda commented Dec 20, 2020

This is the start of a CMake overhaul for HODLR (see #40)
The goal is to make the library more usable from a user standpoint and to reduce built time.

It would probably be better to also handle the dependencies of HODLR (OpenMP ...) in a more CMake-like fashion.
Mainly, using find_package would make it more clear what dependencies exist and would help depending libraries like the code I'm writing as well.
I haven't made these changes yet though.

I would love any feedback on this and am happy to make/revert any changes.

By not recompiling the same code for each executable we can save time.
Before:
time make -j4: 6m12s
After:
time make -j4 : 1m52s
Default behaviour unchanged.
Again, build time is shortened.
With tests:
time make -j4: 1m52.
Without tests:
tume make j4: 1m19s

The remaining time is mainly caused by Eigen headers, thus unavoidable.
Depending projects need this definition
Better when installing the library - avoid name clashes.
@derpda
Copy link
Author

derpda commented Dec 20, 2020

I see that some CI tests are failing, I'll get that fixed as soon as I can!

Eigen can now be downloaded automatically if none is found.
Other dependencies (pthread, OMP) clearly specified.
MKL dependency also fixed, but remains optional.
@derpda
Copy link
Author

derpda commented Dec 21, 2020

I have made some quite drastic changes to the CMake code.
In particular, the way dependencies are handled is cleaned up.
Eigen can be downloaded automatically now if it isn't found (Eigen3_DIR environment variable can be set to point to an Eigen install).
I believe this makes the install.sh script largely obsolete, but I have left it in for now.

@derpda
Copy link
Author

derpda commented Dec 21, 2020

I also just noticed that I added a lot of unintentional whitespace changes when I was modifying the include structure.
Rest assured that I did not actually make any actual changes to the C++ code.

@derpda
Copy link
Author

derpda commented Dec 21, 2020

The test runs now, but fails with

terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string::_M_construct null not valid
/home/travis/.travis/functions: line 109:  5280 Aborted                 (core dumped) ./test/test_HODLR

The same happens on all machines I tested on - I assume it is an issue with the test itself.

@xantares
Copy link
Contributor

xantares commented Mar 7, 2022

that's probably because of HOLDR_PATH:
https://github.com/sivaramambikasaran/HODLR/blob/master/src/HODLR.cpp#L190
just drop the getenv and set it via a cmake compile definition:

target_compile_definition(HODLR PRIVATE HODLR_PATH="${PROJECT_SOURCE_DIR}/src")

or something like that

@derpda derpda closed this Aug 20, 2024
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