Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1869083002:
🎽 Fix instrumentation test proguard parser when gmscore has annotations (Closed)
|
Created:
4 years, 8 months ago by agrieve Modified:
4 years, 8 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org, dgn Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix instrumentation test proguard parser when gmscore has annotations
Most output with annotations look like:
Class member attributes (count = 2):
- Deprecated attribute
- Runtime visible annotations attribute:
- Annotation [Ljava/lang/Deprecated;]:
- Method: <init>(Ljava/security/cert/X509Certificate;)V
But it was choking when it hit:
Class member attributes (count = 2):
- Runtime invisible annotations attribute:
- Annotation [Landroid/support/annotation/Nullable;]:
- Signature attribute:
- Utf8 [()Ljava/util/List<Ljava/lang/String;>;]
The code was treating - Utf8 as an annotation-related line.
BUG=525035
Committed: https://crrev.com/44a0b274c777c0a6aee6f5ba722c435e71d54273
Cr-Commit-Position: refs/heads/master@{#386706}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 17 (7 generated)
Description was changed from ========== Fix instrumentation test proguard parser when gmscore has annotations BUG=525035 ========== to ========== Fix instrumentation test proguard parser when gmscore has annotations BUG=525035 ==========
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org
On 2016/04/07 18:08:06, agrieve wrote: > mailto:agrieve@chromium.org changed reviewers: > + mailto:jbudorick@chromium.org Without this, setting proguard_preprocess=false on play services .jar results in an assertion error: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... Confirmed that before & after this change test runner reports the same number of tests to be run.
https://codereview.chromium.org/1869083002/diff/1/build/android/pylib/utils/p... File build/android/pylib/utils/proguard.py (right): https://codereview.chromium.org/1869083002/diff/1/build/android/pylib/utils/p... build/android/pylib/utils/proguard.py:288: state.InitMethod(None) What does this line that gets here contain? It's not empty, doesn't match any of the other regexes, and we just parsed an annotation...?
Description was changed from
==========
Fix instrumentation test proguard parser when gmscore has annotations
BUG=525035
==========
to
==========
Fix instrumentation test proguard parser when gmscore has annotations
Most output with annotations look like:
Class member attributes (count = 2):
- Deprecated attribute
- Runtime visible annotations attribute:
- Annotation [Ljava/lang/Deprecated;]:
- Method: <init>(Ljava/security/cert/X509Certificate;)V
But it was choking when it hit:
Class member attributes (count = 2):
- Runtime invisible annotations attribute:
- Annotation [Landroid/support/annotation/Nullable;]:
- Signature attribute:
- Utf8 [()Ljava/util/List<Ljava/lang/String;>;]
The code was treating - Utf8 as an annotation-related line.
BUG=525035
==========
agrieve@chromium.org changed reviewers: + dgn@chromium.org
On 2016/04/08 00:59:09, jbudorick (ooo until apr 18) wrote: > https://codereview.chromium.org/1869083002/diff/1/build/android/pylib/utils/p... > File build/android/pylib/utils/proguard.py (right): > > https://codereview.chromium.org/1869083002/diff/1/build/android/pylib/utils/p... > build/android/pylib/utils/proguard.py:288: state.InitMethod(None) > What does this line that gets here contain? It's not empty, doesn't match any of > the other regexes, and we just parsed an annotation...? to dgn@ for review while john is away.
https://codereview.chromium.org/1869083002/diff/1/build/android/pylib/utils/p... File build/android/pylib/utils/proguard.py (right): https://codereview.chromium.org/1869083002/diff/1/build/android/pylib/utils/p... build/android/pylib/utils/proguard.py:288: state.InitMethod(None) So that clears the annotation stack when we hit something that is not an annotation field value. Shouldn't the stack be cleared when we hit a new section anyway? "- Signature attribute" should reset the state, right?
https://codereview.chromium.org/1869083002/diff/1/build/android/pylib/utils/p... File build/android/pylib/utils/proguard.py (right): https://codereview.chromium.org/1869083002/diff/1/build/android/pylib/utils/p... build/android/pylib/utils/proguard.py:288: state.InitMethod(None) On 2016/04/08 16:54:52, dgn wrote: > So that clears the annotation stack when we hit something that is not an > annotation field value. Shouldn't the stack be cleared when we hit a new section > anyway? "- Signature attribute" should reset the state, right? That's what we want, but it's not what the code was doing. It doesn't actually look at indentation as a part of the core logic. It was just saying: 1. Keep looking until we find the start of a method 2. Then, keep looking until we find "Annotations" 3. Then, keep parsing annotations until we hit another method or section. Our "sections" are defined as: Interfaces|Constant Pool|Fields|Methods|Class file attributes So, it was missing logic to handle more siblings of - Annotations.
On 2016/04/08 17:35:36, agrieve wrote: > https://codereview.chromium.org/1869083002/diff/1/build/android/pylib/utils/p... > File build/android/pylib/utils/proguard.py (right): > > https://codereview.chromium.org/1869083002/diff/1/build/android/pylib/utils/p... > build/android/pylib/utils/proguard.py:288: state.InitMethod(None) > On 2016/04/08 16:54:52, dgn wrote: > > So that clears the annotation stack when we hit something that is not an > > annotation field value. Shouldn't the stack be cleared when we hit a new > section > > anyway? "- Signature attribute" should reset the state, right? > > That's what we want, but it's not what the code was doing. It doesn't actually > look at indentation as a part of the core logic. It was just saying: > > 1. Keep looking until we find the start of a method > 2. Then, keep looking until we find "Annotations" > 3. Then, keep parsing annotations until we hit another method or section. > > Our "sections" are defined as: > Interfaces|Constant Pool|Fields|Methods|Class file attributes > > So, it was missing logic to handle more siblings of - Annotations. 🏃🏃🏃
Thanks for the explanation, lgtm
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869083002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869083002/1
Message was sent while issue was closed.
Description was changed from
==========
Fix instrumentation test proguard parser when gmscore has annotations
Most output with annotations look like:
Class member attributes (count = 2):
- Deprecated attribute
- Runtime visible annotations attribute:
- Annotation [Ljava/lang/Deprecated;]:
- Method: <init>(Ljava/security/cert/X509Certificate;)V
But it was choking when it hit:
Class member attributes (count = 2):
- Runtime invisible annotations attribute:
- Annotation [Landroid/support/annotation/Nullable;]:
- Signature attribute:
- Utf8 [()Ljava/util/List<Ljava/lang/String;>;]
The code was treating - Utf8 as an annotation-related line.
BUG=525035
==========
to
==========
Fix instrumentation test proguard parser when gmscore has annotations
Most output with annotations look like:
Class member attributes (count = 2):
- Deprecated attribute
- Runtime visible annotations attribute:
- Annotation [Ljava/lang/Deprecated;]:
- Method: <init>(Ljava/security/cert/X509Certificate;)V
But it was choking when it hit:
Class member attributes (count = 2):
- Runtime invisible annotations attribute:
- Annotation [Landroid/support/annotation/Nullable;]:
- Signature attribute:
- Utf8 [()Ljava/util/List<Ljava/lang/String;>;]
The code was treating - Utf8 as an annotation-related line.
BUG=525035
==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from
==========
Fix instrumentation test proguard parser when gmscore has annotations
Most output with annotations look like:
Class member attributes (count = 2):
- Deprecated attribute
- Runtime visible annotations attribute:
- Annotation [Ljava/lang/Deprecated;]:
- Method: <init>(Ljava/security/cert/X509Certificate;)V
But it was choking when it hit:
Class member attributes (count = 2):
- Runtime invisible annotations attribute:
- Annotation [Landroid/support/annotation/Nullable;]:
- Signature attribute:
- Utf8 [()Ljava/util/List<Ljava/lang/String;>;]
The code was treating - Utf8 as an annotation-related line.
BUG=525035
==========
to
==========
Fix instrumentation test proguard parser when gmscore has annotations
Most output with annotations look like:
Class member attributes (count = 2):
- Deprecated attribute
- Runtime visible annotations attribute:
- Annotation [Ljava/lang/Deprecated;]:
- Method: <init>(Ljava/security/cert/X509Certificate;)V
But it was choking when it hit:
Class member attributes (count = 2):
- Runtime invisible annotations attribute:
- Annotation [Landroid/support/annotation/Nullable;]:
- Signature attribute:
- Utf8 [()Ljava/util/List<Ljava/lang/String;>;]
The code was treating - Utf8 as an annotation-related line.
BUG=525035
Committed: https://crrev.com/44a0b274c777c0a6aee6f5ba722c435e71d54273
Cr-Commit-Position: refs/heads/master@{#386706}
==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/44a0b274c777c0a6aee6f5ba722c435e71d54273 Cr-Commit-Position: refs/heads/master@{#386706} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
