Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
293003010:
Combine base_java.jar, net_java.jar and url_java.jar into cronet.jar (Closed)
|
Created:
6 years, 7 months ago by mef Modified:
6 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCombine base_java.jar, net_java.jar and url_java.jar into cronet.jar
BUG=354143
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272988
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address review comments. #Patch Set 3 : Address review comment, don't exclude ApplicationStatus and CommandLine. #
Messages
Total messages: 15 (0 generated)
Hi guys, please take a look.
LGTM Also adding Ryan who knows more about gyp. https://codereview.chromium.org/293003010/diff/1/components/cronet/tools/extr... File components/cronet/tools/extract_from_jars.py (right): https://codereview.chromium.org/293003010/diff/1/components/cronet/tools/extr... components/cronet/tools/extract_from_jars.py:3: # Copyright 2013 The Chromium Authors. All rights reserved. nit: 2014 https://codereview.chromium.org/293003010/diff/1/components/cronet/tools/extr... components/cronet/tools/extract_from_jars.py:25: build_utils.MakeDirectory(jar_cwd); nit: Remove ; from the end, too much c++ ;-)
LGTM. Reviewed all the code, but heavily deferring to other reviewers. On 2014/05/22 13:34:41, Paweł Hajdan Jr. wrote: > LGTM > > Also adding Ryan who knows more about gyp. > > https://codereview.chromium.org/293003010/diff/1/components/cronet/tools/extr... > File components/cronet/tools/extract_from_jars.py (right): > > https://codereview.chromium.org/293003010/diff/1/components/cronet/tools/extr... > components/cronet/tools/extract_from_jars.py:3: # Copyright 2013 The Chromium > Authors. All rights reserved. > nit: 2014 > > https://codereview.chromium.org/293003010/diff/1/components/cronet/tools/extr... > components/cronet/tools/extract_from_jars.py:25: > build_utils.MakeDirectory(jar_cwd); > nit: Remove ; from the end, too much c++ ;-)
https://codereview.chromium.org/293003010/diff/1/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/293003010/diff/1/components/cronet.gypi#newco... components/cronet.gypi:164: '--jars=<@(_inputs)', This doesn't seem right. <@(_inputs) is going to expand to space separated list, but presumably you want to quote it? I didn't think <@( handled quoting. https://codereview.chromium.org/293003010/diff/1/components/cronet.gypi#newco... components/cronet.gypi:185: '--excluded-classes=<(jar_excluded_classes)', Why <( vs <@( here?
Thanks! https://codereview.chromium.org/293003010/diff/1/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/293003010/diff/1/components/cronet.gypi#newco... components/cronet.gypi:164: '--jars=<@(_inputs)', On 2014/05/22 22:16:53, Ryan Sleevi wrote: > This doesn't seem right. > > <@(_inputs) is going to expand to space separated list, but presumably you want > to quote it? I didn't think <@( handled quoting. It gets expanded into this: [135/138] cd ../../components; python cronet/tools/extract_from_jars.py "--classes-dir=../out/Debug/gen/cronet/cronet_jar_extract" "--jars=\"../out/Debug/lib.java/cronet.jar\" \"../out/Debug/lib.java/base_java.jar\" \"../out/Debug/lib.java/net_java.jar\" \"../out/Debug/lib.java/url_java.jar\"" "--stamp=../out/Debug/gen/cronet/jar_extract.stamp" The python script then iterates over |jars|. https://codereview.chromium.org/293003010/diff/1/components/cronet.gypi#newco... components/cronet.gypi:185: '--excluded-classes=<(jar_excluded_classes)', On 2014/05/22 22:16:53, Ryan Sleevi wrote: > Why <( vs <@( here? Um, I'm not sure. I took it from here: https://code.google.com/p/chromium/codesearch#chromium/src/build/java.gypi&l=310 What is <( vs <@(? It gets expanded into this: [137/138] cd ../../components; python ../build/android/gyp/jar.py "--classes-dir=../out/Debug/gen/cronet/cronet_jar_extract" "--jar-path=../out/Debug/cronet/cronet.jar" "--excluded-classes=\"*/Application*.class\" \"*/BaseChrom*.class\" \"*/CommandLine*.class\"" "--stamp=../out/Debug/gen/cronet/cronet_jar.stamp" https://codereview.chromium.org/293003010/diff/1/components/cronet/tools/extr... File components/cronet/tools/extract_from_jars.py (right): https://codereview.chromium.org/293003010/diff/1/components/cronet/tools/extr... components/cronet/tools/extract_from_jars.py:3: # Copyright 2013 The Chromium Authors. All rights reserved. On 2014/05/22 13:34:42, Paweł Hajdan Jr. wrote: > nit: 2014 Done. https://codereview.chromium.org/293003010/diff/1/components/cronet/tools/extr... components/cronet/tools/extract_from_jars.py:25: build_utils.MakeDirectory(jar_cwd); On 2014/05/22 13:34:42, Paweł Hajdan Jr. wrote: > nit: Remove ; from the end, too much c++ ;-) Done.
lgtm https://codereview.chromium.org/293003010/diff/1/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/293003010/diff/1/components/cronet.gypi#newco... components/cronet.gypi:164: '--jars=<@(_inputs)', On 2014/05/22 22:28:09, mef wrote: > On 2014/05/22 22:16:53, Ryan Sleevi wrote: > > This doesn't seem right. > > > > <@(_inputs) is going to expand to space separated list, but presumably you > want > > to quote it? I didn't think <@( handled quoting. > > It gets expanded into this: > > [135/138] cd ../../components; python cronet/tools/extract_from_jars.py > "--classes-dir=../out/Debug/gen/cronet/cronet_jar_extract" > "--jars=\"../out/Debug/lib.java/cronet.jar\" > \"../out/Debug/lib.java/base_java.jar\" \"../out/Debug/lib.java/net_java.jar\" > \"../out/Debug/lib.java/url_java.jar\"" > "--stamp=../out/Debug/gen/cronet/jar_extract.stamp" > > The python script then iterates over |jars|. Ah, it's a bit of a side-effect of how the args are being specified, that's forcing an outer-quoting. The only reason --jars is being quoted is because of how <() is being expanded. That is, one would normally do 'something' like ['--classes-dir', '<(jar_extract_dir)', '--jars', '<@(_inputs)', '--stamp', '<(jar_extract_stamp)'] This would be expanded as python cronet/tools/extract_from_jars.py --classes-dir "../out/Debug/gen/cronet/cronet_jar_extract" --jars "cronet.jar" "base_java.jar" "net_java.jar" "url_java.jar" --stamp "jar_extract.stamp" Which would screw up the processing of --jars, because the entire string wasn't quoted (just the individual path components) https://codereview.chromium.org/293003010/diff/1/components/cronet.gypi#newco... components/cronet.gypi:185: '--excluded-classes=<(jar_excluded_classes)', On 2014/05/22 22:28:09, mef wrote: > On 2014/05/22 22:16:53, Ryan Sleevi wrote: > > Why <( vs <@( here? > > Um, I'm not sure. I took it from here: > https://code.google.com/p/chromium/codesearch#chromium/src/build/java.gypi&l=310 > > What is <( vs <@(? > > It gets expanded into this: > > [137/138] cd ../../components; python ../build/android/gyp/jar.py > "--classes-dir=../out/Debug/gen/cronet/cronet_jar_extract" > "--jar-path=../out/Debug/cronet/cronet.jar" > "--excluded-classes=\"*/Application*.class\" \"*/BaseChrom*.class\" > \"*/CommandLine*.class\"" "--stamp=../out/Debug/gen/cronet/cronet_jar.stamp" > > <() vs <@() is list-expansion versus string expansion <() of ["foo", "bar", "baz"] would result in "foo bar baz". <@() of ["foo", "bar", "baz"] would result in "foo" "bar" "baz" In this case, since it's part of a string, it doesn't matter. <@() is more idiomatic for list expansion though.
thanks! https://codereview.chromium.org/293003010/diff/1/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/293003010/diff/1/components/cronet.gypi#newco... components/cronet.gypi:164: '--jars=<@(_inputs)', On 2014/05/22 22:59:34, Ryan Sleevi wrote: > On 2014/05/22 22:28:09, mef wrote: > > On 2014/05/22 22:16:53, Ryan Sleevi wrote: > > > This doesn't seem right. > > > > > > <@(_inputs) is going to expand to space separated list, but presumably you > > want > > > to quote it? I didn't think <@( handled quoting. > > > > It gets expanded into this: > > > > [135/138] cd ../../components; python cronet/tools/extract_from_jars.py > > "--classes-dir=../out/Debug/gen/cronet/cronet_jar_extract" > > "--jars=\"../out/Debug/lib.java/cronet.jar\" > > \"../out/Debug/lib.java/base_java.jar\" \"../out/Debug/lib.java/net_java.jar\" > > \"../out/Debug/lib.java/url_java.jar\"" > > "--stamp=../out/Debug/gen/cronet/jar_extract.stamp" > > > > The python script then iterates over |jars|. > > Ah, it's a bit of a side-effect of how the args are being specified, that's > forcing an outer-quoting. > > The only reason --jars is being quoted is because of how <() is being expanded. > > That is, one would normally do 'something' like > ['--classes-dir', '<(jar_extract_dir)', '--jars', '<@(_inputs)', '--stamp', > '<(jar_extract_stamp)'] > > This would be expanded as > python cronet/tools/extract_from_jars.py --classes-dir > "../out/Debug/gen/cronet/cronet_jar_extract" --jars "cronet.jar" "base_java.jar" > "net_java.jar" "url_java.jar" --stamp "jar_extract.stamp" > > Which would screw up the processing of --jars, because the entire string wasn't > quoted (just the individual path components) I see, thanks for the explanation! https://codereview.chromium.org/293003010/diff/1/components/cronet.gypi#newco... components/cronet.gypi:185: '--excluded-classes=<(jar_excluded_classes)', On 2014/05/22 22:59:34, Ryan Sleevi wrote: > On 2014/05/22 22:28:09, mef wrote: > > On 2014/05/22 22:16:53, Ryan Sleevi wrote: > > > Why <( vs <@( here? > > > > Um, I'm not sure. I took it from here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/java.gypi&l=310 > > > > What is <( vs <@(? > > > > It gets expanded into this: > > > > [137/138] cd ../../components; python ../build/android/gyp/jar.py > > "--classes-dir=../out/Debug/gen/cronet/cronet_jar_extract" > > "--jar-path=../out/Debug/cronet/cronet.jar" > > "--excluded-classes=\"*/Application*.class\" \"*/BaseChrom*.class\" > > \"*/CommandLine*.class\"" "--stamp=../out/Debug/gen/cronet/cronet_jar.stamp" > > > > > > <() vs <@() is list-expansion versus string expansion > > <() of ["foo", "bar", "baz"] would result in "foo bar baz". > <@() of ["foo", "bar", "baz"] would result in "foo" "bar" "baz" > > In this case, since it's part of a string, it doesn't matter. <@() is more > idiomatic for list expansion though. Done.
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/293003010/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/293003010/40001
Message was sent while issue was closed.
Change committed as 272988 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
