Skip to content

Conversation

@mpoquet
Copy link
Contributor

@mpoquet mpoquet commented Feb 11, 2021

This PR enables CMake to generate a flatbuffers.pc file on install, so end users can find the flatbuffers library.

pkg-config eases the utilization of software libraries by enabling the developers of a library to define how the library should be used.

It can be used as a tool by build systems to find and manage dependencies, this is notably the default Meson behavior and optionally used by CMake.


I Tested that the generated flatbuffers.pc was valid:

  • On the supplied commit 322a90d
    • By manually building/installing flatbuffers via Cmake, in a shell with cmake-3.14.5, then inspecting flatbuffers.pc manually.
    • In a Nix setup where flatbuffers is used by my external library — flatbuffers is found by Meson via pkg-config. Everything builds/runs correctly.
  • On top of last release v1.12.0, applying only 7e4124d and 322a90d as patches
    • In a Nix setup where flatbuffers is used by my external library — flatbuffers is found by Meson via pkg-config. Everything builds/runs correctly.

This commit enables CMake to generate a flatbuffers.pc file on install.

pkg-config eases the utilization of software libraries by enabling the
developers of a library to define how the library should be used.

It can be used as a tool by build systems to find and manage dependencies,
this is notably the default Meson behavior and optionally used by CMake.
@aardappel
Copy link
Collaborator

You forgot to mention which os/platform(s) need this file, and to do what :)

@mpoquet
Copy link
Contributor Author

mpoquet commented Feb 15, 2021

Oops :p. Strictly speaking this support is not needed by any platform, this is an improvement of the packaging/distribution of flatbuffers. Packages often contain pkg-config files in most Linux distros (examples below) and are used by build systems to externalize library retrieval and use. Personally I need pkg-config to find the flatbuffers C++ library to build my tools using Meson+Nix, which will enable me to deploy my tools on Linux and macOS.

Here are debian packages of serialization libraries packaged with pkg-config files:

@aardappel
Copy link
Collaborator

@vglavnyy opinion?

Note that in the case of FlatBuffers (moreso than Protobuf) distributing it thru linux distro packages is not great, since FlatBuffers depends heavily on flatc & runtime & generated code coming from the same version (commit), and distro packages being more long-lived things this easily creates problems. Ensuring you build flatc yourself as part of your CMake build is less error prone for example.

That said, not particularly for or against adding this file.

@vglavnyy
Copy link
Contributor

@aardappel
I don't have an opinion. I've never used pkg-config.

Note that in the case of FlatBuffers (moreso than Protobuf) distributing it thru linux distro packages is not great

Ubuntu 20.04 already has flatbuffers packages.

@aardappel
Copy link
Collaborator

Ok, looks like we should just merge this and see :)
Thanks!

@aardappel aardappel merged commit 1da6f4f into google:master Feb 22, 2021
@aardappel
Copy link
Collaborator

This PR is now making our CI fail, because it generates different paths on different OSes: https://ci.appveyor.com/project/aardappel/flatbuffers/builds/38015947/job/9ne7publivh24q4p

Maybe simply turn off generating of this file on Windows, or non-*nix?

vglavnyy added a commit to vglavnyy/flatbuffers that referenced this pull request Mar 6, 2021
Probably the generated flatbuffers.pc should not be a part of repo.
@vglavnyy
Copy link
Contributor

vglavnyy commented Mar 6, 2021

I think the generated flatbuffers.pc should be removed from the version control.
Even on Linux, install paths might differ from the paths from the current flatbuffers.pc file.
For example, protobuf project has only the template file protobuf.pc.in like the CMake/flatbuffers.pc.in.
@mpoquet what do you think?

@mpoquet
Copy link
Contributor Author

mpoquet commented Mar 8, 2021

I also think that flatbuffers.pc should not be in the git tree as its content depends on where end users want to install flatbuffers. My commit added a flatbuffers.pc.in file instead, so that the build system (CMake here) can generate a flatbuffers.pc file with correct content at install time (depending on CMAKE_INSTALL_PREFIX or other finer-grain installation path CMake variables if they are set).

aardappel pushed a commit that referenced this pull request Mar 11, 2021
* [idl_parser] Add kTokenNumericConstant token

This commit adds the new token for correct parsing of signed numeric constants.
Before this expressions `-nan` or `-inf` were treated as kTokenStringConstant.
This was ambiguous if a real string field parsed.
For example, `{ "text_field" : -name }` was accepted by the parser as valid JSON object.

Related oss-fuzz issue: 6200301176619008

* Add additional positive tests fo 'inf' and 'nan' as identifiers

* Rebase to HEAD

* Move processing of signed constants to ParseSingleValue method.

* Add missed `--cpp-static-reflection` (#6324) to pass CI

* Remove `flatbuffers.pc` from repository to unblock CI (#6455).

Probably the generated flatbuffers.pc should not be a part of repo.

* Fix FieldIdentifierTest()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants