Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2762193002:
[protobuf] Fix potential double definition build errors. (Closed)
|
Created:
3 years, 9 months ago by Eric Seckler Modified:
3 years, 9 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[protobuf] Fix potential double definition build errors.
https://codereview.chromium.org/2756543002 introduced a protobuf_globals
target, whose sources include stubs/atomicops_internals_x86_gcc.cc.
Since this source file is also included in the protobuf_lite target,
some builds may complain about double definitions.
BUG=700120
Review-Url: https://codereview.chromium.org/2762193002
Cr-Commit-Position: refs/heads/master@{#458725}
Committed: https://chromium.googlesource.com/chromium/src/+/396d7f89575a83e9d8b23a609acf60bb383fbaf1
Patch Set 1 #
Total comments: 5
Patch Set 2 : add protobuf_globals_sources #Messages
Total messages: 22 (13 generated)
Description was changed from ========== [protobuf] Fix potential double definition build errors. https://codereview.chromium.org/2756543002 introduced a protobuf_global target, whose sources include stubs/atomicops_internals_x86_gcc.cc. Since this source file is also included in the protobuf_lite target, some builds may complain about double definitions. BUG=700120 ========== to ========== [protobuf] Fix potential double definition build errors. https://codereview.chromium.org/2756543002 introduced a protobuf_globals target, whose sources include stubs/atomicops_internals_x86_gcc.cc. Since this source file is also included in the protobuf_lite target, some builds may complain about double definitions. BUG=700120 ==========
eseckler@chromium.org changed reviewers: + pkasting@chromium.org, thomasanderson@google.com
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2762193002/diff/1/third_party/protobuf/BUILD.gn File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/2762193002/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:202: "src/google/protobuf/globals.cc", it would probably be better to define protobuf_globals_sources that contains these 2 files, and add it here and in protobuf_full https://codereview.chromium.org/2762193002/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:227: sources += [ while we're here, can you change this to be: sources = protobuf_lite_sources + protobuf_globals_sources + [ ...
LGTM with Tom's proposed changes. https://codereview.chromium.org/2762193002/diff/1/third_party/protobuf/BUILD.gn File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/2762193002/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:202: "src/google/protobuf/globals.cc", On 2017/03/21 18:06:21, Tom Anderson wrote: > it would probably be better to define protobuf_globals_sources that contains > these 2 files, and add it here and in protobuf_full Yes. This way we won't list these in multiple different places which are easy to not have in sync (as in the case of this bug).
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by eseckler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, thomasanderson@google.com Link to the patchset: https://codereview.chromium.org/2762193002/#ps40001 (title: "add protobuf_globals_sources")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2762193002/diff/1/third_party/protobuf/BUILD.gn File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/2762193002/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:202: "src/google/protobuf/globals.cc", On 2017/03/21 22:39:12, Peter Kasting wrote: > On 2017/03/21 18:06:21, Tom Anderson wrote: > > it would probably be better to define protobuf_globals_sources that contains > > these 2 files, and add it here and in protobuf_full > > Yes. This way we won't list these in multiple different places which are easy > to not have in sync (as in the case of this bug). Done. https://codereview.chromium.org/2762193002/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:227: sources += [ On 2017/03/21 18:06:21, Tom Anderson wrote: > while we're here, can you change this to be: > sources = protobuf_lite_sources + protobuf_globals_sources + [ ... Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by eseckler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490187146457990,
"parent_rev": "da9b0f18fccbbce9b59abaef3ad0d8ad1ebcd1f5", "commit_rev":
"396d7f89575a83e9d8b23a609acf60bb383fbaf1"}
Message was sent while issue was closed.
Description was changed from ========== [protobuf] Fix potential double definition build errors. https://codereview.chromium.org/2756543002 introduced a protobuf_globals target, whose sources include stubs/atomicops_internals_x86_gcc.cc. Since this source file is also included in the protobuf_lite target, some builds may complain about double definitions. BUG=700120 ========== to ========== [protobuf] Fix potential double definition build errors. https://codereview.chromium.org/2756543002 introduced a protobuf_globals target, whose sources include stubs/atomicops_internals_x86_gcc.cc. Since this source file is also included in the protobuf_lite target, some builds may complain about double definitions. BUG=700120 Review-Url: https://codereview.chromium.org/2762193002 Cr-Commit-Position: refs/heads/master@{#458725} Committed: https://chromium.googlesource.com/chromium/src/+/396d7f89575a83e9d8b23a609acf... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/396d7f89575a83e9d8b23a609acf... |
