dropbox / json11

A tiny JSON library for C++11.
MIT License
2.55k stars 615 forks source link

installation if json11 is added as a library works now #95

Closed Pfeifenjoy closed 7 years ago

Pfeifenjoy commented 7 years ago

Please consider to change this, otherwise if you include the library like this:

add_subdirectory(external/json11) set(JSON11_INCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/external/json11) set(JSON11_LIBRARIES json11)

it will work but if you run make install on your project it cannot find the json11.hpp and json11.pc file, because it searches in the wrong build directory.

Also double check if this might cause trouble with other configuration. I haven't experienced side effects.

smarx commented 7 years ago

Automated message from Dropbox CLA bot

@Pfeifenjoy, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here.

Pfeifenjoy commented 7 years ago

@smarx I agree that this minor change is published under the dropbox licence.

artwyman commented 7 years ago

Thanks @Pfeifenjoy. To keep the lawyers happy, I need to ask you to actually sign the online CLA form before I can merge this, so we can track your agreement beyond your comment here.

Also, since we don't use CMake at Dropbox this file is community-maintained, and I don't trust myself to be sure of not impacting others. I wonder if other contributors like @admsyn or @4brunu have any commentary on the suitability of this change.

4brunu commented 7 years ago

Hi, I was updating json11 to try this, and found that CMakeLists.txt is broken to me, since https://github.com/dropbox/json11/commit/649b54ae2886c41d78861e48b2eaa89bb5ca993d get merged. I tried this commit but it's still broken.

artwyman commented 7 years ago

@4brunu do you want to suggest a fix, or do you think I should revert? @hhony want to take a look?

As I said, CMake is sort of a side-project for json11, not something I use directly, so I'm just the gatekeeper on other people's contributions.

admsyn commented 7 years ago

@4brunu what is the error you get? or in what way is it broken for you? For me:

adam@adam-xps:~/workspace/scratch$ git clone https://github.com/dropbox/json11.git
Cloning into 'json11'...
remote: Counting objects: 248, done.
remote: Total 248 (delta 0), reused 0 (delta 0), pack-reused 248
Receiving objects: 100% (248/248), 66.63 KiB | 0 bytes/s, done.
Resolving deltas: 100% (136/136), done.
Checking connectivity... done.
adam@adam-xps:~/workspace/scratch$ mkdir json11/build
adam@adam-xps:~/workspace/scratch$ cd json11/build/
adam@adam-xps:~/workspace/scratch/json11/build$ cmake ..
-- The CXX compiler identification is GNU 5.4.0
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /home/adam/workspace/scratch/json11/build
adam@adam-xps:~/workspace/scratch/json11/build$ make
Scanning dependencies of target json11
[ 25%] Building CXX object CMakeFiles/json11.dir/json11.cpp.o
[ 50%] Linking CXX static library libjson11.a
[ 50%] Built target json11
Scanning dependencies of target json11_test
[ 75%] Building CXX object CMakeFiles/json11_test.dir/test.cpp.o
[100%] Linking CXX executable json11_test
[100%] Built target json11_test
adam@adam-xps:~/workspace/scratch/json11/build$ 
smarx commented 7 years ago

Automated message from Dropbox CLA bot

@Pfeifenjoy, thanks for signing the CLA!

Pfeifenjoy commented 7 years ago

@artwyman Sorry I didn't have the time to sign it right away. @4brunu have you tried also changing target_include_directories(json11 PUBLIC ${CMAKE_SOURCE_DIR}) to target_include_directories(json11 PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}).

I only had the problem when I added this library in cmake und created an install goal, because cmake was looking for the json11.pc in the my project directory. However this file is located in the json11 build directory.

- project
    - src
    - external
         - json11

my build directory looked something like this.

- Makefile, program.exe ...
- external
    -json11
        -json11.pc

It was searching for the json11.pc in the build directory rather than in build/external/json11.

hhony commented 7 years ago

@4brunu: sorry you're having trouble (and the delayed response). What is the cmake error you get? What cmake --version are you running? I made this change while using v3.5.2.

@artwyman: you're always welcome to change L24-L25 to DESTINATION local/{include,lib} instead of {include,lib}/<arch>.

My thought first thought is that /usr/local/lib and /usr/local/include might work better for the general public. My apologies - that commit was relative to a side-project at the time.. and the more important portion of that pull is -fPIC flag in target_compile_options.

hhony commented 7 years ago

@Pfeifenjoy: from your comment 'cmake was looking for the json11.pc in the my project directory'.. confuses me.

json11.pc is always generated in the build directory by using json11.pc.in as the input template. Therefore, it will always reside in the build directory unless you run make install.. then of course - any project attempting to find json11 for linking should use pkgconfig, to find it - as long as the install location is in your $PATH.

Have your tried just adding /path/to/your/json11/builddir/ to your $PATH? If you revert your changes, does that solve your issue?

4brunu commented 7 years ago

Changing target_include_directories(json11 PUBLIC ${CMAKE_SOURCE_DIR}) to target_include_directories(json11 PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) solved the issue.

@Pfeifenjoy could you add this change to your pull request?

My problem was when building with Android Studio, CMake 3.6.2155560.

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':app:externalNativeBuildProjectDebug'.
> Build command failed.
  Error while executing '/Applications/android-sdk/cmake/3.6.3155560/bin/cmake' with arguments {--build /Users/user/Developer/Project/android_project/ProjectAndroid/app/.externalNativeBuild/cmake/projectDebug/mips64 --target project}
  [1/52] Building CXX object deps/json11/CMakeFiles/json11.dir/json11.cpp.o
  [2/52] Building CXX object deps/SQLiteCpp/CMakeFiles/SQLiteCpp.dir/src/Backup.cpp.o
  [3/52] Building CXX object deps/SQLiteCpp/CMakeFiles/SQLiteCpp.dir/src/Column.cpp.o
  [4/52] Building CXX object deps/SQLiteCpp/CMakeFiles/SQLiteCpp.dir/src/Database.cpp.o
  [5/52] Building CXX object deps/SQLiteCpp/CMakeFiles/SQLiteCpp.dir/src/Exception.cpp.o
  [6/52] Building CXX object deps/SQLiteCpp/CMakeFiles/SQLiteCpp.dir/src/Statement.cpp.o
  [7/52] Building CXX object deps/SQLiteCpp/CMakeFiles/SQLiteCpp.dir/src/Transaction.cpp.o
  [8/52] Building C object deps/SQLiteCpp/sqlite3/CMakeFiles/sqlite3.dir/sqlite3.c.o
  /Users/user/Developer/Project/deps/SQLiteCpp/src/Database.cpp:31:13: warning: unused variable 'OPEN_MEMORY' [-Wunused-const-variable]
  const int   OPEN_MEMORY     = SQLITE_OPEN_MEMORY;
              ^
  1 warning generated.
  [9/52] Linking CXX static library deps/SQLiteCpp/libSQLiteCpp.a
  [10/52] Linking CXX static library deps/json11/libjson11.a
  [11/52] Linking C static library deps/SQLiteCpp/sqlite3/libsqlite3.a
  [12/52] Building CXX object android_project/ProjectAndroid/app/src/main/cpp/CMakeFiles/project.dir/__/__/__/__/__/__/src/main/cpp/apns/ModelAPNSImpl.cpp.o
  [13/52] Building CXX object android_project/ProjectAndroid/app/src/main/cpp/CMakeFiles/project.dir/__/__/__/__/__/__/src/main/cpp/database/DatabaseManagerImpl.cpp.o
  [14/52] Building CXX object android_project/ProjectAndroid/app/src/main/cpp/CMakeFiles/project.dir/__/__/__/__/__/__/src/main/cpp/database/DatabaseMigarionHelper.cpp.o
  [15/52] Building CXX object android_project/ProjectAndroid/app/src/main/cpp/CMakeFiles/project.dir/__/__/__/__/__/__/src/main/cpp/network/NetworkRequestManager.cpp.o
  [16/52] Building CXX object android_project/ProjectAndroid/app/src/main/cpp/CMakeFiles/project.dir/__/__/__/__/__/__/src/main/cpp/example/ModelExampleImpl.cpp.o
  [17/52] Building CXX object android_project/ProjectAndroid/app/src/main/cpp/CMakeFiles/project.dir/__/__/__/__/__/__/src/main/cpp/test/ModelTestImpl.cpp.o
  FAILED: /Applications/android-sdk/ndk-bundle/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang++  -target mips64el-none-linux-android -gcc-toolchain /Applications/android-sdk/ndk-bundle/toolchains/mips64el-linux-android-4.9/prebuilt/darwin-x86_64 --sysroot=/Applications/android-sdk/ndk-bundle/platforms/android-21/arch-mips64  -Dproject_EXPORTS -I../../../../../../../src/main/cpp -I../../../../../../../deps/djinni/support-lib/jni -I../../../../../../../generated-src/cpp -I../../../../../../../generated-src/jni -I../../../../../../../ -I../../../../../../../deps/SQLiteCpp/include -I../../../../../../../deps/SQLiteCpp/sqlite3 -isystem /Applications/android-sdk/ndk-bundle/sources/cxx-stl/llvm-libc++/include -isystem /Applications/android-sdk/ndk-bundle/sources/android/support/include -isystem /Applications/android-sdk/ndk-bundle/sources/cxx-stl/llvm-libc++abi/include -g -DANDROID -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -Wa,--noexecstack -Wformat -Werror=format-security -fno-exceptions -fno-rtti -std=c++11 -g -DANDROID -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -Wa,--noexecstack -Wformat -Werror=format-security -fno-exceptions -fno-rtti -std=c++11  -std=c++14 -fexceptions -frtti -Wall -O0 -fno-limit-debug-info -O0 -fno-limit-debug-info  -fPIC -MD -MT android_project/ProjectAndroid/app/src/main/cpp/CMakeFiles/project.dir/__/__/__/__/__/__/src/main/cpp/test/ModelTestImpl.cpp.o -MF android_project/ProjectAndroid/app/src/main/cpp/CMakeFiles/project.dir/__/__/__/__/__/__/src/main/cpp/test/ModelTestImpl.cpp.o.d -o android_project/ProjectAndroid/app/src/main/cpp/CMakeFiles/project.dir/__/__/__/__/__/__/src/main/cpp/test/ModelTestImpl.cpp.o -c /Users/user/Developer/Project/src/main/cpp/test/ModelTestImpl.cpp
  In file included from /Users/user/Developer/Project/src/main/cpp/test/ModelTestImpl.cpp:2:
  /Users/user/Developer/Project/src/main/cpp/test/ModelTestImpl.hpp:10:5: warning: class 'DTOTestToServer' was previously declared as a struct [-Wmismatched-tags]
      class DTOTestToServer;
      ^
  ../../../../../../../generated-src/cpp/DTOTestToServer.hpp:10:8: note: previous use is here
  struct DTOTestToServer final {
         ^
  /Users/user/Developer/Project/src/main/cpp/test/ModelTestImpl.hpp:10:5: note: did you mean struct here?
      class DTOTestToServer;
      ^~~~~
      struct
  /Users/user/Developer/Project/src/main/cpp/test/ModelTestImpl.cpp:3:10: fatal error: 'json11.hpp' file not found
  #include <json11.hpp>
           ^
  1 warning and 1 error generated.
  FAILED: /Applications/android-sdk/ndk-bundle/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang++  -target mips64el-none-linux-android -gcc-toolchain /Applications/android-sdk/ndk-bundle/toolchains/mips64el-linux-android-4.9/prebuilt/darwin-x86_64 --sysroot=/Applications/android-sdk/ndk-bundle/platforms/android-21/arch-mips64  -Dproject_EXPORTS -I../../../../../../../src/main/cpp -I../../../../../../../deps/djinni/support-lib/jni -I../../../../../../../generated-src/cpp -I../../../../../../../generated-src/jni -I../../../../../../../ -I../../../../../../../deps/SQLiteCpp/include -I../../../../../../../deps/SQLiteCpp/sqlite3 -isystem /Applications/android-sdk/ndk-bundle/sources/cxx-stl/llvm-libc++/include -isystem /Applications/android-sdk/ndk-bundle/sources/android/support/include -isystem /Applications/android-sdk/ndk-bundle/sources/cxx-stl/llvm-libc++abi/include -g -DANDROID -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -Wa,--noexecstack -Wformat -Werror=format-security -fno-exceptions -fno-rtti -std=c++11 -g -DANDROID -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -Wa,--noexecstack -Wformat -Werror=format-security -fno-exceptions -fno-rtti -std=c++11  -std=c++14 -fexceptions -frtti -Wall -O0 -fno-limit-debug-info -O0 -fno-limit-debug-info  -fPIC -MD -MT android_project/ProjectAndroid/app/src/main/cpp/CMakeFiles/project.dir/__/__/__/__/__/__/src/main/cpp/example/ModelExampleImpl.cpp.o -MF android_project/ProjectAndroid/app/src/main/cpp/CMakeFiles/project.dir/__/__/__/__/__/__/src/main/cpp/example/ModelExampleImpl.cpp.o.d -o android_project/ProjectAndroid/app/src/main/cpp/CMakeFiles/project.dir/__/__/__/__/__/__/src/main/cpp/example/ModelExampleImpl.cpp.o -c /Users/user/Developer/Project/src/main/cpp/example/ModelExampleImpl.cpp
  /Users/user/Developer/Project/src/main/cpp/example/ModelExampleImpl.cpp:5:10: fatal error: 'json11.hpp' file not found
  #include <json11.hpp>
           ^
  1 error generated.
  ninja: build stopped: subcommand failed.
hhony commented 7 years ago

weird.. I do not have that issue. According to the doc for CMAKE_CURRENT_SOURCE_DIR:

This the full path to the source directory that is currently being processed by cmake.

as opposed to CMAKE_SOURCE_DIR .. which represents the top-level json11.git checkout on your filesystem:

This is the full path to the top level of the current CMake source tree.

Pfeifenjoy commented 7 years ago

@4brunu I will do that this evening @hhony lets say I have a project and place json11 in external/json11. Than I put in my CMakeLists.txt:

add_subdirectory(external/json11)
set(JSON11_INCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/external/json11)
set(JSON11_LIBRARIES json11)
include_directories(${JSON11_INCLUDE_DIR})
target_link_libraries(${CMAKE_PROJECT_NAME} ${JSON11_LIBRARIES})
install(TARGETS ${PROJECT_NAME}
    ARCHIVE  DESTINATION lib
    LIBRARY  DESTINATION lib
    RUNTIME  DESTINATION bin  # This is for Windows
)

That means I don't install json11 rather include it in my project and if I than run make && make install in my project it will also be installed. In this scenario in the build directory of "my project" (not json11) there is no json11.pc, because json11 is added in "my project" build directory under external/json11. This directory however includes the json11.pc. This PR just fixes if I run make install in "my projects" build directory that cmake is looking for the json11.pc in the json11 build directory which is a subdirectory of "my projects" build directory.

I hope this can resolve your confusion.

hhony commented 7 years ago

That's a little clearer.. sounds like a little bit of taste is mixed into it.

I suppose I generally build this a shared library inside json11/builddir and use cmake .. from inside builddir.. then I run make install - so other projects can link against when they build later.

I generally have to build json11 only once (would be the main difference, I suppose).

Either way seems valid, I suppose CMAKE_CURRENT_SOURCE_DIR might offer the most flexibility, in retrospect.

Pfeifenjoy commented 7 years ago

@artwyman you can merge now

artwyman commented 7 years ago

@hhony @4brunu can you review the latest form of this and tell me if you concur? Apparently I can't assign you as reviewers because you're not in the Dropbox organization, but since I don't have a CMake setup to test this, I want to make sure there's consensus before merging.

4brunu commented 7 years ago

@artwyman For me it's good, now my builds are working 👍

hhony commented 7 years ago

@artwyman: yes, cmake, make and make install work as expected.