Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Bazel build support #3709

Merged
merged 7 commits into from
Sep 18, 2022
Merged

Add Bazel build support #3709

merged 7 commits into from
Sep 18, 2022

Conversation

Vertexwahn
Copy link
Contributor

This PR helps users of the Bazel build system to integrate JSON for Modern C++ in their builds.

@gregmarr
Copy link
Contributor

See PR #1606 for a previous attempt that has questions that apply to this PR as well.

@coveralls
Copy link

coveralls commented Aug 23, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling 21f4fbb on Vertexwahn:add-bazel-support into 307c053 on nlohmann:develop.

@nlohmann
Copy link
Owner

Yes, please add documentation and check the linked PR.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUILD.bazel Outdated Show resolved Hide resolved
WORKSPACE.bazel Outdated Show resolved Hide resolved
Change workspace name

Fix typo in  WORKSPACE name

Try to generate Bazel BUILD file via CMake

Update README.md

Update README.md

Update CMakeLists.txt

Still trying to get CMake to generte a list of all JSON sourcfiles ...

Remove generated source file

Can generate a list of all files now...

Some progress

Update create_bazel_build_file.cmake

Delete nlohmann_json.bzl

fgd
@falbrechtskirchinger
Copy link
Contributor

I have a very clear picture of how this should work and how it should integrate with generating release artifacts, etc. This isn't it.
Specifically, I'd prefer a CMake script (i.e., a CMake file that can be invoked via cmake -P) that is tailored to our needs instead of adding Google's generalized CMake functions. Also, there's no need for a CMake option and generating the file during the build is only useful if we're going to install it. (Do we want that? What's the proper install directory? Can BUILD.bazel be made relocatable or do we have to hard-code absolute paths?)
We can decide later if we want to add the script to the amalgamation process and always keep it up-to-date or only update it manually. (I'd start with the latter and move to the former after the amalgamation stuff has been cleaned up.)

So here's my suggestion @Vertexwahn: If you finish the documentation as requested by Niels here (the relevant files are in docs/mkdocs/docs/integration/), I'll work on the CMake code for this PR (shouldn't require more than a handful of lines).

@Vertexwahn
Copy link
Contributor Author

@falbrechtskirchinger I added a Bazel paragraph to the Integration section. It would be nice if you could work on the CMake code. BTW: The Bazel files need not be installed. I can maintain the manual update of the Bazel files at least for the next 6 months.

@Vertexwahn
Copy link
Contributor Author

I added to the README as well as the integration documentation a section about Bazel

@Vertexwahn Vertexwahn requested review from falbrechtskirchinger and removed request for nlohmann September 10, 2022 12:04
@falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger I added a Bazel paragraph to the Integration section. It would be nice if you could work on the CMake code. BTW: The Bazel files need not be installed. I can maintain the manual update of the Bazel files at least for the next 6 months.

I've submitted a PR to your fork with the relevant changes. Please merge and we can continue here.

Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor whitespace issues.
Otherwise, looks good to me.

cmake/scripts/gen_bazel_build_file.cmake Outdated Show resolved Hide resolved
docs/mkdocs/docs/integration/package_managers.md Outdated Show resolved Hide resolved
Vertexwahn and others added 2 commits September 11, 2022 15:05
Co-authored-by: Florian Albrechtskirchinger <falbrechtskirchinger@gmail.com>
Co-authored-by: Florian Albrechtskirchinger <falbrechtskirchinger@gmail.com>
Co-authored-by: Florian Albrechtskirchinger <falbrechtskirchinger@gmail.com>
@Vertexwahn
Copy link
Contributor Author

@nlohmann Can it be merged now or is there any further action needed?

@falbrechtskirchinger
Copy link
Contributor

@Vertexwahn He's on vacation. I'm sure this will be merged shortly after he gets back.

@nlohmann nlohmann added this to the Release 3.11.3 milestone Sep 18, 2022
@nlohmann nlohmann merged commit 2d1f9b6 into nlohmann:develop Sep 18, 2022
@nlohmann
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants