Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1930173003:
Reland: Split protoc into library and executable. (Closed)
DescriptionReland: Split protoc into library and executable.
Similarly to the upstream BUILD file, we split the protoc compiler into
a library and executable. The purpose is to allow protoc compiler
plugins (see plugin.h).
This was initially submitted as https://codereview.chromium.org/1923733002. In this reland I add the sanitizer deps I removed I thought were
unnecessary. They weren't.
BUG=597321
Committed: https://crrev.com/ce0e58ddfd41c1382eea24cb92ee4caf84c7765e
Cr-Commit-Position: refs/heads/master@{#390546}
Patch Set 1 #
Total comments: 10
Patch Set 2 : comment #
Messages
Total messages: 21 (9 generated)
xyzzyz@chromium.org changed reviewers: + marcinjb@chromium.org
Description was changed from ========== Reland: Split protoc into library and executable. Similarly to the upstream BUILD file, we split the protoc compiler into a library and executable. The purpose is to allow protoc compiler plugins (see plugin.h). In this reland I add the sanitizer deps I removed I though were unnecessary. They weren't. BUG=597321 ========== to ========== Reland: Split protoc into library and executable. Similarly to the upstream BUILD file, we split the protoc compiler into a library and executable. The purpose is to allow protoc compiler plugins (see plugin.h). This was initially submitted as https://codereview.chromium.org/1923733002. In this reland I add the sanitizer deps I removed I thought were unnecessary. They weren't. BUG=597321 ==========
lgtm
The CQ bit was checked by xyzzyz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1930173003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1930173003/1
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
wez@chromium.org changed reviewers: + wez@chromium.org
https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.gn File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:310: "//build/config/sanitizers:deps", So there is no GYP equivalent of this required? Does this definitely have the desired effect? Previously this deps was at the executable level, not the source-set, so I'd expect to still see it in the executable's deps, not here. https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:336: source_set("protoc_lib") { nit: protoc_sources, since it's a source-set? It might be handy to provide a brief comment explaining the split, even if all it does is refer to the bug # for that. https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:549: configs += [ "//build/config/compiler:no_chromium_code" ] I assume that these configs affect the protoc_lib as well, since its a source-set rather than static-library? https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/protob... File third_party/protobuf/protobuf.gyp (right): https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/protob... third_party/protobuf/protobuf.gyp:438: '-Wno-unused-function', Why are they unused? Do we have a bug filed against protobuf to address that..?
https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.gn File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:310: "//build/config/sanitizers:deps", On 2016/04/28 22:46:05, Wez wrote: > So there is no GYP equivalent of this required? > > Does this definitely have the desired effect? Previously this deps was at the > executable level, not the source-set, so I'd expect to still see it in the > executable's deps, not here. It's this target that uses sanitizer interface, not the executable, and the purpose of this CL is to create a separate target so that I can add the gRPC target that would depend on protoc_lib, but not on protoc itself. The way it works is by adding some public_configs, so all targets depending on this will also depend on the sanitizer stuff. I think it's not necessary in gyp, because they set up asan in a completely different way. https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:336: source_set("protoc_lib") { On 2016/04/28 22:46:05, Wez wrote: > nit: protoc_sources, since it's a source-set? > > It might be handy to provide a brief comment explaining the split, even if all > it does is refer to the bug # for that. I'd rather keep it protoc_lib, to match the name in upstream BUILD. I'll add the comment. https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:549: configs += [ "//build/config/compiler:no_chromium_code" ] On 2016/04/28 22:46:05, Wez wrote: > I assume that these configs affect the protoc_lib as well, since its a > source-set rather than static-library? Sorry, I don't quite understand your question.
https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.gn File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:549: configs += [ "//build/config/compiler:no_chromium_code" ] On 2016/04/28 22:56:56, xyzzyz wrote: > On 2016/04/28 22:46:05, Wez wrote: > > I assume that these configs affect the protoc_lib as well, since its a > > source-set rather than static-library? > > Sorry, I don't quite understand your question. These are compiler configurations - if you were building a static-library then you'd need the configs in the library declaration as well as here in the executable declaration. I'm just asking you to confirm that source_set works differently from static_library in that regard, and uses the compiler configuration of the embedding library or exe?
https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.gn File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:549: configs += [ "//build/config/compiler:no_chromium_code" ] On 2016/04/28 23:00:04, Wez wrote: > On 2016/04/28 22:56:56, xyzzyz wrote: > > On 2016/04/28 22:46:05, Wez wrote: > > > I assume that these configs affect the protoc_lib as well, since its a > > > source-set rather than static-library? > > > > Sorry, I don't quite understand your question. > > These are compiler configurations - if you were building a static-library then > you'd need the configs in the library declaration as well as here in the > executable declaration. > > I'm just asking you to confirm that source_set works differently from > static_library in that regard, and uses the compiler configuration of the > embedding library or exe? I'm not sure, I think they should work mostly the same except the static_library also packs the objects to *.a file. These compiler configs mostly refer to warnings level in the compiler, and I in fact have them both here and in the library (source_set) declaration.
The CQ bit was checked by wez@chromium.org
lgtm https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.gn File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/1930173003/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:549: configs += [ "//build/config/compiler:no_chromium_code" ] On 2016/04/28 23:07:39, xyzzyz wrote: > On 2016/04/28 23:00:04, Wez wrote: > > On 2016/04/28 22:56:56, xyzzyz wrote: > > > On 2016/04/28 22:46:05, Wez wrote: > > > > I assume that these configs affect the protoc_lib as well, since its a > > > > source-set rather than static-library? > > > > > > Sorry, I don't quite understand your question. > > > > These are compiler configurations - if you were building a static-library then > > you'd need the configs in the library declaration as well as here in the > > executable declaration. > > > > I'm just asking you to confirm that source_set works differently from > > static_library in that regard, and uses the compiler configuration of the > > embedding library or exe? > > I'm not sure, I think they should work mostly the same except the static_library > also packs the objects to *.a file. > > These compiler configs mostly refer to warnings level in the compiler, and I in > fact have them both here and in the library (source_set) declaration. > Gah! codereview caught me out again by eliding unchanged lines *facepalm* Ahem, I'll just go and hit that LGTM button...
The patchset sent to the CQ was uploaded after l-g-t-m from marcinjb@chromium.org Link to the patchset: https://codereview.chromium.org/1930173003/#ps20001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1930173003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1930173003/20001
Message was sent while issue was closed.
Description was changed from ========== Reland: Split protoc into library and executable. Similarly to the upstream BUILD file, we split the protoc compiler into a library and executable. The purpose is to allow protoc compiler plugins (see plugin.h). This was initially submitted as https://codereview.chromium.org/1923733002. In this reland I add the sanitizer deps I removed I thought were unnecessary. They weren't. BUG=597321 ========== to ========== Reland: Split protoc into library and executable. Similarly to the upstream BUILD file, we split the protoc compiler into a library and executable. The purpose is to allow protoc compiler plugins (see plugin.h). This was initially submitted as https://codereview.chromium.org/1923733002. In this reland I add the sanitizer deps I removed I thought were unnecessary. They weren't. BUG=597321 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ce0e58ddfd41c1382eea24cb92ee4caf84c7765e Cr-Commit-Position: refs/heads/master@{#390546}
Message was sent while issue was closed.
Description was changed from ========== Reland: Split protoc into library and executable. Similarly to the upstream BUILD file, we split the protoc compiler into a library and executable. The purpose is to allow protoc compiler plugins (see plugin.h). This was initially submitted as https://codereview.chromium.org/1923733002. In this reland I add the sanitizer deps I removed I thought were unnecessary. They weren't. BUG=597321 ========== to ========== Reland: Split protoc into library and executable. Similarly to the upstream BUILD file, we split the protoc compiler into a library and executable. The purpose is to allow protoc compiler plugins (see plugin.h). This was initially submitted as https://codereview.chromium.org/1923733002. In this reland I add the sanitizer deps I removed I thought were unnecessary. They weren't. BUG=597321 Committed: https://crrev.com/ce0e58ddfd41c1382eea24cb92ee4caf84c7765e Cr-Commit-Position: refs/heads/master@{#390546} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
