close
Skip to content

Add Bazel build support#3709

Merged
nlohmann merged 7 commits into
nlohmann:developfrom
Vertexwahn:add-bazel-support
Sep 18, 2022
Merged

Add Bazel build support#3709
nlohmann merged 7 commits into
nlohmann:developfrom
Vertexwahn:add-bazel-support

Conversation

@Vertexwahn
Copy link
Copy Markdown
Contributor

@Vertexwahn Vertexwahn commented Aug 22, 2022

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

@Vertexwahn Vertexwahn requested a review from nlohmann as a code owner August 22, 2022 21:52
@github-actions github-actions Bot added the M label Aug 22, 2022
@gregmarr
Copy link
Copy Markdown
Contributor

gregmarr commented Aug 22, 2022

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

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 23, 2022 •
edited
Loading

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
Copy Markdown
Owner

nlohmann commented Aug 26, 2022

Yes, please add documentation and check the linked PR.

nlohmann
nlohmann requested changes Aug 27, 2022
View reviewed changes
Copy link
Copy Markdown
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.

@Vertexwahn Vertexwahn mentioned this pull request Sep 4, 2022
Closed
falbrechtskirchinger
falbrechtskirchinger suggested changes Sep 4, 2022
View reviewed changes
Comment thread BUILD.bazel Outdated
Comment thread WORKSPACE.bazel Outdated
@Vertexwahn
Add Bazel build support
5a4d47e 
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
@Vertexwahn Vertexwahn requested a review from nlohmann September 5, 2022 06:27
@falbrechtskirchinger falbrechtskirchinger mentioned this pull request Sep 7, 2022
Merged
@falbrechtskirchinger
Copy link
Copy Markdown
Contributor

falbrechtskirchinger commented Sep 7, 2022

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
Copy Markdown
Contributor Author

Vertexwahn commented Sep 7, 2022

@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
Copy Markdown
Contributor Author

Vertexwahn commented Sep 8, 2022

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
Copy Markdown
Contributor

falbrechtskirchinger commented Sep 11, 2022

@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.

falbrechtskirchinger
falbrechtskirchinger approved these changes Sep 11, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread cmake/scripts/gen_bazel_build_file.cmake Outdated
Comment thread docs/mkdocs/docs/integration/package_managers.md Outdated
Vertexwahn and others added 2 commits September 11, 2022 15:05
@Vertexwahn @falbrechtskirchinger
Update cmake/scripts/gen_bazel_build_file.cmake
84f1fe8 
Co-authored-by: Florian Albrechtskirchinger <falbrechtskirchinger@gmail.com>
@Vertexwahn @falbrechtskirchinger
Update docs/mkdocs/docs/integration/package_managers.md
da7dbbc 
Co-authored-by: Florian Albrechtskirchinger <falbrechtskirchinger@gmail.com>
@Vertexwahn Vertexwahn requested a review from nlohmann September 11, 2022 20:09
falbrechtskirchinger
falbrechtskirchinger reviewed Sep 13, 2022
View reviewed changes
Comment thread CMakeLists.txt Outdated
@Vertexwahn @falbrechtskirchinger
Update CMakeLists.txt
21f4fbb 
Co-authored-by: Florian Albrechtskirchinger <falbrechtskirchinger@gmail.com>
@Vertexwahn
Copy link
Copy Markdown
Contributor Author

Vertexwahn commented Sep 16, 2022

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

@falbrechtskirchinger
Copy link
Copy Markdown
Contributor

falbrechtskirchinger commented Sep 17, 2022

@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
Copy Markdown
Owner

nlohmann commented Sep 18, 2022

Thanks!

@fmeum fmeum mentioned this pull request Nov 1, 2022
Closed
4 tasks
@nlohmann nlohmann mentioned this pull request Nov 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nlohmann nlohmann Awaiting requested review from nlohmann nlohmann is a code owner

1 more reviewer

@falbrechtskirchinger falbrechtskirchinger falbrechtskirchinger approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CMake documentation M

Projects

None yet

Milestone

Release 3.11.3

Development

Successfully merging this pull request may close these issues.

5 participants

@Vertexwahn @gregmarr @coveralls @nlohmann @falbrechtskirchinger