Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
75213003:
Add libc++ and libc++abi to third-party. (Closed)
|
Created:
7 years, 1 month ago by alextaran1 Modified:
7 years ago Reviewers:
Alexander Potapenko, bradn, cpu_(ooo_6.6-7.5), bradnelson, Nico, Paweł Hajdan Jr., kcc2, darin (slow to review) CC:
chromium-reviews, jshin+watch_chromium.org, feature-media-reviews_chromium.org, erikwright+watch_chromium.org Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd libc++ and libc++abi to third-party.
BUG=318770, 313751
R=glider@chromium.org,thakis@chromium.org,bradnelson@chromium.org,darin@chromium.org,phajdan@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240682
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241574
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242088
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 13
Patch Set 6 : Add libc++ and libc++abi to third-party. #Patch Set 7 : #Patch Set 8 : Add libc++ and libc++abi to third-party. #
Total comments: 2
Patch Set 9 : #
Total comments: 14
Patch Set 10 : #
Total comments: 4
Patch Set 11 : #
Total comments: 3
Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #
Messages
Total messages: 58 (0 generated)
Please take a look
Please add a bug ID to the CL description.
https://codereview.chromium.org/75213003/diff/840027/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/75213003/diff/840027/build/common.gypi#newcod... build/common.gypi:903: 'libcxx%': '<(libcxx)', I suggest "use_libcxx" instead, this is a common prefix in common.gypi https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/libcx... File third_party/libcxx/libcxx.gyp (right): https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/libcx... third_party/libcxx/libcxx.gyp:45: 'include', This should be relative to <(DEPTH) https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/libcx... third_party/libcxx/libcxx.gyp:46: '../libcxxabi/include', Ditto. https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/libcx... third_party/libcxx/libcxx.gyp:49: '-g', '-Os', '-fPIC', Is there any upstream build config, doc or script that contains these flags? Can you add a link here? https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/libcx... third_party/libcxx/libcxx.gyp:67: 'include', Must be relative to <(DEPTH) https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/libcx... third_party/libcxx/libcxx.gyp:77: '-o libc++.so.1.0', Why do you need this one? https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/libcx... third_party/libcxx/libcxx.gyp:78: '-shared', -shared should be implied by the target type. https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/libcx... third_party/libcxx/libcxx.gyp:86: '-lcxxabi', Provided that you have libcxxabi in the dependencies, is -lcxxabi required?
https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/.arcc... File third_party/libcxx/.arcconfig (right): https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/.arcc... third_party/libcxx/.arcconfig:3: "conduit_uri" : "http://llvm-reviews.chandlerc.com/" Please remove this file
https://codereview.chromium.org/75213003/diff/840027/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/75213003/diff/840027/build/common.gypi#newcod... build/common.gypi:3441: '<(DEPTH)/third_party/libcxxabi/libcxxabi.gyp:libcxxabi', Rather then inject the dependency into everything, can you attach it to some low level target like base? Using all_dependent_settings (which you're already doing) propagates the includes through the transitive dependencies. As long as you pick the non-nacl version of whatever root target it won't spill over into nacl. https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/libcx... File third_party/libcxx/libcxx.gyp (right): https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/libcx... third_party/libcxx/libcxx.gyp:61: 'all_dependent_settings': { So if you use all_dependent_settings here you shouldn't need to inject it all over the place. See other comment. https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/libcx... third_party/libcxx/libcxx.gyp:63: '-lcxx', You shouldn't need to do this as you're building this yourself and normal dependency propagation should add this. https://codereview.chromium.org/75213003/diff/840027/third_party/libcxx/libcx... third_party/libcxx/libcxx.gyp:67: 'include', In a gyp file this should be ok without <(DEPTH) actually, actually.
Brad, the actual flags passed to the compiler differ a bit. Alex will upload the latest version on Monday. Thanks for your suggestions, we'll review the possibility of having a single target depend on libcxx. On Nov 22, 2013 9:49 PM, <bradnelson@google.com> wrote: > > https://codereview.chromium.org/75213003/diff/840027/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/75213003/diff/840027/ > build/common.gypi#newcode3441 > build/common.gypi:3441: > '<(DEPTH)/third_party/libcxxabi/libcxxabi.gyp:libcxxabi', > Rather then inject the dependency into everything, can you attach it to > some low level target like base? Using all_dependent_settings (which > you're already doing) propagates the includes through the transitive > dependencies. As long as you pick the non-nacl version of whatever root > target it won't spill over into nacl. > > https://codereview.chromium.org/75213003/diff/840027/ > third_party/libcxx/libcxx.gyp > File third_party/libcxx/libcxx.gyp (right): > > https://codereview.chromium.org/75213003/diff/840027/ > third_party/libcxx/libcxx.gyp#newcode61 > third_party/libcxx/libcxx.gyp:61: 'all_dependent_settings': { > So if you use all_dependent_settings here you shouldn't need to inject > it all over the place. See other comment. > > https://codereview.chromium.org/75213003/diff/840027/ > third_party/libcxx/libcxx.gyp#newcode63 > third_party/libcxx/libcxx.gyp:63: '-lcxx', > You shouldn't need to do this as you're building this yourself and > normal dependency propagation should add this. > > https://codereview.chromium.org/75213003/diff/840027/ > third_party/libcxx/libcxx.gyp#newcode67 > third_party/libcxx/libcxx.gyp:67: 'include', > In a gyp file this should be ok without <(DEPTH) actually, actually. > > https://codereview.chromium.org/75213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Add libc++ and libc++abi to third-party. BUG=318770, 313751 R=glider@chromium.org,thakis@chromium.org,bradnelson@chromium.org TBR=cpu@chromium.org
(if i'm supposed to look at anything here, please say what. as is, i got an email with nothing but "https://codereview.chromium.org/75213003/" in it) On Mon, Nov 25, 2013 at 2:23 AM, <alextaran@chromium.org> wrote: > https://codereview.chromium.org/75213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
looks like a new library. Please remove the tbr=cpu. Has this library license been approved? Also adding darin@ since this seems to be an impactfull change.
To be clear, this change isn't going to have much impact, because use_libcxx will be off by default. But another pair of eyes will be great. The main goal of this change is to allow ASan, TSan and MSan build Chrome with libc++, however someday we may want to use libc++ instead of libstdc++ in other build configurations. We haven't picked the appropriate reviewers yet, because the CL is still a WIP. Alex is trying to figure out how to remove the libc++ dependency from NaCl (we're leaning towards Brad's suggestion for now) and will hopefully upload the working version today. AFAIK this license has been approved, "University of Illinois/NCSA Open Source License" is present in licensecheck.pl and there're other pieces of code with the same license in Chromium.
Please take a look. gyp-files: https://codereview.chromium.org/75213003/patch/2690001/5737979670691840 https://codereview.chromium.org/75213003/patch/2690001/5741031244955648 https://codereview.chromium.org/75213003/patch/2690001/6276093975724032 Unfortunately, there exists target "tracing_untrusted" which should be built without libc++, but it depends on target "ipc/ipc.gyp:ipc" which depends on target "base/base.gyp:base" :( There are several other targets which seem doing the same. Is it possible to add libc++ as dependency for all targets except several ones? I've tried several things but in general it seems that it's not possible in "common.gypi" to detect which target gyp is processing because "common.gypi" is included first to all other gyp-files. Now it works with target "base_unittests". Also, it works with "chrome" only if variable disable_nacl is set to 1
Add libc++ and libc++abi to third-party. BUG=318770, 313751 R=glider@chromium.org,thakis@chromium.org,bradnelson@chromium.org,darin@chrom...
Added dependency excluding for untrusted targets, so now chome with libc++ can be built. Please take a look.
https://codereview.chromium.org/75213003/diff/2710001/media/media_untrusted.gyp File media/media_untrusted.gyp (right): https://codereview.chromium.org/75213003/diff/2710001/media/media_untrusted.g... media/media_untrusted.gyp:45: '<(DEPTH)/third_party/libc++abi/libc++abi.gyp:libc++abi', I think it would be good to avoid requiring this in every untrusted target (which are an unbounded set), if at all possible. I'm going to suggest an idea for how in another location for context. https://codereview.chromium.org/75213003/diff/2710001/third_party/libc++/libc... File third_party/libc++/libc++.gyp (right): https://codereview.chromium.org/75213003/diff/2710001/third_party/libc++/libc... third_party/libc++/libc++.gyp:55: 'include_dirs': [ Rather than have to inject boilerplate into each nacl target. You should be able to reliably wrap these flags and include_dirs in a target_condition (which will enable them only in relevant targets): 'target_conditions': [ ['_type!="none"', { includes.... flags... ], }], By avoiding the none targets you should be able to avoid injecting flags into the nacl parts of the build without having to call them out explicitly. There's a couple of options for the condition, I'm not sure which is ideal: - _type!="none" (avoid none targets which either don't need the flags or are nacl targets) - nacl_untrusted_build==0 (just avoid the nacl targets) I would lean towards the first, because some nacl targets do not use the nacl_untrusted_build flag (typically these would have nlib_target!=""). I'm a little surprised you didn't hit these targets.
I don't see the open source license reviewer in the review.
On 2013/12/06 23:50:09, cpu wrote: > I don't see the open source license reviewer in the review. Carlos, do you mean Pawel Hajdan, who's maintaining licensecheck, or someone else?
Any reason why this isn't pulled in through DEPS, so that this CL only contains a DEPS entry, a README.chromium, the LICENSE, and a gyp file? Then it'd be more obvious that there are no downstream diffs. (If you go with DEPS, you need to file an infra ticket to set up an llvm svn mirror if we don't have one yet, see sourceforge_url in DEPS. But it's possible we already have a mirror somewhere.) On 2013/12/07 09:10:14, Alexander Potapenko wrote: > On 2013/12/06 23:50:09, cpu wrote: > > I don't see the open source license reviewer in the review. > > Carlos, do you mean Pawel Hajdan, who's maintaining licensecheck, or someone > else? He probably means the open-source-third-party-reviews@google.com bit from http://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review
On Sun, Dec 8, 2013 at 12:52 PM, <thakis@chromium.org> wrote: > Any reason why this isn't pulled in through DEPS, so that this CL only > contains > a DEPS entry, a README.chromium, the LICENSE, and a gyp file? Then it'd be > more > obvious that there are no downstream diffs. (If you go with DEPS, you need > to > file an infra ticket to set up an llvm svn mirror if we don't have one > yet, see > sourceforge_url in DEPS. But it's possible we already have a mirror > somewhere.) (Yes we do, see LLVM_URL in cs. It's set to the mirror on the bots.) > > > On 2013/12/07 09:10:14, Alexander Potapenko wrote: > >> On 2013/12/06 23:50:09, cpu wrote: >> > I don't see the open source license reviewer in the review. >> > > Carlos, do you mean Pawel Hajdan, who's maintaining licensecheck, or >> someone >> else? >> > > He probably means the open-source-third-party-reviews@google.com bit from > http://www.chromium.org/developers/adding-3rd-party- > libraries#TOC-Get-a-Review > > https://codereview.chromium.org/75213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
But we don't need the eng review.
(We probably don't need the license review either, given that this is covered by the same license as the rest of llvm which is already heavily used in many places. But I guess that means the legal review will be very quick, so if cpu wants it, might as well do it.) On Sun, Dec 8, 2013 at 1:09 PM, <cpu@chromium.org> wrote: > But we don't need the eng review. > > https://codereview.chromium.org/75213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'll send out the letters for legal and security reviews (hope they'll be really quick), but I wonder if we actually need the legal approval if we use DEPS to pull this code. http://www.chromium.org/developers/adding-3rd-party-libraries doesn't clarify that.
Added libc++ and libc++abi to DEPS. Removed dependency-excluding from untrusted targets. Added OWNERS. Please take a look.
https://codereview.chromium.org/75213003/diff/2720001/DEPS File DEPS (right): https://codereview.chromium.org/75213003/diff/2720001/DEPS#newcode117 DEPS:117: Var("llvm_url") + "/libcxx/trunk", We must pin fixed versions of libc++ and libc++abi. https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++/READ... File third_party/libc++/README.chromium (right): https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++/READ... third_party/libc++/README.chromium:7: Security Critical: yes I don't think this is security critical for now, but let us wait for the security team. https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++abi/R... File third_party/libc++abi/README.chromium (right): https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++abi/R... third_party/libc++abi/README.chromium:7: Security Critical: yes Ditto
Any good reason to call this libc++, not libcxx?
Also, when building with this patch and use_libcxx=1 I can see ninja: Entering directory `out/Release' [448/762] SOLINK lib/libc++.so clang: warning: argument unused during compilation: '-pthread' [713/762] SOLINK lib/libc++abi.so clang: warning: argument unused during compilation: '-pthread' [762/762] LINK url_unittests
Thanks! Make sure that llvm_url points to the internal mirror on the bots (I think all that requires is adding something like CUSTOM_VARS_SOURCEFORGE_URL but for llvm to build/scripts/master/factory/chromium_factory.py – config.Master.llvm_url does already exist and contains the right value). https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++/READ... File third_party/libc++/README.chromium (right): https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++/READ... third_party/libc++/README.chromium:7: Security Critical: yes On 2013/12/09 13:52:56, Alexander Potapenko wrote: > I don't think this is security critical for now, but let us wait for the > security team. Yes, "yes" means "used in the shipping product" https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++/libc... File third_party/libc++/libc++.gyp (right): https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++/libc... third_party/libc++/libc++.gyp:15: 'trunk/src/algorithm.cpp', Normally, "trunk" isn't included in the checkout. Can you change the DEPS entry so that this includes "src/ios.cpp" etc? https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++abi/l... File third_party/libc++abi/libc++abi.gyp (right): https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++abi/l... third_party/libc++abi/libc++abi.gyp:31: 'trunk/src/typeinfo.cpp', Do you need the Unwind stuff too? (and the same comment about trunk) https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++abi/l... third_party/libc++abi/libc++abi.gyp:75: '-lgcc_s', Do you still need to link against libgcc?
Fixed. Please take a look. https://codereview.chromium.org/75213003/diff/2720001/DEPS File DEPS (right): https://codereview.chromium.org/75213003/diff/2720001/DEPS#newcode117 DEPS:117: Var("llvm_url") + "/libcxx/trunk", On 2013/12/09 13:52:56, Alexander Potapenko wrote: > We must pin fixed versions of libc++ and libc++abi. Done. https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++/READ... File third_party/libc++/README.chromium (right): https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++/READ... third_party/libc++/README.chromium:7: Security Critical: yes On 2013/12/09 16:18:55, Nico wrote: > On 2013/12/09 13:52:56, Alexander Potapenko wrote: > > I don't think this is security critical for now, but let us wait for the > > security team. > > Yes, "yes" means "used in the shipping product" Done. https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++/libc... File third_party/libc++/libc++.gyp (right): https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++/libc... third_party/libc++/libc++.gyp:15: 'trunk/src/algorithm.cpp', On 2013/12/09 16:18:55, Nico wrote: > Normally, "trunk" isn't included in the checkout. Can you change the DEPS entry > so that this includes "src/ios.cpp" etc? Done. https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++abi/R... File third_party/libc++abi/README.chromium (right): https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++abi/R... third_party/libc++abi/README.chromium:7: Security Critical: yes On 2013/12/09 13:52:56, Alexander Potapenko wrote: > Ditto Done. https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++abi/l... File third_party/libc++abi/libc++abi.gyp (right): https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++abi/l... third_party/libc++abi/libc++abi.gyp:31: 'trunk/src/typeinfo.cpp', This Unwind stuff has declarations under #ifdef __APPLE__, so on linux it's uncompileable. On 2013/12/09 16:18:55, Nico wrote: > Do you need the Unwind stuff too? > > (and the same comment about trunk) https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++abi/l... third_party/libc++abi/libc++abi.gyp:75: '-lgcc_s', On 2013/12/09 16:18:55, Nico wrote: > Do you still need to link against libgcc? Some time ago it was nesessary for build. Now it seems not needed anymore
https://codereview.chromium.org/75213003/diff/2740001/third_party/libc++/libc... File third_party/libc++/libc++.gyp (right): https://codereview.chromium.org/75213003/diff/2740001/third_party/libc++/libc... third_party/libc++/libc++.gyp:50: '-Wall', '-Wextra', '-Wshadow', '-Wconversion', '-Wnewline-eof', '-Wpadded', Can you please wrap this line? https://codereview.chromium.org/75213003/diff/2740001/third_party/libc++abi/l... File third_party/libc++abi/libc++abi.gyp (right): https://codereview.chromium.org/75213003/diff/2740001/third_party/libc++abi/l... third_party/libc++abi/libc++abi.gyp:42: '-Wmissing-field-initializers', '-Wchar-subscripts', '-Wmismatched-tags', Please fix line wrapping.
Done. https://codereview.chromium.org/75213003/diff/2740001/third_party/libc++/libc... File third_party/libc++/libc++.gyp (right): https://codereview.chromium.org/75213003/diff/2740001/third_party/libc++/libc... third_party/libc++/libc++.gyp:50: '-Wall', '-Wextra', '-Wshadow', '-Wconversion', '-Wnewline-eof', '-Wpadded', On 2013/12/10 13:57:00, Alexander Potapenko wrote: > Can you please wrap this line? Done. https://codereview.chromium.org/75213003/diff/2740001/third_party/libc++abi/l... File third_party/libc++abi/libc++abi.gyp (right): https://codereview.chromium.org/75213003/diff/2740001/third_party/libc++abi/l... third_party/libc++abi/libc++abi.gyp:42: '-Wmissing-field-initializers', '-Wchar-subscripts', '-Wmismatched-tags', On 2013/12/10 13:57:00, Alexander Potapenko wrote: > Please fix line wrapping. Done.
lgtm after legal blesses this.
lgtm with common.gypi changes https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++abi/l... File third_party/libc++abi/libc++abi.gyp (right): https://codereview.chromium.org/75213003/diff/2720001/third_party/libc++abi/l... third_party/libc++abi/libc++abi.gyp:31: 'trunk/src/typeinfo.cpp', On 2013/12/10 13:49:30, alextaran1 wrote: > This Unwind stuff has declarations under #ifdef __APPLE__, so on linux it's > uncompileable. That's a bug I suppose. We were able to build it on Android with some minor tweak (that we're currently upstreaming). Does msan not need instrumented unwind code? > > On 2013/12/09 16:18:55, Nico wrote: > > Do you need the Unwind stuff too? > > > > (and the same comment about trunk) > https://codereview.chromium.org/75213003/diff/2760001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/75213003/diff/2760001/build/common.gypi#newco... build/common.gypi:369: # Use libc++ instead of stdlibc++ as standard library for final binary. Please add a warning to this comment that this is used for instrumented builds. https://codereview.chromium.org/75213003/diff/2760001/build/common.gypi#newco... build/common.gypi:370: 'use_libcxx%': 0, Call this "use_custom_libcxx" or similar to make clear that this doesn't control linking against system libc++
Message was sent while issue was closed.
We've got a legal approval (see the thread in open-source-third-party-reviews@). Alex, please fix the remaining comments and submit.
Fixed. https://codereview.chromium.org/75213003/diff/2760001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/75213003/diff/2760001/build/common.gypi#newco... build/common.gypi:369: # Use libc++ instead of stdlibc++ as standard library for final binary. On 2013/12/10 17:47:30, Nico wrote: > Please add a warning to this comment that this is used for instrumented builds. Anyway, it works without instrumentation too.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alextaran@chromium.org/75213003/2800001
Message was sent while issue was closed.
Change committed as 240682
Message was sent while issue was closed.
On 2013/12/13 17:12:17, I haz the power (commit-bot) wrote: > Change committed as 240682 This fails in check_deps2git: http://build.chromium.org/p/chromium/buildstatus?builder=Linux%20x64&number=5... No match for http://llvm.org/svn/llvm-project/libcxxabi/trunk/include
Message was sent while issue was closed.
On 2013/12/13 17:12:17, I haz the power (commit-bot) wrote: > Change committed as 240682 This fails in check_deps2git: http://build.chromium.org/p/chromium/buildstatus?builder=Linux%20x64&number=5... No match for http://llvm.org/svn/llvm-project/libcxxabi/trunk/include
Feel free to revert. We weren't expecting the CQ to land this because some trybots were failing. I've opened an infra ticket for the git mirror, but that may take some time. On Dec 13, 2013 9:51 PM, <szym@chromium.org> wrote: > On 2013/12/13 17:12:17, I haz the power (commit-bot) wrote: > >> Change committed as 240682 >> > > This fails in check_deps2git: > http://build.chromium.org/p/chromium/buildstatus?builder= > Linux%20x64&number=59626 > > No match for http://llvm.org/svn/llvm-project/libcxxabi/trunk/include > > > > https://codereview.chromium.org/75213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Changed svn repository to git repository. Now deps2git does not failing on linux_rel, etc. Please, take a look.
On 2013/12/17 17:49:00, alextaran1 wrote: > Changed svn repository to git repository. Now deps2git does not failing on > linux_rel, etc. Please, take a look. If the git mirror has been set up, shouldn't this just work as it was (with svn repo and svn revision)? (I don't know, honest question. But git hashes are unreadable, and other entries have revision numbers, so it's possible somehow)
On 2013/12/17 18:20:00, Nico wrote: > On 2013/12/17 17:49:00, alextaran1 wrote: > > Changed svn repository to git repository. Now deps2git does not failing on > > linux_rel, etc. Please, take a look. > > If the git mirror has been set up, shouldn't this just work as it was (with svn > repo and svn revision)? (I don't know, honest question. But git hashes are > unreadable, and other entries have revision numbers, so it's possible somehow) I agree that SVN revision numbers are better because the upstream development of libcxx is done in SVN. But that's the only bit that needs to be changed, and I suggest to commit that change separately to avoid blocking on the infra team actions. Since Nico and Carlos have already blessed the previous patch set which differed only in the mirror paths, I suppose it's ok to land this patch.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alextaran@chromium.org/75213003/2860001
Message was sent while issue was closed.
Change committed as 241574
Please take a look. This CL was reverted once again due to problem with licensecheck. I've added libc++ license to the whitelist of licensecheck.
+phajdan for the licensecheck change
"not lgtm" https://codereview.chromium.org/75213003/diff/2880001/tools/checklicenses/che... File tools/checklicenses/checklicenses.py (right): https://codereview.chromium.org/75213003/diff/2880001/tools/checklicenses/che... tools/checklicenses/checklicenses.py:216: 'third_party/libc++': [ Do the affected files have license headers that we just don't recognize? If we don't have any license headers, this needs to be reported upstream, and the link to the bug posted near the suppression.
On Wed, Dec 18, 2013 at 12:07 PM, <phajdan.jr@chromium.org> wrote: > "not lgtm" > > > https://codereview.chromium.org/75213003/diff/2880001/tools/checklicenses/ > checklicenses.py > File tools/checklicenses/checklicenses.py (right): > > https://codereview.chromium.org/75213003/diff/2880001/tools/checklicenses/ > checklicenses.py#newcode216 > tools/checklicenses/checklicenses.py:216: 'third_party/libc++': [ > Do the affected files have license headers that we just don't recognize? > > If we don't have any license headers, this needs to be reported > upstream, which upstream? > and the link to the bug posted near the suppression. > > https://codereview.chromium.org/75213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Dec 18, 2013 at 12:08 PM, Nico Weber <thakis@chromium.org> wrote: > On Wed, Dec 18, 2013 at 12:07 PM, <phajdan.jr@chromium.org> wrote: > >> "not lgtm" >> >> >> https://codereview.chromium.org/75213003/diff/2880001/ >> tools/checklicenses/checklicenses.py >> File tools/checklicenses/checklicenses.py (right): >> >> https://codereview.chromium.org/75213003/diff/2880001/ >> tools/checklicenses/checklicenses.py#newcode216 >> tools/checklicenses/checklicenses.py:216: 'third_party/libc++': [ >> Do the affected files have license headers that we just don't recognize? >> >> If we don't have any license headers, this needs to be reported >> upstream, > > > which upstream? > And llvm files have headers that look like this: //===-------------------------- tgmath.h ----------------------------------===// // // The LLVM Compiler Infrastructure // // This file is dual licensed under the MIT and the University of Illinois Open // Source Licenses. See LICENSE.TXT for details. // //===----------------------------------------------------------------------===// > > >> and the link to the bug posted near the suppression. >> >> https://codereview.chromium.org/75213003/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I've filed http://crbug.com/329819 about the missing support for this dual license in licensecheck.pl Paweł, is it ok to proceed with the suppression annotated with that bug? I'm also wondering why didn't the trybots run checklicense.py
On 2013/12/19 08:49:34, Alexander Potapenko wrote: > I've filed http://crbug.com/329819 about the missing support for this dual > license in licensecheck.pl > Paweł, is it ok to proceed with the suppression annotated with that bug? Sorry, no. The license headers exist, so we should modify our copy of licensecheck.pl to recognize them. Please let me know if you'd like me to help with this. Note that just having a check that becomes suppressed on failures wouldn't make much sense. The whole idea is to push towards having per-file license headers recognized by a script. When there are no license headers we need to rely on upstream to add them, and then suppression is a pragmatic choice (but we _do_ ask upstream for this before proceeding).
> The license headers exist, so we should modify our copy of licensecheck.pl to > recognize them. https://codereview.chromium.org/103493008/ Alex, please revert checklicenses.py in your patch.
Added ignoring files without license header to checklicenses.py (due to http://llvm.org/bugs/show_bug.cgi?id=18291). Please take a look.
checklicenses LGTM; thank you
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alextaran@chromium.org/75213003/2940001
Message was sent while issue was closed.
Change committed as 242088 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
