json icon indicating copy to clipboard operation
json copied to clipboard

pkgconfig integration wrongly rendered if tests are run

Open dvzrv opened this issue 4 years ago • 5 comments

When running the tests for the project after building it (with -DCMAKE_INSTALL_PREFIX=/usr), the pkgconfig file is rendered with /usr/local/include instead of /usr/include.

What is the issue you have?

When building and running the tests, the CMAKE_INSTALL_PREFIX is not honored when creating the pkgconfig integration.

Please describe the steps to reproduce the issue.

  cmake -DCMAKE_INSTALL_PREFIX='/usr' \
        -DCMAKE_INSTALL_LIBDIR='/usr/lib' \
        -DCMAKE_BUILD_TYPE='None' \
        -DBUILD_TESTING=ON \
        -DJSON_BuildTests=ON \
        -DJSON_MultipleHeaders=ON \
        -Wno-dev \
        -B "build" \
        -S .
  make VERBOSE=1 -C "build"
  make -k test -C "build"
  make DESTDIR="${pkgdir}" install -C build

The pkgconfig integration will contain /usr/local/include, but should include /usr/include.

When building without tests, CMAKE_INSTALL_PREFIX is honored and the pkgconfig file contains /usr/include:

  cmake -DCMAKE_INSTALL_PREFIX='/usr' \
        -DCMAKE_INSTALL_LIBDIR='/usr/lib' \
        -DCMAKE_BUILD_TYPE='None' \
        -DJSON_MultipleHeaders=ON \
        -Wno-dev \
        -B "build" \
        -S .
  make VERBOSE=1 -C "build"
  make DESTDIR="${pkgdir}" install -C build

Can you provide a small but working code example?

n/a

What is the expected behavior?

CMAKE_INSTALL_PREFIX is always honored. Tests never influence the installed files.

And what is the actual behavior instead?

CMAKE_INSTALL_PREFIX is not honored if tests are run. Running tests influences the installed files.

Which compiler and operating system are you using?

  • Compiler: gcc 11.1.0
  • Operating system: Arch Linux

Which version of the library did you use?

  • [x] latest release version 3.9.1
  • [ ] other release - please state the version: ___
  • [ ] the develop branch

If you experience a compilation error: can you compile and run the unit tests?

  • [ ] yes
  • [ ] no - please copy/paste the error message below

dvzrv avatar Aug 01 '21 19:08 dvzrv

Thanks for reporting. Maybe @doronbehar @eli-schwartz @xvitaly @ericonr who worked on pkg-config related PRs in the past can help here.

nlohmann avatar Aug 01 '21 19:08 nlohmann

Is this just #2324 and #2318 again?

For what it's worth, I simply don't understand the initial resolution at all. The testsuite should not be writing into the main project's build configuration. Admittedly, I'm equally at fault here, for slapping on a band-aid fix in the second link.

In self-defense, I find cmake incredibly confusing, and at the time I believe I thought it would be enough. But as later history showed, it ends up producing the wrong content... and I just do not think that "don't test those tests" is the correct way to handle it.

I don't actually understand the underlying problem (and haven't really investigated it), but... why does running a test that executes cmake to build a test project, cause the main build configuration to execute the make install of the test case?

eli-schwartz avatar Aug 01 '21 20:08 eli-schwartz

I also have little experience with pkg-config and only use CMake in this project to serve my needs (i.e., compile and execute the tests).

nlohmann avatar Dec 29 '21 09:12 nlohmann

I could not reproduce in Ubuntu 22.04, cmake 3.22.1. Checked the pkg-config files:

/build/nlohmann_json.pc
./build/tests/cmake_fetch_content/_deps/json-build/nlohmann_json.pc
./build/tests/cmake_fetch_content2/_deps/json-build/nlohmann_json.pc
./build/tests/cmake_add_subdirectory/nlohmann_json/nlohmann_json.pc

In all tests the main file (nlohmann_json.pc) have the config Cflags: -I/usr/include and the others with the local path.

Can you drive me where (file?, install output?) the error was found?

aleungaro avatar Aug 17 '22 19:08 aleungaro

I've also tried to reproduce this issue before the 3.11.0 release and was unable to do so. I suspect this might have been fixed when uses of CMAKE_BINARY_DIR were replaced with PROJECT_BINARY_DIR.

falbrechtskirchinger avatar Aug 18 '22 04:08 falbrechtskirchinger