Remove proguard specific override block

Remove proguard specific override block

Proguard config is already forwarded using a
separate mechanism. Remove this if-block
to get rid of the duplication.

BUG=623672
Committed: https://crrev.com/ef53525821a41adb38fedd2e9b14deb12578da3d
Cr-Commit-Position: refs/heads/master@{#402578}

[kraush, 2016-06-27 19:31:07]

kraush@amazon.com changed reviewers:
+ dpranke@chromium.org

[kraush, 2016-06-27 19:31:08]

Hi Dirk,

Can you take a look at this GN change to allow test apks to use proguard?
This is related to https://codereview.chromium.org/2000333003 where you
recommended not using += (thus the nulling)
Let me know if you'd rather have += though (both of them work)

Thanks! :)
Holger

[Dirk Pranke, 2016-06-27 23:17:58]

On 2016/06/27 19:31:08, kraush wrote:
> Hi Dirk,
> 
> Can you take a look at this GN change to allow test apks to use proguard?
> This is related to https://codereview.chromium.org/2000333003 where you
> recommended not using += (thus the nulling)

In that CL, I said not to use "+=" because proguard_configs wasn't actually 
already defined.

In the linked bug, it looks like somehow you're getting into the situation
where it is defined, but it's not obvious to me how that's happening, and
so I'm not sure that this is actually the right fix.

Are you importing "//build/config/android/rules.gni" from a file *after* the
line where you assign a value to proguard_configs?

[kraush, 2016-06-27 23:29:41]

On 2016/06/27 23:17:58, Dirk Pranke wrote:
> On 2016/06/27 19:31:08, kraush wrote:
> > Hi Dirk,
> > 
> > Can you take a look at this GN change to allow test apks to use proguard?
> > This is related to https://codereview.chromium.org/2000333003 where you
> > recommended not using += (thus the nulling)
> 
> In that CL, I said not to use "+=" because proguard_configs wasn't actually 
> already defined.
> 
> In the linked bug, it looks like somehow you're getting into the situation
> where it is defined, but it's not obvious to me how that's happening, and
> so I'm not sure that this is actually the right fix.
> 
> Are you importing "//build/config/android/rules.gni" from a file *after* the
> line where you assign a value to proguard_configs?

No I'm not :(
The import is in lines 10-12 of my file and looks like this:
if (is_android) {
  import("//build/config/android/rules.gni")
}

Later, I'm using it in a test target:
[28] test("my_unit_tests") {
...
[143] if (is_android) {
[144] proguard_enabled = true
[145] proguard_configs = [ "//amazon/myfolder/android/java/proguard.flags" ]

Does that look incorrect?
Any recommendations on how else it should look like?

[Dirk Pranke, 2016-06-27 23:37:00]

dpranke@chromium.org changed reviewers:
+ agrieve@chromium.org

[Dirk Pranke, 2016-06-27 23:37:01]

At first look, it looks like you're doing the right thing and I don't know why
you're getting that error.

agrieve@, can you look at this?

[agrieve, 2016-06-28 00:10:56]

On 2016/06/27 23:37:01, Dirk Pranke wrote:
> At first look, it looks like you're doing the right thing and I don't know why
> you're getting that error.
> 
> agrieve@, can you look at this?

I'd guess it's because there's already a line just above your change:

forward_variables_from(invoker, "*")

The right "fix" is probably to delete the added if clause, since
proguard_enabled and proguard_configs will already be passed through from the
forward_variables_from.

[kraush, 2016-06-28 00:55:38]

On 2016/06/28 00:10:56, agrieve wrote:
> On 2016/06/27 23:37:01, Dirk Pranke wrote:
> > At first look, it looks like you're doing the right thing and I don't know
why
> > you're getting that error.
> > 
> > agrieve@, can you look at this?
> 
> I'd guess it's because there's already a line just above your change:
> 
> forward_variables_from(invoker, "*")
> 
> The right "fix" is probably to delete the added if clause, since
> proguard_enabled and proguard_configs will already be passed through from the
> forward_variables_from.

Good call agrieve! :)
Removing the whole if block indeed also fixes the build and the tests work just
fine.
Patch set 2 incoming.

[kraush, 2016-06-28 00:58:33]

Description was changed from

==========
Allow overriding proguard_config

This change allows overriding the proguard_config
flag by first nulling it.

BUG=623672
==========

to

==========
Remove proguard specific override block

Proguard config is already forwarded using a
to get rid of the duplication.

BUG=623672
==========

[kraush, 2016-06-28 00:59:10]

Updated with patch set two, which just gets rid of the whole if block.

Please take another look.
(Also updated commit message)

[Dirk Pranke, 2016-06-28 01:33:46]

This is probably fine (i.e., lgtm), but the prior code was ensuring that if
proguard_enabled is true, configs != [], and the new code won't necessarily
ensure that.

[kraush, 2016-06-28 15:31:44]

On 2016/06/28 01:33:46, Dirk Pranke wrote:
> This is probably fine (i.e., lgtm), but the prior code was ensuring that if
> proguard_enabled is true, configs != [], and the new code won't necessarily
> ensure that.

Good point!
I'll add an assert to make sure this constraint is still intact.
Will post a new diff momentarily.

[kraush, 2016-06-28 15:34:14]

Description was changed from

==========
Remove proguard specific override block

Proguard config is already forwarded using a
to get rid of the duplication.

BUG=623672
==========

to

==========
Remove proguard specific override block

Proguard config is already forwarded using a
separate mechanism. Remove this if-block
to get rid of the duplication.

BUG=623672
==========

[kraush, 2016-06-28 15:34:47]

Added an assert that makes sure the original constraint is still kept.
Please let me know what you think.

[Dirk Pranke, 2016-06-28 17:40:51]

lgtm

[kraush, 2016-06-28 17:45:21]

The CQ bit was checked by kraush@amazon.com

[commit-bot: I haz the power, 2016-06-28 17:45:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 19:18:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 19:18:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kraush, 2016-06-28 19:23:44]

On 2016/06/28 19:18:55, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Since this is a pure Android GN change, I'm pretty sure I didn't cause those
Windows test failures :/
Is there a process on how to proceed?
Can I just check the commit box again, or should I wait? (From what I can tell
other Windows test jobs have passed since, so I don't think this is an ongoing
issue)

[kraush, 2016-06-28 19:45:24]

On 2016/06/28 19:23:44, kraush wrote:
> On 2016/06/28 19:18:55, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> 
> Since this is a pure Android GN change, I'm pretty sure I didn't cause those
> Windows test failures :/
> Is there a process on how to proceed?
> Can I just check the commit box again, or should I wait? (From what I can tell
> other Windows test jobs have passed since, so I don't think this is an ongoing
> issue)

Since this passed on all other bots (and as I mentioned is a pure Android change
that failed on Windows), I'm gonna run a retry.

[kraush, 2016-06-28 19:45:32]

The CQ bit was checked by kraush@amazon.com

[commit-bot: I haz the power, 2016-06-28 19:46:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dirk Pranke, 2016-06-28 19:51:21]

On 2016/06/28 19:23:44, kraush wrote:
> Since this is a pure Android GN change, I'm pretty sure I didn't cause those
> Windows test failures :/
> Is there a process on how to proceed?
> Can I just check the commit box again, or should I wait? (From what I can tell
> other Windows test jobs have passed since, so I don't think this is an ongoing
> issue)

The process is to check the box again and see what happens a second time :)

If the failures persist we should see what's actually going on.

[kraush, 2016-06-28 19:53:52]

On 2016/06/28 19:51:21, Dirk Pranke wrote:
> On 2016/06/28 19:23:44, kraush wrote:
> > Since this is a pure Android GN change, I'm pretty sure I didn't cause those
> > Windows test failures :/
> > Is there a process on how to proceed?
> > Can I just check the commit box again, or should I wait? (From what I can
tell
> > other Windows test jobs have passed since, so I don't think this is an
ongoing
> > issue)
> 
> The process is to check the box again and see what happens a second time :)
> 
> If the failures persist we should see what's actually going on.

Gotcha, thanks for the explanation Dirk! :)
I have a bad feeling it's gonna fail again: Every job on that tester is failing
right now with "swarming.py is too old" :(
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...

[commit-bot: I haz the power, 2016-06-28 19:57:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 19:57:42]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Dirk Pranke, 2016-06-28 20:02:58]

On 2016/06/28 19:53:52, kraush wrote:
> On 2016/06/28 19:51:21, Dirk Pranke wrote:
> > On 2016/06/28 19:23:44, kraush wrote:
> > > Since this is a pure Android GN change, I'm pretty sure I didn't cause
those
> > > Windows test failures :/
> > > Is there a process on how to proceed?
> > > Can I just check the commit box again, or should I wait? (From what I can
> tell
> > > other Windows test jobs have passed since, so I don't think this is an
> ongoing
> > > issue)
> > 
> > The process is to check the box again and see what happens a second time :)
> > 
> > If the failures persist we should see what's actually going on.
> 
> Gotcha, thanks for the explanation Dirk! :)
> I have a bad feeling it's gonna fail again: Every job on that tester is
failing
> right now with "swarming.py is too old" :(
>
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...

Yeah, you probably just want to wait a while ...

[kraush, 2016-06-28 21:01:49]

On 2016/06/28 20:02:58, Dirk Pranke wrote:
> On 2016/06/28 19:53:52, kraush wrote:
> > On 2016/06/28 19:51:21, Dirk Pranke wrote:
> > > On 2016/06/28 19:23:44, kraush wrote:
> > > > Since this is a pure Android GN change, I'm pretty sure I didn't cause
> those
> > > > Windows test failures :/
> > > > Is there a process on how to proceed?
> > > > Can I just check the commit box again, or should I wait? (From what I
can
> > tell
> > > > other Windows test jobs have passed since, so I don't think this is an
> > ongoing
> > > > issue)
> > > 
> > > The process is to check the box again and see what happens a second time
:)
> > > 
> > > If the failures persist we should see what's actually going on.
> > 
> > Gotcha, thanks for the explanation Dirk! :)
> > I have a bad feeling it's gonna fail again: Every job on that tester is
> failing
> > right now with "swarming.py is too old" :(
> >
>
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> 
> Yeah, you probably just want to wait a while ...

It started succeeding on jobs again ~20 minutes ago.
I'll give it another shot :)

[kraush, 2016-06-28 21:01:56]

The CQ bit was checked by kraush@amazon.com

[commit-bot: I haz the power, 2016-06-28 21:03:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 22:43:03]

Description was changed from

==========
Remove proguard specific override block

Proguard config is already forwarded using a
separate mechanism. Remove this if-block
to get rid of the duplication.

BUG=623672
==========

to

==========
Remove proguard specific override block

Proguard config is already forwarded using a
separate mechanism. Remove this if-block
to get rid of the duplication.

BUG=623672
==========

[commit-bot: I haz the power, 2016-06-28 22:43:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-28 22:44:38]

Description was changed from

==========
Remove proguard specific override block

Proguard config is already forwarded using a
separate mechanism. Remove this if-block
to get rid of the duplication.

BUG=623672
==========

to

==========
Remove proguard specific override block

Proguard config is already forwarded using a
separate mechanism. Remove this if-block
to get rid of the duplication.

BUG=623672

Committed: https://crrev.com/ef53525821a41adb38fedd2e9b14deb12578da3d
Cr-Commit-Position: refs/heads/master@{#402578}
==========

[commit-bot: I haz the power, 2016-06-28 22:44:40]

Patchset 3 (id:??) landed as
https://crrev.com/ef53525821a41adb38fedd2e9b14deb12578da3d
Cr-Commit-Position: refs/heads/master@{#402578}