Add CTest support #310
Merged
Add CTest support #310
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
April 7, 2020 14:46
Compilers other than the Intel compiler also do not like some flags. So it is better to whitelist rather than blacklist.
We need to set the CC/CXX environment variables to pick up newer GCC/Clang (and not the system GCC) anyway, so make this consistent for the Intel compiler as well (CC=icc CXX=icpc).
dmed256
requested changes
Apr 8, 2020
Member
dmed256
left a comment
dmed256
left a comment
There was a problem hiding this comment.
I left a few comments but overall it looks pretty good! Thanks for heavily updating the CMake build!
Contributor
Author
|
Thanks for the quick review. I'll get on the requested changes today. |
added 5 commits
April 9, 2020 12:33
Add all tests currently called from the run_* scripts in the tests directory to CMake, so they can be built automatically and run using 'ctest' in the build directory. Set ENABLE_TESTS=ON to enable this. This also changes some paths so that all build artefacts end up inside the build directory, and no longer pollute the source tree.
There is now a CMakeLists.txt, so one more file in the directory.
PWD is not always the same as current working directory. This is a problem when calling tests from ctest: the working directory is changed, but the value of env::PWD is then incorrect. Kernel files are then not found. Using this system call the directory is correctly returned.
Some tests override the OCCA_CACHE_DIR environment variable. This leads to artefacts (e.g. lock files) in the source tree instead of the build tree. In this way we let CMake take care of the correct paths.
Clang also defines __GNUC__, so it would be incorrectly identified as GNU before reaching the check for __clang__.
Contributor
Author
|
I've addressed your concerns I hope and merged the changes into the relevant commits. Currently running tests locally before updating this PR. Could the Travis pipeline be expanded in the future to also test the CMake/CTest path? |
Contributor
Author
|
Updated my branch; works on my end :) |
dmed256
approved these changes
Apr 9, 2020
Member
|
@SFrijters Awesome, thanks for the quick fixes! I'll try running the tests tonight and merge if they pass. Not sure why Travis didn't run them for this PR :( |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Attempts to close #309 .
This also includes some general improvements to the CMake files, and some fixes that were exposed during the implementation and testing of the CTest.
One bugfix that may need discussion in particular is db7e8e4.
Please let me know if you want me to split things up differently.
EDIT: updated commit hash after rebase