Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
322373002:
GN: Add BUILD.gn file to third_party/jsoncpp (Closed)
|
Created:
6 years, 6 months ago by kjellander_chromium Modified:
6 years, 6 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGN: Add BUILD.gn file to third_party/jsoncpp
BUG=webrtc:3441
TEST=Successful build with WebRTC, see
https://review.webrtc.org/17669004/ for details.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276676
Patch Set 1 : #
Total comments: 8
Patch Set 2 : Updated according to comments #
Total comments: 6
Patch Set 3 : Removed third_party/jsoncpp/BUILD.gn and moved jsoncpp BUILD.gn #Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/322373002/diff/20001/tools/gn/secondary/third... File tools/gn/secondary/third_party/jsoncpp/BUILD.gn (right): https://codereview.chromium.org/322373002/diff/20001/tools/gn/secondary/third... tools/gn/secondary/third_party/jsoncpp/BUILD.gn:12: static_library("jsoncpp") { is there a reason for using static_library? if not, then I thin our preference is to use source_set here. https://codereview.chromium.org/322373002/diff/20001/tools/gn/secondary/third... tools/gn/secondary/third_party/jsoncpp/BUILD.gn:16: defines = [ please, refert to http://code.google.com/p/chromium/wiki/GNStyleGuide#Ordering_within_a_target for how the ordering should be https://codereview.chromium.org/322373002/diff/20001/tools/gn/secondary/third... tools/gn/secondary/third_party/jsoncpp/BUILD.gn:24: sources = [ sources come before everything, move this at the top. https://codereview.chromium.org/322373002/diff/20001/tools/gn/secondary/third... tools/gn/secondary/third_party/jsoncpp/BUILD.gn:32: "overrides/include/json/value.h", could you sort this list.
Updated according to comments. Seems to build just fine for WebRTC purposes still. https://codereview.chromium.org/322373002/diff/20001/tools/gn/secondary/third... File tools/gn/secondary/third_party/jsoncpp/BUILD.gn (right): https://codereview.chromium.org/322373002/diff/20001/tools/gn/secondary/third... tools/gn/secondary/third_party/jsoncpp/BUILD.gn:12: static_library("jsoncpp") { On 2014/06/11 16:28:41, tfarina wrote: > is there a reason for using static_library? if not, then I thin our preference > is to use source_set here. No reason, just that it was static_library in the .gyp. source_set seems to work fine. https://codereview.chromium.org/322373002/diff/20001/tools/gn/secondary/third... tools/gn/secondary/third_party/jsoncpp/BUILD.gn:16: defines = [ On 2014/06/11 16:28:41, tfarina wrote: > please, refert to > http://code.google.com/p/chromium/wiki/GNStyleGuide#Ordering_within_a_target for > how the ordering should be Sorry about that, I'm new here ;) https://codereview.chromium.org/322373002/diff/20001/tools/gn/secondary/third... tools/gn/secondary/third_party/jsoncpp/BUILD.gn:24: sources = [ On 2014/06/11 16:28:41, tfarina wrote: > sources come before everything, move this at the top. Done. https://codereview.chromium.org/322373002/diff/20001/tools/gn/secondary/third... tools/gn/secondary/third_party/jsoncpp/BUILD.gn:32: "overrides/include/json/value.h", On 2014/06/11 16:28:41, tfarina wrote: > could you sort this list. Done.
https://codereview.chromium.org/322373002/diff/40001/build/config/linux/BUILD.gn File build/config/linux/BUILD.gn (right): https://codereview.chromium.org/322373002/diff/40001/build/config/linux/BUILD... build/config/linux/BUILD.gn:68: deps = [ "//net/third_party/nss/ssl:libssl" ] The rule is that stuff in src/build can't depend on anything outside of that for layering purposes. This is one of the reasons that all the crypto config was moved out of the build directory. If you need this and don't want to depend on /crypto (it's pretty small), you should probably duplicate this. https://codereview.chromium.org/322373002/diff/40001/tools/gn/secondary/third... File tools/gn/secondary/third_party/jsoncpp/BUILD.gn (right): https://codereview.chromium.org/322373002/diff/40001/tools/gn/secondary/third... tools/gn/secondary/third_party/jsoncpp/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. I think you can put this in the "real" directory in src/third_party/jsoncpp. The only reason to use the secondary tree at this point is for things DEPSed in. https://codereview.chromium.org/322373002/diff/40001/tools/gn/secondary/third... tools/gn/secondary/third_party/jsoncpp/BUILD.gn:30: configs += [ ":jsoncpp_config" ] You can delete this line. Direct dependent configs also apply to the current target.
Changed CL title and description to match what this evolved into. https://codereview.chromium.org/322373002/diff/40001/build/config/linux/BUILD.gn File build/config/linux/BUILD.gn (right): https://codereview.chromium.org/322373002/diff/40001/build/config/linux/BUILD... build/config/linux/BUILD.gn:68: deps = [ "//net/third_party/nss/ssl:libssl" ] On 2014/06/11 16:58:35, brettw wrote: > The rule is that stuff in src/build can't depend on anything outside of that for > layering purposes. This is one of the reasons that all the crypto config was > moved out of the build directory. If you need this and don't want to depend on > /crypto (it's pretty small), you should probably duplicate this. OK, I'll duplicate this in WebRTC for now. I don't know enough about openssl/nss and all that to refactor those things or introduce a dependency on src/crypto right now. I just wanted to provide a close-to-GYP solution for now. I'll add it to https://review.webrtc.org/17669004 instead. https://codereview.chromium.org/322373002/diff/40001/tools/gn/secondary/third... File tools/gn/secondary/third_party/jsoncpp/BUILD.gn (right): https://codereview.chromium.org/322373002/diff/40001/tools/gn/secondary/third... tools/gn/secondary/third_party/jsoncpp/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2014/06/11 16:58:35, brettw wrote: > I think you can put this in the "real" directory in src/third_party/jsoncpp. The > only reason to use the secondary tree at this point is for things DEPSed in. Ah, of course. I didn't think about that there was a directory in Chrome and that just most of the source was pulled via DEPS. https://codereview.chromium.org/322373002/diff/40001/tools/gn/secondary/third... tools/gn/secondary/third_party/jsoncpp/BUILD.gn:30: configs += [ ":jsoncpp_config" ] On 2014/06/11 16:58:35, brettw wrote: > You can delete this line. Direct dependent configs also apply to the current > target. Done.
lgtm
The CQ bit was checked by kjellander@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kjellander@chromium.org/322373002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15636) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18773)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15712) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18845)
The CQ bit was checked by kjellander@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kjellander@chromium.org/322373002/60001
Message was sent while issue was closed.
Change committed as 276676 |
