Fix capnp/capnpc --version when built using CMake

[nyanpasu64]

Currently, running capnp[c] --version on Arch Linux (both the current 0.8.0 and my self-built 0.9.0 package) prints "Cap'n Proto version (unknown)" because VERSION is a CMake variable but not a C++ definition. This adds VERSION as a C++ definition, fixing the resulting binaries. This also switches CAPNP_INCLUDE_DIR to a target_compile_definitions call for simplicity.

Old

The existing CMakeLists.txt is strange though; the following overwrites the list of definitions:

  set_target_properties(capnp_tool PROPERTIES
    COMPILE_DEFINITIONS CAPNP_INCLUDE_DIR=\"${CMAKE_INSTALL_FULL_INCLUDEDIR}\"
  )

I tried adding variations of ;VERSION="${VERSION}" to set two definitions, but it was rejected since CMake thought it was multiple arguments to set_target_properties. I decided to append onto your existing string with a separate target_compile_definitions command, since it was easier to get working.

Do you want to replace set_target_properties(... COMPILE_DEFINITIONS) with target_compile_definitions entirely?

[kentonv]

Note that the official Cap'n Proto build is the autotools build. Any distro packages should be built with autotools, not with cmake. The cmake files were contributed by external volunteers and I personally don't know cmake, hence the cmake files are more likely to have bugs -- like this one.

Arch should switch to using the autotools build.

With that said, we should still fix this. @harrishancock does this look OK?

[vlovich]

I do happen to know CMake quite a bit although that knowledge has waxed and waned over the years . I think this PR is fine as is the suggestion to switch to target_compile_definitions. I can't think of why it might be a problem, but I also don't understand why set_target_properties would ever be preferred. Maybe this CMake was written before 2.8.11 or needed to support older CMake (2015)? That would be plausible since the core CMakeLists.txt file dates back to 2014. I don't think there's any plausible reason to support a CMake executable older than that - distributors can be responsible for using a newer CMake.

[vlovich]

@nyanpasu64 did you want to change the whole thing to use target_compile_definitions or do you want me to merge it in as-is?

[nyanpasu64]

I'm not sure. It was added in 2020 at #1071, and it seems @p4l1ly wanted "to pass the double quotes to COMPILE_FLAGS", which I don't know if it's possible with target_compile_definitions. I'm not sure why both COMPILE_FLAGS and COMPILE_DEFINITIONS were brought up in the discussion, but not target_compile_definitions. It does seem that the intent was not to wipe out prior definitions if present (which I'm not sure if any exist).

In any case, the CAPNP_INCLUDE_DIR code is related to additional include directories. msys2 also builds capnp using CMake, and is currently on 0.8.0 still, and has capnp --version = (unknown), and is missing the actual include directory so you need to call capnp compile -I/mingw64/include/ (0.9.0 should have it fixed).

I could try the following, but I don't know if \" is necessary for CAPNP_INCLUDE_DIR in either COMPILE_DEFINITIONS or target_compile_definitions (the above PR says the backslashes were necessary for COMPILE_DEFINITIONS).

  target_compile_definitions(capnp_tool PRIVATE
    CAPNP_INCLUDE_DIR=\"${CMAKE_INSTALL_FULL_INCLUDEDIR}\"
    VERSION="${VERSION}"
  )

Perhaps I'll remove the backslashes while I'm at it. Both CAPNP_INCLUDE_DIR and VERSION are string literals, and target_compile_definitions(VERSION) works fine without backslashes.

[Sarcasm]

Your latest commit, with target_compile_definitions and no backslashes works fine for me.

$ ninja -v capnp_tool
[1/2] /usr/bin/c++ -DCAPNP_INCLUDE_DIR=\"/usr/local/include\" -DVERSION=\"0.10-dev\" ... -c /tmp/capnproto/c++/src/capnp/compiler/capnp.c++

And it seems it is intended according to CMake documentation, although it may be using legacy syntax?

CMake will automatically escape the value correctly for the native build system (note that CMake language syntax may require escapes to specify some values).
-- https://cmake.org/cmake/help/git-master/prop_tgt/COMPILE_DEFINITIONS.html

To support legacy CMake code, unquoted arguments may also contain double-quoted strings ("...", possibly enclosing horizontal whitespace), <snip...>.

Unescaped double-quotes must balance, may not appear at the beginning of an unquoted argument, and are treated as part of the content.
For example, the unquoted arguments -Da="b c", -Da=$(v), and a" "b"c"d are each interpreted literally.
They may instead be written as quoted arguments "-Da=\"b c\"", "-Da=$(v)", and "a\" \"b\"c\"d", respectively.

-- https://cmake.org/cmake/help/v3.19/manual/cmake-language.7.html#unquoted-argument

[p4l1ly]

If I remember well, the only reason why I did not bring target_compile_definitions into the discussion in #1071 was that I did not know about it. I just came across with COMPILE_FLAGS and COMPILE_DEFINITIONS first in the CMake documentation.

[nyanpasu64]

Replacing my current "legacy" code with the recommended alternative looks a lot uglier, but works as well (on Windows at least). I'm not sure what the community (as opposed to the official docs) prefers.

  target_compile_definitions(capnp_tool PRIVATE
    "CAPNP_INCLUDE_DIR=\"${CMAKE_INSTALL_FULL_INCLUDEDIR}\""
    "VERSION=\"${VERSION}\""
  )

I'm not pushing this command at the moment because I feel it's less readable, but I'm open to receiving feedback either way.

[p4l1ly]

🤷 not too ugly... As the previous, slightly more beautiful option is officially legacy, I would keep the latter one.

[Sarcasm]

Although I find the unquoted argument prettier, I think it may be better to use the quoted argument syntax:

We do not recommend using legacy unquoted arguments in new code.
Instead use a Quoted Argument or a Bracket Argument to represent the content.
-- https://cmake.org/cmake/help/v3.19/manual/cmake-language.7.html#unquoted-argument

[nyanpasu64]

I found a bug report with a discussion saying that unquoted_legacy (foo="bar") parameters seem to be janky and unreliable. I force-pushed the ugly version instead.

[harrishancock]

Makes sense to me, thanks for figuring this out!