-
Notifications
You must be signed in to change notification settings - Fork 156
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
dev branch: cmake + catch + travis fix #145
Conversation
@zauguin please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I added some small remarks. Additionally the file cmake/.DS_Store probably does not belong in the repo.
.travis.yml
Outdated
packages: | ||
- gcc-5 | ||
- g++-5 | ||
- libsqlite3-dev | ||
- libsqlcipher-dev | ||
- libboost-all-dev | ||
- cmake-data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cmake depends on cmake-data anyway, so this is redundant. I think we should remove it to keep the list of packages short.
Maybe gcc-5 can be removed too, it's just a dependency of g++-5
.travis.yml
Outdated
addons: | ||
apt: | ||
sources: | ||
- ubuntu-toolchain-r-test | ||
- george-edison55-precise-backports | ||
packages: | ||
- gcc-5 | ||
- g++-5 | ||
- libsqlite3-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake ignores the preinstalled SQLite and downloads and installs it's own version, so there is no point in requiring this.
tests/exception_dont_execute.cc
Outdated
using namespace sqlite; | ||
using namespace std; | ||
|
||
|
||
int main() { | ||
TEST_CASE("Prepered statement will not execute on exceptions", "[prepared_statements]") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in "Prepared"
.ycm_extra_conf.py
Outdated
if compilation_database_folder: | ||
database = ycm_core.CompilationDatabase(compilation_database_folder) | ||
else: | ||
database = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The else block is unreachable
CMakeLists.txt
Outdated
|
||
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_SOURCE_DIR}/cmake") | ||
include("cmake/HunterGate.cmake") | ||
include("cmake/ParseAndAddCatchTests.cmake") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParseAndAddCatchTests tries to parse the source to extract the tests.
This leads to problems with our disabled tests (especially the variant test).
These problems could probably be fixed by using this script from the Catch repo instead.
.travis.yml
Outdated
|
||
before_install: | ||
- export CXX="g++-5" CC="gcc-5" | ||
|
||
script: ./configure && make test && make clean && make LDFLAGS="-lsqlcipher -DENABLE_SQLCIPHER_TESTS" test | ||
script: mkdir build && cd ./build && cmake .. && make && ./tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using make test
instead of ./tests
would be less surprising for newcomers familiar with cmake
.ycm_extra_conf.py
Outdated
'.', | ||
'-isystem', '/usr/local/include', | ||
'-isystem', '/usr/include', | ||
'-I.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current directory is already added on line 21 and 22.
Is there a reason you want to add the |
Thank you for reviewing :-)
I removed the flags you mentioned, feel free to update this file if you are using YCM too. |
No description provided.