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

ThirdParty Libs: IMPORTED #389

Merged
merged 3 commits into from
Dec 1, 2018
Merged

ThirdParty Libs: IMPORTED #389

merged 3 commits into from
Dec 1, 2018

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Nov 8, 2018

CMake thirdParty libraries should be marked as imported. We do so by creating openPMD::thirdparty:: targets that are imported only and depend on find_package or add_subdirectory targets to decouple them.

Imported targets are always SYSTEM libraries: https://cmake.org/cmake/help/v3.8/prop_tgt/NO_SYSTEM_FROM_IMPORTED.html

That allows us to use stricter warnings in our project than third party headers support.

Original issue:
The nlohmann-json library should be marked as SYSTEM linked lib. This will cause it to be linked with -isystem, allowing us to use warnings in openPMD that are stricter than the ones in the dependency (since their headers do not throw warnings anymore).

nlohmann/json#1339

@ax3l
Copy link
Member Author

ax3l commented Nov 8, 2018

oh wait, that's not working with target_link_libraries and target objects...
Probably the dependency needs to be clearly marked as IMPORTED.

Dang, that's a CMake 3.11.0+ feature! Updated accordingly. Maybe even needs 3.11.2+

@ax3l ax3l changed the title JSON: Link as SYSTEM ThirdParty Libs: IMPORTED Nov 8, 2018
@ax3l ax3l added the third party third party libraries that are shipped and/or linked label Nov 8, 2018
The nlohmann-json library should be marked as `SYSTEM` linked lib.
This will cause it to be linked with `-isystem`, allowing us to use
warnings in openPMD that are stricter than the ones in the dependency
(since their headers do not throw warnings anymore).
CMake thirdParty libraries should be marked as imported.
We do so by creating `openPMD::thirdparty::` targets that are
imported only and depend on `find_package` or
`add_subdirectory` targets to decouple them.

Imported targets are always SYSTEM libraries:
https://cmake.org/cmake/help/v3.8/prop_tgt/NO_SYSTEM_FROM_IMPORTED.html

That allows us to use stricter warnings in our project than third
party headers support.
Modifying properties of `IMPORTED` targets is a CMake
3.11.0+ feature:
  https://cmake.org/cmake/help/v3.11/release/3.11.html#commands
@ax3l ax3l mentioned this pull request Nov 30, 2018
6 tasks
@ax3l ax3l merged commit d5f5175 into openPMD:dev Dec 1, 2018
@ax3l ax3l deleted the topic-jsonSystemIncl branch December 1, 2018 17:01
@franzpoeschel
Copy link
Contributor

Hi @ax3l,
I am experiencing an issue including the openPMD API into other projects since this PR has been merged. Minimal failing CMake file:

cmake_minimum_required(VERSION 3.0)
project(openPMD_test)
add_executable(openPMD_mpi_write mpi_write.cpp)
find_package(MPI REQUIRED)
add_definitions(-DOMPI_SKIP_MPICXX)
find_package(openPMD REQUIRED)
target_link_libraries(openPMD_mpi_write PRIVATE openPMD::openPMD MPI::MPI_C MPI::MPI_CXX)

Fails with the output:

-- Configuring done
CMake Error at CMakeLists.txt:3 (add_executable):
  Target "openPMD_mpi_write" links to target
  "openPMD::thirdparty::mpark_variant" but the target was not found.  Perhaps
  a find_package() call is missing for an IMPORTED target, or an ALIAS target
  is missing?


CMake Error at CMakeLists.txt:3 (add_executable):
  Target "openPMD_mpi_write" links to target
  "openPMD::thirdparty::mpark_variant" but the target was not found.  Perhaps
  a find_package() call is missing for an IMPORTED target, or an ALIAS target
  is missing?

The openPMD build configuration is as follows:

> cmake .
-- MPark.Variant: Using INTERNAL version 1.3.0
-- Note: run
   <src_dir>/.travis/download_samples.sh
to add example files to samples/git-sample/ directory!

openPMD build configuration:
  library Version: 0.6.2
  openPMD Standard: 1.1.0
  C++ Compiler: GNU 8.2.0 
    /opt/openmpi/bin/mpic++

  Installation prefix: /usr/local
        bin: bin
        lib: lib
    include: include
      cmake: lib/cmake/openPMD

  Additionally, install following third party libraries:
    MPark.Variant: ON

  Build Type: Release
  Testing: OFF
  Invasive Tests: OFF
  Internal VERIFY: ON
  Build Options:
    MPI: ON
    JSON: OFF
    HDF5: OFF
    ADIOS1: OFF
    ADIOS2: OFF
    PYTHON: OFF

-- Configuring done
-- Generating done
-- Build files have been written to: <build_dir>/openPMD-api-build-MPI

Do you have an idea whether the issue is on my side? When I checkout the last commit before this one has been merged, everything works fine.

@ax3l
Copy link
Member Author

ax3l commented Dec 14, 2018

I think it's on my side, indeed there might be a find_package missing after import or an export of the new targets. Will check!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: JSON install third party third party libraries that are shipped and/or linked warning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants