Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2497043002:
Add GN build rules to allow java_assertion_enabler to enable Java asserts. (Closed)
DescriptionAdd GN build rules to allow java_assertion_enabler to enable Java asserts.
Also modify java_assertion_enabler to resolve cycle dependency issue
and empty jar issue.
BUG=462676, 665157, 665478
Committed: https://crrev.com/5d3328f05a88be232e11d2ce34bffa64ca2362a5
Cr-Commit-Position: refs/heads/master@{#432521}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Addressing comments #
Total comments: 3
Patch Set 3 : Fixing assert exceptions #
Total comments: 2
Patch Set 4 : Fixing assert exceptions #
Messages
Total messages: 40 (24 generated)
zpeng@chromium.org changed reviewers: + agrieve@chromium.org, jbudorick@chromium.org
Hi Andrew & John, PTAL. Thanks!
just nits. Probably a good idea to wait until monday to submit this in case any bots break. https://codereview.chromium.org/2497043002/diff/1/build/android/java_assertio... File build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java (right): https://codereview.chromium.org/2497043002/diff/1/build/android/java_assertio... build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java:90: String tempJarPath = inputJarPath + TEMPORARY_FILE_SUFFIX; nit: you should use outputJarPath + TEMP here (not inputJarPath) https://codereview.chromium.org/2497043002/diff/1/build/android/java_assertio... build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java:119: Path src = Paths.get(tempJarPath), dest = Paths.get(outputJarPath); nit: declaring multiple variable via a , is against style guide. Put dest on its own line. https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:1032: _enable_assert = is_java_debug && defined(invoker.supports_android) && nit: also enable when dcheck_always_on is set (this will make it match the value of BuildConfig.DCHECK_IS_ON) https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:1033: invoker.supports_android nit: you have supports_android here twice :P. https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:1125: _build_config, nit: this isn't an input and can be removed.
lgtm On 2016/11/11 21:30:48, agrieve wrote: > just nits. Probably a good idea to wait until monday to submit this in case any > bots break. > > https://codereview.chromium.org/2497043002/diff/1/build/android/java_assertio... > File > build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java > (right): > > https://codereview.chromium.org/2497043002/diff/1/build/android/java_assertio... > build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java:90: > String tempJarPath = inputJarPath + TEMPORARY_FILE_SUFFIX; > nit: you should use outputJarPath + TEMP here (not inputJarPath) > > https://codereview.chromium.org/2497043002/diff/1/build/android/java_assertio... > build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java:119: > Path src = Paths.get(tempJarPath), dest = Paths.get(outputJarPath); > nit: declaring multiple variable via a , is against style guide. Put dest on its > own line. > > https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... > File build/config/android/internal_rules.gni (right): > > https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... > build/config/android/internal_rules.gni:1032: _enable_assert = is_java_debug && > defined(invoker.supports_android) && > nit: also enable when dcheck_always_on is set (this will make it match the value > of BuildConfig.DCHECK_IS_ON) > > https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... > build/config/android/internal_rules.gni:1033: invoker.supports_android > nit: you have supports_android here twice :P. > > https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... > build/config/android/internal_rules.gni:1125: _build_config, > nit: this isn't an input and can be removed.
The CQ bit was checked by agrieve@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: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
big +1 to not landing this before Monday. https://codereview.chromium.org/2497043002/diff/1/build/android/java_assertio... File build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java (right): https://codereview.chromium.org/2497043002/diff/1/build/android/java_assertio... build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java:91: try (ZipInputStream inputStream = new ZipInputStream( I think you have the honor of writing the first multi-statement try w/ resources in chromium. nit: Can you try to format this in a way that makes it a bit more readable? As-is, everything is at different indentation depths. https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:1033: invoker.supports_android On 2016/11/11 21:30:47, agrieve wrote: > nit: you have supports_android here twice :P. defined & checking the value? https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:1107: action(_output_jar_target) { This should have some kind of stated dependency on //build/android/java_assertion_enabler. https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:1108: script = "$root_out_dir/bin/java_assertion_enabler" I would prefer if this wasn't in $root_out_dir/bin -- we drop a bunch of scripts in there that we expect users to call explicitly rather than scripts that are part of the build process.
The CQ bit was checked by zpeng@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...
Thanks Andrew & John! PTAL https://codereview.chromium.org/2497043002/diff/1/build/android/java_assertio... File build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java (right): https://codereview.chromium.org/2497043002/diff/1/build/android/java_assertio... build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java:90: String tempJarPath = inputJarPath + TEMPORARY_FILE_SUFFIX; On 2016/11/11 21:30:47, agrieve wrote: > nit: you should use outputJarPath + TEMP here (not inputJarPath) Done. https://codereview.chromium.org/2497043002/diff/1/build/android/java_assertio... build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java:119: Path src = Paths.get(tempJarPath), dest = Paths.get(outputJarPath); On 2016/11/11 21:30:47, agrieve wrote: > nit: declaring multiple variable via a , is against style guide. Put dest on its > own line. Done. Thanks! https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:1032: _enable_assert = is_java_debug && defined(invoker.supports_android) && On 2016/11/11 21:30:47, agrieve wrote: > nit: also enable when dcheck_always_on is set (this will make it match the value > of BuildConfig.DCHECK_IS_ON) Done. https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:1107: action(_output_jar_target) { On 2016/11/11 21:50:20, jbudorick wrote: > This should have some kind of stated dependency on > //build/android/java_assertion_enabler. Done. https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:1108: script = "$root_out_dir/bin/java_assertion_enabler" On 2016/11/11 21:50:20, jbudorick wrote: > I would prefer if this wasn't in $root_out_dir/bin -- we drop a bunch of scripts > in there that we expect users to call explicitly rather than scripts that are > part of the build process. Done. https://codereview.chromium.org/2497043002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:1125: _build_config, On 2016/11/11 21:30:47, agrieve wrote: > nit: this isn't an input and can be removed. Done.
lgtm /w one nit https://codereview.chromium.org/2497043002/diff/20001/BUILD.gn File BUILD.gn (left): https://codereview.chromium.org/2497043002/diff/20001/BUILD.gn#oldcode857 BUILD.gn:857: "//build/android/incremental_install:bootstrap_java", note: I said this was fine to remove as well since it's also referenced by targets (in a similar way to java_assertion_enabler) https://codereview.chromium.org/2497043002/diff/20001/build/android/java_asse... File build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java (right): https://codereview.chromium.org/2497043002/diff/20001/build/android/java_asse... build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java:121: Files.copy(src, dest, StandardCopyOption.REPLACE_EXISTING); Should use Files.move() so as to delete the .tmp file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
zpeng@chromium.org changed reviewers: + toyoshim@chromium.org
Thanks Andrew & John! PTAL Hi Takashi, PTAL. Thanks! https://codereview.chromium.org/2497043002/diff/20001/build/android/java_asse... File build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java (right): https://codereview.chromium.org/2497043002/diff/20001/build/android/java_asse... build/android/java_assertion_enabler/java/org/chromium/javaassertionenabler/AssertionEnabler.java:121: Files.copy(src, dest, StandardCopyOption.REPLACE_EXISTING); On 2016/11/14 19:00:57, agrieve wrote: > Should use Files.move() so as to delete the .tmp file. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2497043002/diff/40001/media/midi/java/src/org... File media/midi/java/src/org/chromium/midi/MidiManagerAndroid.java (right): https://codereview.chromium.org/2497043002/diff/40001/media/midi/java/src/org... media/midi/java/src/org/chromium/midi/MidiManagerAndroid.java:70: // TODO(crbug.com/665157) Sorry, I don't have much background about this work (sorry, didn't read the original bug thread well). But, when this TODO can be removed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
LGTM. But, can you also add 665157 to the BUG= line? https://codereview.chromium.org/2497043002/diff/40001/media/midi/java/src/org... File media/midi/java/src/org/chromium/midi/MidiManagerAndroid.java (right): https://codereview.chromium.org/2497043002/diff/40001/media/midi/java/src/org... media/midi/java/src/org/chromium/midi/MidiManagerAndroid.java:70: // TODO(crbug.com/665157) Ah, I see. I read the 66517.
Description was changed from ========== Add GN build rules to allow java_assertion_enabler to enable Java asserts. Also modify java_assertion_enabler to resolve cycle dependency issue and empty jar issue. BUG=462676 ========== to ========== Add GN build rules to allow java_assertion_enabler to enable Java asserts. Also modify java_assertion_enabler to resolve cycle dependency issue and empty jar issue. BUG=462676,665157,665478 ==========
The CQ bit was checked by zpeng@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...
zpeng@chromium.org changed reviewers: + qinmin@chromium.org
Thanks Andrew, John, and Takashi! PTAL Hi Min, PTAL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
The CQ bit was checked by zpeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org, toyoshim@chromium.org Link to the patchset: https://codereview.chromium.org/2497043002/#ps60001 (title: "Fixing assert exceptions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add GN build rules to allow java_assertion_enabler to enable Java asserts. Also modify java_assertion_enabler to resolve cycle dependency issue and empty jar issue. BUG=462676,665157,665478 ========== to ========== Add GN build rules to allow java_assertion_enabler to enable Java asserts. Also modify java_assertion_enabler to resolve cycle dependency issue and empty jar issue. BUG=462676,665157,665478 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add GN build rules to allow java_assertion_enabler to enable Java asserts. Also modify java_assertion_enabler to resolve cycle dependency issue and empty jar issue. BUG=462676,665157,665478 ========== to ========== Add GN build rules to allow java_assertion_enabler to enable Java asserts. Also modify java_assertion_enabler to resolve cycle dependency issue and empty jar issue. BUG=462676,665157,665478 Committed: https://crrev.com/5d3328f05a88be232e11d2ce34bffa64ca2362a5 Cr-Commit-Position: refs/heads/master@{#432521} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5d3328f05a88be232e11d2ce34bffa64ca2362a5 Cr-Commit-Position: refs/heads/master@{#432521}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2507153002/ by zpeng@chromium.org. The reason for reverting is: Broke arm64 builder: https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20ARM.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
