Use a consistent target name for jsoncpp in CMake scripts

[rcdailey]

The target name for the jsoncpp library would change based on whether we were building a static or shared library. This inconsistency made it difficult and unintuitive to pull in jsoncpp as a submodule in
another repository and link to it directly via other CMake scripts.

Having a consistent target name will allow libraries with their own CMake scripts to reliably refer to jsoncpp as a dependency. Example CMake script below shows how I would refer to jsoncpp as a dependency in my own parent project. Note that jsoncpp in this case is a submodule in my git repository.

set( project_name my-project )

project( ${project_name} )

set( sources
    main.cpp
)

add_subdirectory( jsoncpp )

add_executable( ${project_name} ${sources} )
target_link_libraries( ${project_name} PRIVATE jsoncpp )

Other Changes:

• BUILD_SHARED_LIBS and BUILD_STATIC_LIBS removed in favor of JSONCPP_LIBRARY_TYPE, which allows you to pick either SHARED or STATIC library variations. This change was made to prevent both shared and static libraries being built at the same time. This isn't allowed anymore since we only generate 1 target for the jsoncpp library now.
• Clean up closing statements for if conditions, functions, macros, and other entities. Newer versions of CMake do not require you to redundantly respecify the parameters to the opening arguments.
• travis.sh build script updated to perform CMake generation in an out-of-source binary directory. This will prevent the temporary generated output files from intermixing into the source tree and allow for multiple generations with different configurations using the same source tree.

[rcdailey]

Obviously the CI build is failing because of this change. If you have any advice on how you'd like to fix this, let me know.

What I'm attempting right now is to make CMake perform an out-of-source build (Run CMake from a temporary directory). This will allow the CI build to generate multiple times per CMake configuration, each time into its own build directory.

This will require a new CI build parameter to specify the desired cmake binary directory. I am not sure how to do that.

[cdunn2001]

👍

For the new branch only. The old code needs to work with olc cmake.

I'll look more closely this weekend.

[rcdailey]

I'm not sure what you mean by "old code needs to work with old cmake". Are you referring to old build script? Passing in 1 new environment variable (leaving existing ones intact) should be fully backward compatible. But I can only generalize without being familiar with the way Travis works. Never used it before.

[rcdailey]

FYI: I updated my original commit message to go into more detail on all the changes. I may split my commit into smaller ones at a later time, but the diff itself isn't too huge to begin with.

Please review my original PR description again to get updated on the scope of change and let me know how you feel about the changes. Happy to work with you on any adjustments needed. All changes here were made on Windows only via CMake 3.3.0 for my needs only (i.e. plugging your library up into my own project so I can use it).

Thanks again for your time and I look forward to your feedback.

[cdunn2001]

By "old code" I mean the old branch, 0.y.z, which actually still builds fine with rather old compilers. We'll probably not merge these changes to that branch, which is fine.

[cdunn2001]

Why did you remove the expressions from ENDIF/ENDFUNCTION/etc?

[cdunn2001]

I don't see any reason for the change to travis.sh either. Travis parallel builds occur in separate VMs already, I think. And because we use Docker containers, the builds start quickly.

Of course, this doesn't hurt either, but it is helpful to limit changes to the purpose. It seems like every time I accept changes to the cmakefiles, somebody breaks, so I'd really prefer to keep the changes minimal.

[cdunn2001]

Thanks for giving the cmakefiles some needed attention. I have just a few questions/suggestions.

You didn't change the matrix in .travis.yml, and you removed the options from cmake without changing the invocation of cmake in travis.sh.

Could you update those 2 files? If you prefer to build in 2 separate build-directories for a single Travis build (dropping the matrix, and instead invoking cmake/make twice in travis.sh), that's fine with me. But we definitely need to test building both shared and static in the same build, since that was a specific feature request from someone.

Also, I see an odd behavior.

cmake -DCMAKE_BUILD_TYPE=debug -DBUILD_SHARED_LIBS=ON -DARCHIVE_INSTALL_DIR=. -DCMAKE_VERBOSE_MAKEFILE=ON -G "Unix Makefiles" ../..
cmake -DCMAKE_BUILD_TYPE=debug -DBUILD_STATIC_LIBS=ON -DARCHIVE_INSTALL_DIR=. -DCMAKE_VERBOSE_MAKEFILE=ON -G "Unix Makefiles" ../..

Both those succeed without warnings. But

$  cmake -DCMAKE_BUILD_TYPE=debug -DBUILD_STATIC_LIBS=ON -DBUILD_SHARED_LIBS=ON -DARCHIVE_INSTALL_DIR=. -DCMAKE_VERBOSE_MAKEFILE=ON -G "Unix Makefiles" ../..
-- JsonCpp Version: 1.6.5
-- Configuring done
-- Generating done
CMake Warning:
  Manually-specified variables were not used by the project:

    BUILD_STATIC_LIBS

Why?

I also wonder whether we should update this file:

devtools/agent_vmw7.json:            ["BUILD_SHARED_LIBS=true"],

We should definitely update README.md.

In principle, I like these changes and plan to merge them, but you could address those minor issues first?

[rcdailey]

I will try to address all of your concerns below.

Why did you remove the expressions from ENDIF/ENDFUNCTION/etc?

Older (and I mean really old) versions of CMake syntactically required you to repeat the contents of the beginning block for if, macro, etc in the closing block. Newer versions of CMake (2.8 and up especially, which is what your minimum CMake is set to) do not require this. And you can simply close blocks with () (i.e. no repetition of content). This makes scripts easier to read and less of a maintenance burden.

These changes were not related to the work being reviewed, and while it does add some "white-noise" to the review, it's good to iteratively clean them up as you go along. I tried to keep the changes minimal to avoid distraction, but there are a lot more that can be cleaned up. I'd propose just cleaning them up as you see them versus hulking it all at once.

I don't see any reason for the change to travis.sh either

Honestly now that I've read back over the script and your comments, I'm seeing that I didn't completely finish the work. I am missing a few variable usage changes when CMake is invoked. I will try to fix up those issues.

The main goal with doing an out-of-source build instead of in-source is not just best practice (which it is, by the way) but to prepare for the situation where a single build will generate 2 times: One for the shared lib, a second for the static.

I went with this approach because it matches more closely to how the script behaved before. Previously, you could enable BUILD_STATIC_LIBS and BUILD_SHARED_LIBS and you'd get 2 output binaries. Now, it's merely a toggle and 2 passes are required to get both output formats if that is your wish.

Another option is to simply have 1 travis build function on 1 form of the library (either static or shared, but not both). I'm not sure what direction you wish to go, so let me know and I'm happy to make the changes. You have more familiarity / history with the code base so this isn't something I can just decide for you. There is no "wrong" way.

It seems like every time I accept changes to the cmakefiles, somebody breaks

Can you give some examples of past occurrences? Certainly we don't want this to happen. I'd also recommend that your CI builds catch these issues as much as possible. Let's go into more detail here and I'll see what I can help with.

You didn't change the matrix in .travis.yml

What is "the matrix" (Waits for corny movie reference)?

you removed the options from cmake without changing the invocation of cmake in travis.sh.

You're correct. I mentioned this lightly earlier on in this response. I'll make those fixes for you; oversight on my part.

Both those succeed without warnings. But

$  cmake -DCMAKE_BUILD_TYPE=debug -DBUILD_STATIC_LIBS=ON -DBUILD_SHARED_LIBS=ON -DARCHIVE_INSTALL_DIR=. -DCMAKE_VERBOSE_MAKEFILE=ON -G "Unix Makefiles" ../..
-- JsonCpp Version: 1.6.5
-- Configuring done
-- Generating done
CMake Warning:
  Manually-specified variables were not used by the project:

    BUILD_STATIC_LIBS

This has to do with the fact that BUILD_STATIC_LIBS is still provided by the build script but it is no longer used in the CMake scripts. Again, this is because I forgot to update the shell script to pass in the new parameters (BUILD_STATIC_LIBS and BUILD_SHARED_LIBS will not be used anymore).

I also wonder whether we should update this file:

devtools/agent_vmw7.json:            ["BUILD_SHARED_LIBS=true"],

We should definitely update README.md.

Yep, documentation is another thing I missed. Sorry about the sloppiness. I'll get this updated as well. In fact, I'll grep everything for these old variables and make sure I don't miss anything else.

Really appreciate the feedback.

[rcdailey]

All corrections addressed (amended to my original commit). Please review again and let me know if all of your concerns are addressed.

FYI: Build matrix need not change since I handle the options better in travis.sh.

[cdunn2001]

This makes scripts easier to read and less of a maintenance burden.

I could debate this, but I'm trying to be concise. Please avoid unrelated changes. I would welcome such changes in a separate commit.

Anyway, you've made the change, so let's keep it.

I found one other minor problem:

+            ["JSONCPP_LIBRARY_TYPE=SHARED"]^M

Your editor seems to add CR sometimes.

[cdunn2001]

Here is more info on the preference for building both shared and static at once:

• Building SHARED and STATIC library #147
• Adapting buildsystem for shared and static libs in same build config #149
• 5e8595c
• f0f6beb81855db2bec799cf78b0a7b0ee70cf0d9

I prefer your way -- build one target at a time -- but it's not really up to me. @cinemast handles the Debian integration. If he needs to build both at the same time, then I defer to him. Can you think of a way to support that?

[rcdailey]

You need a proper .gitattributes file. I can add this for you in a separate commit if you'd like. But at a minimum you need:

* text=auto
*.sh eol=lf
*.bat eol=crlf

This will prevent my EOL from making it into the repository unconverted. There is a risk that by doing this you will need to normalize your line endings in the repository.

[rcdailey]

Per #147, he states:

Yes, but it is not the common practice, and makes the packaging more complicated without benefits.

I disagree. I don't see how what is happening right now is not common practice. It makes zero sense to have 2 targets for the exact same project simply to make it build differently. Not to mention the problems this method introduces (alas, the ones I'm fixing and that are described in my PR description). Maybe he has more information on this he can share. But as it stands his statements are wrong and overly broad without specifics, explanations, or examples.

Also we need a clear definition of "at the same time". Technically, you're getting the same result today as you got yesterday: Two binaries, one build script invocation. If there is more to it than this, I'll wait for some explanations.

[cdunn2001]

re: .gitattributes

Could you rebase to the cma-master branch? Let's see how this works for you. You're the best tester because you're actually developing on Windows.

[rcdailey]

In PR #326, I was attempting to add a .gitattributes for you but looks like you beat me to it :-)

Before I rebase and try things out I recommend you normalize line endings so that my diff doesn't show you full files changed. If you don't normalize line endings all at once now, it will occur the next time someone modifies the files, which will obfuscate your diffs.

[cdunn2001]

Technically, you're getting the same result today as you got yesterday: Two binaries, one build script invocation.

Really? I see only .dylib or .a. Oh, you don't mean from one cmake/make invocation, but rather one 'script', right? Hmmm.

I'm with you, but we'll need a compromise. I'd be fine with building both library types always. The only issue is how to let a consuming package choose the jsconcpp library type. Can you come up with something that would work for everyone?

Also, I see this:

$ cmake -DCMAKE_BUILD_TYPE=debug -DJSONCPP_LIBRARY_TYPE=SHARED -DARCHIVE_INSTALL_DIR=. -G "Unix Makefiles" ../..
-- JsonCpp Version: 1.6.5
-- Configuring done
CMake Warning (dev):
  Policy CMP0042 is not set: MACOSX_RPATH is enabled by default.  Run "cmake
  --help-policy CMP0042" for policy details.  Use the cmake_policy command to
  set the policy and suppress this warning.

  MACOSX_RPATH is not specified for the following targets:

   jsoncpp

That's not new, but do you have any ideas on how to get rid of it?

[rcdailey]

When you refer to a "consuming package", what do you mean exactly? Are you speaking in CMake terms, i.e. a parent project (one that specifies jsoncpp as a dependency)?

Or are you talking about package managers for Linux distros?

If you can throw out some requirements I can probably find a suggestion for you. Honestly the way it is being done now (with my changes) makes no sense from a delivery stand point: Normally you invoke a build knowing in advance how you will build the package and distribute it. So I'll need you to educate me a bit more on your delivery model.

As for your question regarding MACOSX_RPATH, have you ran the command it suggested (cmake --help-policy CMP0042)? I do not have MacOS to test on nor am I very familiar with it. So I may not be much help there.

Thanks.

[cdunn2001]

When you refer to a "consuming package", what do you mean exactly? Are you speaking in CMake terms?

Yes, cmake.

Honestly the way it is being done now (with my changes) makes no sense from a delivery stand point.

? I'm just trying to satisfy the cmake users. I have heard that it's hard to use from other cmake packages, so I was hoping that your changes would help with that.

But I really want to build BOTH a static AND a shared library always. Wouldn't that make everyone happy? Can you make that happen and still have a practical cmake dependency?

(And please rebase to master branch. Updated for gitattributes.)

[rcdailey]

Did you get a chance to review my last comment? I requested some discussion of requirements, so I can better model the build scripts based on your needs.

Thanks.

[cdunn2001]

I was waiting to hear from cinemast in #147. He provided an interesting link:

• http://www.cmake.org/Wiki/CMake_FAQ#Can_I_build_both_shared_and_static_libraries_with_one_ADD_LIBRARY_command.3F

I'm still trying to find a way to avoid build both libs from the same cmake invocation.

[ya1gaurav]

When building both static & shared library duplicate build can be avoided using:
#273

[rcdailey]

I still don't understand why you're trying to avoid 2 distinct builds, each providing a different output. It's the way you want it to work, you just don't know it yet, maybe. What if you have different preprocessor conditions based on the type of library you're building? And if you don't now, you will certainly want that flexibility later (especially on Windows, since objects are not implicitly exported in shared libraries like they are in Linux).

What is the major problem with building the code base twice? Why are you trying to avoid it?

[ya1gaurav]

Current scenario - if both static & shared libraries are built simultaneously .o files are created twice.
The above fix avoid making .o files again & still builds both static & shared libraries simultaneously.

[ya1gaurav]

It is also a cmake feature which is available after cmake verion 2.8.

[rcdailey]

What's wrong with the object files being built twice? Remember: If the preprocessor behaves differently between configurations, the object files themselves will also be different. Even if they were the same, there is nothing wrong with rebuilding translation units twice (once for each distinct target type). The only negative impact is build time, which isn't long anyway for this code base. Plus when it comes to CI builds, how fast the build is isn't that important.

What CMake feature is available at 2.8?

[cdunn2001]

What if you have different preprocessor conditions based on the type of library you're building?

See JSON_API macro.

It's the way you want it to work

Yes! But I cannot harm the Debian build. This is just one library The linux distro folks put an enormous amount of their own time into wonderful free operating systems. Their needs carry weight.

I like your idea. I also like @ya1gaurav's cmake-2.8 improvement from #273, which we've rejected for other reasons. Sorry. I really do want simpler cmakefiles, if possible.

[cinemast]

I figured out a way for Debian (based on your ideas), it is not exactly pretty but it works:

http://anonscm.debian.org/cgit/collab-maint/libjsoncpp.git/tree/debian/rules?h=test&id=44d29209a7cd9264a2c500046c88f9818c386af7

So I do get that you would like to change to only one target. Thank you for considering the effects on Debian of this. I very much appreciate that. From my side, it would be OK to remove the static target, as long as it can be build in a second run of cmake.

jsoncpp is actually quite important inside the Debian archive. Ironically also cmake does depend on it.

Thanks for your efforts. Please let me know when you have come to an agreement on how to set the build-flags for the static and the shared version. I will than test the final approach again.

[cdunn2001]

Replaced (rebased) by #334.

[rcdailey]

Sorry I just realized I somehow forgot about this change for 3 years lol. I guess I ended up dropping this library from my project at work, to be honest I don't remember.

Let me know if I can help any further. Again I sincerely apologize for the neglect.