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 #1606
add bazel build #1606
Conversation
Thanks for the PR! Could you also add a small paragraph to the README file how to use the package? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add bazel build usage and demo project.
@@ -147,6 +147,84 @@ endif() | |||
|
|||
`thirdparty/nlohmann_json` is then a complete copy of this source tree. | |||
|
|||
### Bazel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extensive example! What I had in mind, though, was a paragraph like for the package managers in the "Package Managers" section below. Just enough to let people knowing Bazel understand what they need to do to use this library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these instructions pertain more to developers, it would probably be better to just download the single header release, not build the entire thing from source. Like so:
http_file(
name = "com_github_nlohmann_json_single_header",
urls = [
"https://github.com/nlohmann/json/releases/download/v3.6.1/json.hpp",
],
sha256 = "blah_blah_blah",
)
@@ -0,0 +1 @@ | |||
workspace(name = "nlohmann_json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file really needed in this repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is - otherwise, this is not recognized by Bazel - I named the file WORKSPCE.bazel
(which is an alternative to WORKSPACE only) to make it more clear that this is a Bazel thing -> see my PR: #3709
@horance-liu Can you please have a look at the changes I requested? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
cc_library( | ||
name = "json", | ||
hdrs = glob([ | ||
"include/**/*.hpp", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs: "This pattern should be avoided if possible, as it can reduce build incrementality and therefore increase build times." A better approach would be to generate the list of sources and headers from CMake (that way, you don't need to repeat all of it in Bazel), i.e: https://github.com/googleapis/google-cloud-cpp/blob/8e8dd3d83ec27b7a5176f9fe43cdd93aa5ace3dc/cmake/CreateBazelConfig.cmake#L43
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this in #3709
@@ -147,6 +147,84 @@ endif() | |||
|
|||
`thirdparty/nlohmann_json` is then a complete copy of this source tree. | |||
|
|||
### Bazel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these instructions pertain more to developers, it would probably be better to just download the single header release, not build the entire thing from source. Like so:
http_file(
name = "com_github_nlohmann_json_single_header",
urls = [
"https://github.com/nlohmann/json/releases/download/v3.6.1/json.hpp",
],
sha256 = "blah_blah_blah",
)
} | ||
``` | ||
|
||
NOTE: you should use double quotation marks instead of angle brackets. You can see details at [horance-liu/nlohmann_json_demo](https://github.com/horance-liu/nlohmann_json_demo) project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the demo and didn't see any details about why quotation marks are needed here. Can you explain?
@remyabel Thanks for your comments! As the PR got stale and you seem to be familiar with the topic: would you be able to take over the PR or create a new one with all comments (including yours) addressed? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
User can depends nlohmann json directly using bazel, but this commit don't support test all cases using bazel.