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

Make rapidjson dependency managed at cmake configure step #13

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

blinard-BIOINFO
Copy link
Member

@blinard-BIOINFO blinard-BIOINFO commented Feb 8, 2024

rapidjson is probably the least common dependency of EPIK.
While a reasonable version of boost will be present on most servers, this library will not.

This PR makes the dependency managed at configure time by cmake.
It just clones a fixed version from its repository and link the headers.
For me, this approach worked well on 2 different INRAE servers. The library was absent from the system and adding it via some environment would have been time-consuming (no root access).

Changes:

  • epik/rapidjson.cmake manages the cloning at configure step.
  • the last line set(RAPIDJSON_INCLUDE_DIRS ${source_dir}/include) makes sure it will be accessible by epik/CMakeLists.txt

I'm just not sure why you used 2 variables on the following lines of this last file, but it did not interfere in my case:

# RapidJSON cmake scripts are different between versions
set(RapidJSON_INCLUDES ${RAPIDJSON_INCLUDE_DIRS} ${RapidJSON_INCLUDE_DIR})

… more portable on different servers (contrary to boost, likely uninstalled)
@nromashchenko
Copy link
Member

Thanks for the request! There are a few things I have to mention:

  1. Cmake variables of Rapidjson are different between different versions. That answers the 2 variables question. As far as I remember I might have even seen 3 different spellings for their variables...
  2. I'd prefer .cmake files to be in the cmake/ directory, as we have it specifically for cmake files.
  3. I had a lot of headache with Rapidjson specifically when writing cmake commands in the way they work on all 4 target OS (two latest ubuntu and macos as in our CI). So keep it in mind that if you tested in on your system and it works, it doesn't mean much. Also notice that we suggest users to install Rapidjson system-wide with their package manager; our CI scripts rely on this too:

" run: brew update && brew install cmake boost zlib rapidjson libomp && pip3 install click "

So far CI is green with this pull request because everything is still tested with the version of rapidjson that is installed by apt/brew, and not the one that you pull from github.

I am also not convinced that this change is required. Please tell me what system/package manager you use where Rapidjson is not present.

@blinard-BIOINFO
Copy link
Member Author

blinard-BIOINFO commented Feb 23, 2024

I just read you clear answer, thanks.
The servers are Red Hat Enterprise Linux 8.8 (Ootpa).

Other point to think : bioconda package.

Right now, the recipe installs the rapidjson conda package, then compiles epik.

Cmake finds the header in the include directory created by this package.
This works fine on their linux and osx images and the packages are passing tests.

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