-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 jsoncpp_test bazel configuration #1599
base: master
Are you sure you want to change the base?
Conversation
This patch adds a new configuration for jsoncpp_test in the BUILD.bazel file.
Pull Request Test Coverage Report for Build 13823356124Details
💛 - Coveralls |
@@ -22,10 +22,6 @@ cc_library( | |||
"include/json/version.h", | |||
"include/json/writer.h", | |||
], | |||
copts = [ | |||
"-DJSON_USE_EXCEPTION=0", |
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.
Removing this could be a breaking change for some users. I think a better way to do this would be: #1600
@@ -35,3 +31,22 @@ cc_library( | |||
name = "private", | |||
textual_hdrs = ["src/lib_json/json_valueiterator.inl"], | |||
) | |||
|
|||
cc_binary( |
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.
Both of these should likely be testonly=True
and should probably also be in BUILD.bazel
files in their respective directories.
deps = [":jsoncpp"], | ||
) | ||
|
||
cc_binary( |
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 think this could just be made to be a cc_test
rule. ...
And I got a little carried away check if that was true: #1601
(That said, I'm not 100% happy with how that came out if you want to take a run at making it cleaner.)
This patch adds a new configuration for jsoncpp_test in the BUILD.bazel file.