search

rcsb / mmtf-cpp

The pure C++ implementation of the MMTF API, decoder and encoder.
MIT License
21 stars 24 forks source link

Updated Travis CI and added example-test with C++03 #27

Closed gtauriello closed 5 years ago

gtauriello commented 5 years ago

I updated the Travis CI since I had noticed some issues while reviewing the last PR.

I included a new test which compiles and runs one of the example codes (read and write) with C++03 wherever possible. I didn't manage to write a script to run it with EMSCRIPTEN but I guess that's ok. The problem so far was that Catch2 requires C++11 and hence cmake automatically switches to C++11. As a result, it was possible for C++11 code to get into the main code without CI noticing it. The new test will lead to failures in such a case (see https://travis-ci.org/gtauriello/mmtf-cpp/builds/506419291 and ignore the EMSCRIPTEN failure there). The Linux tests (apart from EMSCRIPTEN) and macOS with gcc would now fail (macOS clang seems to ignore the C++03 request).

In the process I saw that we use an old macOS image and a deprecated way to install packages (see https://docs.travis-ci.com/user/reference/osx/#homebrew). I updated the Travis config accordingly and updated the GCC version used since Homebrew's gcc-5 has issues with recent macOS versions.

danpf commented 5 years ago

darn this is a bit sad, maybe we could switch to the CATCH1.x branch of catch2?

frodofine commented 5 years ago

Code compiled with EMSCRIPTEN cannot be run with ./ and needs to be run with the node command instead. That being said, I don't think testing for C++03 compliance on this platform is necessary as EMSCRIPTEN is an LLVM back end for code typically compiled with clang. Therefore, if clang works, this should too (barring any processor specific code).

gtauriello commented 5 years ago

@danpf It's ok. I think that if we can compile and run a single test with C++03 that should be enough test coverage for that part. Then in the future, we may want to drop the C++03 requirement eventually so I wouldn't put any extra effort on this. And since we give compile instructions in the README, I thought it would be good to have such a test anyway.

@frodofine Yeah I saw that I can run it with node but it wouldn't let me read in the file (or write out the result). I saw that in the unit tests there's a special main function taking care of that, but it seemed a massive overkill for me to get into that for the purpose of extending CI here. And as you pointed out, it's pointless to check for C++03 compliance for EMSCRIPTEN anyway.

gtauriello commented 5 years ago

I thought I'd give the Emscripten part another shot and it turns out that it's not that hard to make it deal with files. So I added the run command there too.

Now I would need one of you to review and approve those changes before I can merge them.

gtauriello commented 5 years ago

@frodofine @danpf I just noticed that neither of you had write access to this repo and hence also couldn't review code. It was just me and @speleo3 as active contributors with write access. So I changed that now and gave access to both of you.

gtauriello commented 5 years ago

@frodofine or @danpf or @speleo3 Can one of you quickly review (and preferably approve) the changes? Otherwise I cannot merge them. The repo is setup that even admins like myself are not allowed to merge changes unless someone else approved them. I could also relax that of course...