marzer / tomlplusplus

Header-only TOML config file parser and serializer for C++17.
https://marzer.github.io/tomlplusplus/
MIT License
1.58k stars 152 forks source link

build(meson): CMake's Package Version file is installed in the wrong place #140

Closed Tachi107 closed 2 years ago

Tachi107 commented 2 years ago

Environment

toml++ version and/or commit hash: 248e6031cfed41e24b3bdbed4da6780ff3e3978f to present

Target arch: all

Describe the bug

Quoting https://github.com/marzer/tomlplusplus/commit/248e6031cfed41e24b3bdbed4da6780ff3e3978f#commitcomment-65698096

  1. The architecture-independent directory of CMake Config and Version files is datadir/cmake
    1. CMake Version files are not architecture independent unless created with the ARCHITECTURE_INDEPENDENT option, and that is currently not supported by Meson (from the CMake docs : _"an architecture check will be performed, and the package will be considered compatible only if the architecture matches exactly. For example, if the package is built for a 32-bit architecture, the package is only considered compatible if it is used on a 32-bit architecture, unless ARCHINDEPENDENT is given")

I don't use personally use the CMake integration, but as a (newbie) Debian package maintainer I'm starting to care about architecture compatibilities. The most appropriate fix would be to set ARCH_INDEPENDENT, but that would require a patch in upstream Meson, so we currently have three options:

  1. Keep using Meson's write_basic_package_version_file(), but move the output to the arch-dependent libdir/cmake - might require to also move the Config file, and it is not right
  2. Manually write an arch-independent CMake Version file, and install that in datadir/cmake - I don't know if you ever looked at one, but they are not really human-friendly
  3. Drop the CMake Version file and keep only the Config file, and wait for a fix in Meson (I'm working on it) - users would still be able to run find_package(tomlplusplus), but without specifying the desired version

I haven't used Toml++ in quite some time, but I believe that this library can be consumed as both an header-only library and a compiled library, right? If so, neither the pkg-config file nor the CMake ones should unconditionally get installed in the architecture-independent directory, but that should depend on the current configuration (libdir when compiling the library, datadir when header-only)

marzer commented 2 years ago

but I believe that this library can be consumed as both an header-only library and a compiled library, right?

The library is always header-only. The user can choose to 'compile' it by disabling header-only mode and marking a specific TU as the 'implementation' one, but that's got nothing to do with the build system in the traditional sense.

Tachi107 commented 2 years ago

Unrelated to this issue, but would you accept a PR enabling support for building and installing the library implemented in Meson? I ask because I'd like to package this library for Debian (and derivatives), and traditional libraries are generally preferred to header-only ones (thanks to shared linking- packaging a header-only library doesn't make users' lives better). I'm thinking to something similar to how cpp-httplib does it (but without the split.py part), you can take a look here: https://github.com/yhirose/cpp-httplib/blob/master/meson.build

marzer commented 2 years ago

Unrelated to this issue, but would you accept a PR enabling support for building and installing the library implemented in Meson?

As long as it's not overly disruptive to current workflows and isn't cumbersome to maintain, sure. It being header-only is mostly for my benefit.

Unrelated to this issue,

What is the path forward for this this issue? I'm not going to do anything about CMake-related issues myself, since I value my blood un-boiled - I'd sooner just close it. From your description above, it sounds like "do nothing until meson gets a fix" is the right move, but I don't know enough about the interop to make any real suggestions here.

Tachi107 commented 2 years ago

The fix to Meson is pending in mesonbuild/meson#9916, but will require toml++ to bump its Meson requirement to version 0.62.0 (when released).

Doing nothing about this would prevent the library from getting packaged in Linux distributions and basically any other package manager (or at least that's what I would do), since it causes issues for things like cross-architecture compatibility.

In my opinion, the "best" solution to this is to drop Meson's write_basic_package_version_file() for the time being and ship a fixed Version file stored in cmake/, and when Meson 0.62.0 will be released the usage of the generator could be restored.

If you like this solution, I could prepare a PR :)

marzer commented 2 years ago

If you like this solution, I could prepare a PR :)

Sounds good to me, fire away

eli-schwartz commented 2 years ago

Doing nothing about this would prevent the library from getting packaged in Linux distributions and basically any other package manager (or at least that's what I would do), since it causes issues for things like cross-architecture compatibility.

They would also have the option to configure the project with -Dgenerate_cmake_config=false. :D

but will require toml++ to bump its Meson requirement to version 0.62.0 (when released).

Not true. Given: https://github.com/marzer/tomlplusplus/blob/8e669aa6990e0ed219c169d491472d749f54c393/meson.build#L534-L538

This could be changed to:

    # cmake_installdir = get_option('build_libary') ? 'lib' : get_option('datadir')
    # cmake_installdir = cmake_installdir / 'cmake' / meson.project_name()
    cmake_installdir = 'lib/cmake' / meson.project_name()
    if meson.version().version_compare('>=0.62.0')
        cmake.write_basic_package_version_file(
            name: meson.project_name(),
            version: meson.project_version(),
            arch_independent: true, # get_option('build_library')
            install_dir: cmake_installdir,
        )
    else
        cmake.write_basic_package_version_file(
            name: meson.project_name(),
            version: meson.project_version(),
            install_dir: cmake_installdir,
        )
    endif

It would then generate an architecture-dependent file on older versions of meson, and an architecture-independent file on newer versions of meson. People using old versions of meson would "pay the price" by having worse versions of the file, which they may or may not care about (Debian certainly does).

You could also use:

    if meson.version().version_compare('>=0.62.0')
        cmake_kwargs = {'arch_independent': true}
    else
        cmake_kwargs = {}
    endif
    cmake.write_basic_package_version_file(
        name: meson.project_name(),
        version: meson.project_version(),
        install_dir: 'lib'/'cmake'/meson.project_name(),
        kwargs: cmake_kwargs,
    )

but that would emit a FeatureNew warning on 0.62.

Tachi107 commented 2 years ago

They would also have the option to configure the project with -Dgenerate_cmake_config=false. :D

Right

but will require toml++ to bump its Meson requirement to version 0.62.0 (when released).

Not true. Given: https://github.com/marzer/tomlplusplus/blob/8e669aa6990e0ed219c169d491472d749f54c393/meson.build#L534-L538

This could be changed to:

  # cmake_installdir = get_option('build_libary') ? 'lib' : get_option('datadir')
  # cmake_installdir = cmake_installdir / 'cmake' / meson.project_name()
  cmake_installdir = 'lib/cmake' / meson.project_name()
  if meson.version().version_compare('>=0.62.0')
      cmake.write_basic_package_version_file(
          name: meson.project_name(),
          version: meson.project_version(),
          arch_independent: true, # get_option('build_library')
          install_dir: cmake_installdir,
      )
  else
      cmake.write_basic_package_version_file(
          name: meson.project_name(),
          version: meson.project_version(),
          install_dir: cmake_installdir,
      )
  endif

Oh right, ifs... I always forget about them :/