Use Proguard 5.3.2 to avoid Java .class incompatibilities across Java 1.7 and Java 1.8

RESOLVED FIXED in Firefox 58

Status

()

P1
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

unspecified
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

The issue is mentioned at https://bugzilla.mozilla.org/show_bug.cgi?id=1259098#c3.  I dug into the Android Gradle plugin from https://bintray.com/android/android-tools/com.android.tools.build.gradle-core/2.4.0-alpha and discovered that it depends on https://bintray.com/bintray/jcenter/net.sf.proguard%3Aproguard-base/5.3.2.  The Gradle task is not doing anything fancy; it's equivalent to just invoking the newer Proguard (perhaps with some configuration tweaks, which we can discover or reverse-engineer).

This ticket tracks bumping the Proguard version we use in automation.  Sadly, tools/proguard/lib/proguard.jar in the Android SDK is not the newer version, so we'll have to arrange to get Proguard ourselves.  PG is licensed GPL, so I'm not sure we can just check the JAR file into the tree; but we can definitely fetch it using the bootstrapper and find it at configure time.
(Assignee)

Comment 1

2 years ago
gerv: Proguard is a step in the Android toolchain, comparable to a compiler or linker that transforms input sources to final object binaries.  We use it only at compile time; no code contained in the Proguard JAR is incorporated as part of Firefox for Android.  Proguard is GPL licensed (see https://www.guardsquare.com/en/proguard).  Can we check the JAR into the tree, which would be the easiest way to consume in the build system?  Or is that unacceptable distribution, and we'll need to arrange for it to be installed as part of the bootstrapping/build configuration process?

Sorry if this is documented -- I looked for documentation (and didn't see anything about GPL-in-MPL) and I looked for prior art in the thirdparty licenses Wiki (and didn't see any GPL licenses that weren't _also_ licensed something like Apache or MPL as well).
Flags: needinfo?(gerv)
(Assignee)

Comment 2

2 years ago
:gps: The Proguard JAR is about 900kb.  The entire ZIP is about 2.5Mb.  Are files of that size OK to check into the tree?  Or are those considered too large these days?

These would be very infrequently updated.
Flags: needinfo?(gps)

Comment 3

2 years ago
I would prefer we not check in this Java JAR file at this time because its:

a) essentially a binary blob derived from source code (I'd rather check in original assets)
b) only used by a smaller subset of developers
c) new

It isn't much work to stuff something like this into tooltool and use an in-tree manifest to track which blob in tooltool contains the Proguard JAR and to download that to ~/.mozbuild as needed. The ESLint integration works something like this IIRC. Although, I'm not sure it uses tooltool as part of the mach command. It should.

So you might be on the hook for writing a helper in MachCommandBase that takes the path to a tooltool manifest and fetches blobs as needed. I don't think that will be too much work. You should check with #build and/or #ateam if someone has written this code. I suspect someone has but I can't find it :/
Flags: needinfo?(gps)
If we are distributing GPLed software, either in a source code repo or some other tool, we need to make sure we meet the distribution requirements. Wikipedia claims this is GPLv2, although I can't find confirmation of that on guardsquare.com. 

If we distribute a binary we either have to distribute the source code along with it, or a written offer of source. The written offer thing is a pain; it's much better to make the source available. So either we should check that in alongside the JAR, or if we do the tooltool thing, make it so tooltool can also provide source. (We don't have to force source on people if our distribution mechanism allows them to pick and choose.)

LMK if you need more.

Gerv
Flags: needinfo?(gerv)
Flags: needinfo?(cnevinchen)
Flags: needinfo?(cnevinchen)
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

a year ago
mozreview-review
Comment on attachment 8914530 [details]
Bug 1352599 - Part 1: Add a Proguard toolchain task for Android builds.

https://reviewboard.mozilla.org/r/185844/#review191042

WFM.  So nice to not have to edit tooltool manifests anymore.  Two small suggestions below.

::: commit-message-15f22:1
(Diff revision 1)
> +Bug 1352599 - Part 1: Add and use a Proguard toolchain in Android builds. r=froydnj

Maybe just "Add a Proguard toolchain to Android builds", since this patch is not the one making use of it?

::: taskcluster/scripts/misc/repack-proguard-jar.sh:11
(Diff revision 1)
> +URL=https://newcontinuum.dl.sourceforge.net/project/proguard/proguard/5.3/proguard5.3.3.tar.gz
> +SHA256SUM=95bf9580107f00d0e26f01026dcfe9e7a772e5449488b03ba832836c3760b3af
> +ARCHIVE=proguard5.3.3.tar.gz
> +DIR=proguard5.3.3

Is it worth separating things out here to avoid repeating information, e.g.:

```
PROGUARD_VERSION=5.3.3
ARCHIVE=${PROGUARD_VERSION}.tar.gz
URL=.../${ARCHIVE}
DIR=${PROGUARD_VERSION}
```

or something?
Attachment #8914530 - Flags: review?(nfroyd) → review+
(Assignee)

Updated

a year ago
Blocks: 1370119
(Assignee)

Updated

a year ago
Blocks: 1405376
(Assignee)

Updated

a year ago
No longer blocks: 1405376

Comment 15

a year ago
mozreview-review
Comment on attachment 8914531 [details]
Bug 1352599 - Part 2: Add PROGUARD_JAR configure option.

https://reviewboard.mozilla.org/r/185846/#review191224

::: build/moz.configure/java.configure:94
(Diff revision 2)
> +            '`export PROGUARD_JAR=/path/to/proguard.jar` to your mozconfig.' % proguard_jar[0])
> +
> +    try:
> +        # Exit code zero shouldn't happen, but roll with it.
> +        output = subprocess.check_output([java, '-jar', proguard_jar[0]])
> +

So is this ever going to do what we want with exit code zero? If not maybe we should `die` here.

::: build/moz.configure/java.configure:110
(Diff revision 2)
> +                'Run |mach bootstrap| to upgrade. ' % version)
> +
> +        return proguard_jar[0]
> +
> +    except Exception as e:
> +        die('Failed to determine proguard.jar version: %s' % e)

Which part of the block above might raise? Catching whatever exception happens here might actually be less informative, I don't think `die` prints a stack trace.
Attachment #8914531 - Flags: review?(cmanchester) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

a year ago
mozreview-review-reply
Comment on attachment 8914530 [details]
Bug 1352599 - Part 1: Add a Proguard toolchain task for Android builds.

https://reviewboard.mozilla.org/r/185844/#review191042

> Is it worth separating things out here to avoid repeating information, e.g.:
> 
> ```
> PROGUARD_VERSION=5.3.3
> ARCHIVE=${PROGUARD_VERSION}.tar.gz
> URL=.../${ARCHIVE}
> DIR=${PROGUARD_VERSION}
> ```
> 
> or something?

I did this -- just extracting the VERSION out.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Callek: glandium: how do toolchains interact with these funky l10n tasks like the failing job at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b68380348059f3ef3ccf22bcd3ca21e7cc5b3f25&selectedJob=135428502?  It looks like the definitions in http://searchfox.org/mozilla-central/source/taskcluster/ci/l10n/kind.yml don't "pick up" the toolchains from the underlying jobs.  (I may be wrong about this, and the underlying jobs are incorrectly configured as well -- but I don't think so.)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(bugspam.Callek)
(Assignee)

Comment 23

a year ago
mozreview-review-reply
Comment on attachment 8914531 [details]
Bug 1352599 - Part 2: Add PROGUARD_JAR configure option.

https://reviewboard.mozilla.org/r/185846/#review191224

> So is this ever going to do what we want with exit code zero? If not maybe we should `die` here.

I was going to be permissive, but I've changed to `die` instead.

> Which part of the block above might raise? Catching whatever exception happens here might actually be less informative, I don't think `die` prints a stack trace.

The `split` or version extraction could fail.  I've changed to not catch the exception for simplicity.
(Assignee)

Updated

a year ago
Depends on: 1406572
(In reply to Nick Alexander :nalexander from comment #22)
> Callek: glandium: how do toolchains interact with these funky l10n tasks
> like the failing job at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b68380348059f3ef3ccf22bcd3ca21e7cc5b3f25&selectedJob=1
> 35428502?  It looks like the definitions in
> http://searchfox.org/mozilla-central/source/taskcluster/ci/l10n/kind.yml
> don't "pick up" the toolchains from the underlying jobs.  (I may be wrong
> about this, and the underlying jobs are incorrectly configured as well --
> but I don't think so.)

I think you're right... 

If you put taskgraph.transforms.use_toolchains:transforms into the transform list above the "job" transform, I think it will work if you also specify the toolchains you need.

I should additionally note, though, that the l10n code is desiring as an end state to not need a compile-environment. So the list of toolchains l10n should need should be vanishingly small, and in new cases where something added to the build breaks l10n, we should understand what l10n needs of said thing in order to work. If its just to pass configure we should instead set it to the no-compile configure file.  (https://dxr.mozilla.org/mozilla-central/source/build/mozconfig.no-compile)
Flags: needinfo?(bugspam.Callek)
(In reply to Justin Wood (:Callek) from comment #24)
> (In reply to Nick Alexander :nalexander from comment #22)
> > Callek: glandium: how do toolchains interact with these funky l10n tasks
> > like the failing job at
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=b68380348059f3ef3ccf22bcd3ca21e7cc5b3f25&selectedJob=1
> > 35428502?  It looks like the definitions in
> > http://searchfox.org/mozilla-central/source/taskcluster/ci/l10n/kind.yml
> > don't "pick up" the toolchains from the underlying jobs.  (I may be wrong
> > about this, and the underlying jobs are incorrectly configured as well --
> > but I don't think so.)
> 
> I think you're right... 
> 
> If you put taskgraph.transforms.use_toolchains:transforms into the transform
> list above the "job" transform, I think it will work if you also specify the
> toolchains you need.
> 
> I should additionally note, though, that the l10n code is desiring as an end
> state to not need a compile-environment. So the list of toolchains l10n
> should need should be vanishingly small, and in new cases where something
> added to the build breaks l10n, we should understand what l10n needs of said
> thing in order to work. If its just to pass configure we should instead set
> it to the no-compile configure file. 
> (https://dxr.mozilla.org/mozilla-central/source/build/mozconfig.no-compile)

Nick, I also looked at the try push and I see where the issue lies, I didn't pass down toolchains from the l10n transform to the toolchain transform, you need to modify this specific transform:

https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/l10n.py#365
Flags: needinfo?(mh+mozilla)
(Assignee)

Updated

a year ago
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Depends on: 1407672
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

a year ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e33478dc9b8b -d 7a93b841a26f: rebasing 426085:e33478dc9b8b "Bug 1352599 - Part 1: Add a Proguard toolchain task for Android builds. r=froydnj"
rebasing 426086:3218f0a4730c "Bug 1352599 - Part 2: Add PROGUARD_JAR configure option. r=chmanchester" (tip)
merging build/moz.configure/java.configure
warning: conflicts while merging build/moz.configure/java.configure! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

a year ago
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52a05ad77fa5
Part 1: Add a Proguard toolchain task for Android builds. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/1da1df814ad3
Part 2: Add PROGUARD_JAR configure option. r=chmanchester
Comment hidden (mozreview-request)
Comment on attachment 8917942 [details]
Bug 1352599 - Follow-up: Fix Python lint errors and tests.

https://reviewboard.mozilla.org/r/188844/#review194166

Thanky you for the fixes.
Attachment #8917942 - Flags: review?(aryx.bugmail) → review+

Comment 36

a year ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/f88b59de4952
Follow-up: Fix Python lint errors. r=Aryx
(Assignee)

Updated

a year ago
Blocks: 1408159
Comment hidden (mozreview-request)
Comment on attachment 8917996 [details]
Bug 1352599 - Post: Disable failing java.configure test.

https://reviewboard.mozilla.org/r/188888/#review194210
Attachment #8917996 - Flags: review?(aryx.bugmail) → review+

Comment 39

a year ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/cb5f4236d080
Post: Disable failing java.configure test. r=Aryx on a CLOSED TREE
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8917996 - Attachment is obsolete: true
Backed out bug 1352599 and bug 1405412 for failing build at python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_mobile_android:

bug 1352599
https://hg.mozilla.org/integration/autoland/rev/6c9706f9f534eb684abbb11c72cdb6f31d94c272
https://hg.mozilla.org/integration/autoland/rev/fbc942d188228d69b3f6d33eeaa45cce2a9e808a
https://hg.mozilla.org/integration/autoland/rev/64a408bde7d2f24606f0806ce36f27480251a442
https://hg.mozilla.org/integration/autoland/rev/ca38508b320378f568c295da00c90ae6edcd9ffd

bug 1405412
https://hg.mozilla.org/integration/autoland/rev/11376fd5366ebe76c926e2e919245e5039c0b9e4
https://hg.mozilla.org/integration/autoland/rev/a6fab9d472152a3582869bac6088eb31b2957403
https://hg.mozilla.org/integration/autoland/rev/0e81b421af3564b89e2a9a646af24d83c36ba9fc

Latest push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cb5f4236d080bb250f7f485a584216f014a18fba&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=136603646&repo=autoland

[task 2017-10-12T21:17:52.266Z] 21:17:52  WARNING - ../python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_mobile_android TEST-UNEXPECTED-FAIL
[task 2017-10-12T21:17:52.266Z] 21:17:52     INFO - =================================== FAILURES ===================================
[task 2017-10-12T21:17:52.266Z] 21:17:52     INFO - ___________________________ Lint.test_mobile_android ___________________________
[task 2017-10-12T21:17:52.266Z] 21:17:52     INFO - self = <lint.Lint testMethod=test_mobile_android>
[task 2017-10-12T21:17:52.266Z] 21:17:52     INFO -     def test(self):
[task 2017-10-12T21:17:52.267Z] 21:17:52     INFO - >       return func(self, project)
[task 2017-10-12T21:17:52.267Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/test/configure/lint.py:26:
[task 2017-10-12T21:17:52.267Z] 21:17:52     INFO - _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
[task 2017-10-12T21:17:52.267Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/test/configure/lint.py:58: in lint
[task 2017-10-12T21:17:52.267Z] 21:17:52     INFO -     sandbox.run(os.path.join(topsrcdir, 'moz.configure'))
[task 2017-10-12T21:17:52.268Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/configure/lint.py:35: in run
[task 2017-10-12T21:17:52.268Z] 21:17:52     INFO -     self.include_file(path)
[task 2017-10-12T21:17:52.268Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/configure/__init__.py:389: in include_file
[task 2017-10-12T21:17:52.268Z] 21:17:52     INFO -     exec_(code, self)
[task 2017-10-12T21:17:52.268Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/util.py:59: in exec_
[task 2017-10-12T21:17:52.269Z] 21:17:52     INFO -     exec(object, globals, locals)
[task 2017-10-12T21:17:52.269Z] 21:17:52     INFO - ../moz.configure:225: in <module>
[task 2017-10-12T21:17:52.269Z] 21:17:52     INFO -     include(include_project_configure)
[task 2017-10-12T21:17:52.269Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/configure/__init__.py:693: in include_impl
[task 2017-10-12T21:17:52.269Z] 21:17:52     INFO -     self.include_file(what)
[task 2017-10-12T21:17:52.270Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/configure/__init__.py:389: in include_file
[task 2017-10-12T21:17:52.270Z] 21:17:52     INFO -     exec_(code, self)
[task 2017-10-12T21:17:52.270Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/util.py:59: in exec_
[task 2017-10-12T21:17:52.270Z] 21:17:52     INFO -     exec(object, globals, locals)
[task 2017-10-12T21:17:52.270Z] 21:17:52     INFO - ../mobile/android/moz.configure:130: in <module>
[task 2017-10-12T21:17:52.271Z] 21:17:52     INFO -     include('../../build/moz.configure/java.configure')
[task 2017-10-12T21:17:52.271Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/configure/__init__.py:693: in include_impl
[task 2017-10-12T21:17:52.271Z] 21:17:52     INFO -     self.include_file(what)
[task 2017-10-12T21:17:52.271Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/configure/__init__.py:389: in include_file
[task 2017-10-12T21:17:52.271Z] 21:17:52     INFO -     exec_(code, self)
[task 2017-10-12T21:17:52.272Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/util.py:59: in exec_
[task 2017-10-12T21:17:52.272Z] 21:17:52     INFO -     exec(object, globals, locals)
[task 2017-10-12T21:17:52.272Z] 21:17:52     INFO - ../build/moz.configure/java.configure:86: in <module>
[task 2017-10-12T21:17:52.272Z] 21:17:52     INFO -     help='Path to proguard.jar')
[task 2017-10-12T21:17:52.272Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/configure/lint.py:121: in option_impl
[task 2017-10-12T21:17:52.272Z] 21:17:52     INFO -     result = super(LintSandbox, self).option_impl(*args, **kwargs)
[task 2017-10-12T21:17:52.273Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/configure/__init__.py:610: in option_impl
[task 2017-10-12T21:17:52.273Z] 21:17:52     INFO -     kwargs = {k: self._resolve(v) for k, v in kwargs.iteritems()
[task 2017-10-12T21:17:52.273Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/configure/__init__.py:611: in <dictcomp>
[task 2017-10-12T21:17:52.273Z] 21:17:52     INFO -     if k != 'when'}
[task 2017-10-12T21:17:52.273Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/configure/__init__.py:463: in _resolve
[task 2017-10-12T21:17:52.274Z] 21:17:52     INFO -     need_help_dependency)
[task 2017-10-12T21:17:52.274Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/util.py:944: in method_call
[task 2017-10-12T21:17:52.274Z] 21:17:52     INFO -     cache[args] = self.func(instance, *args)
[task 2017-10-12T21:17:52.274Z] 21:17:52     INFO - _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
[task 2017-10-12T21:17:52.275Z] 21:17:52     INFO - self = {'msvs_version': <mozbuild.configure.SandboxDependsFunction object at 0x7f55f4...android': <mozbuild.configure.SandboxDependsFunction object at 0x7f55f40e29d0>}
[task 2017-10-12T21:17:52.275Z] 21:17:52     INFO - obj = <mozbuild.configure.DependsFunction proguard_jar_default()>
[task 2017-10-12T21:17:52.275Z] 21:17:52     INFO - need_help_dependency = True
[task 2017-10-12T21:17:52.275Z] 21:17:52     INFO -     @memoize
[task 2017-10-12T21:17:52.276Z] 21:17:52     INFO -     def _value_for_depends(self, obj, need_help_dependency=False):
[task 2017-10-12T21:17:52.276Z] 21:17:52     INFO -         with_help = self._help_option in obj.dependencies
[task 2017-10-12T21:17:52.276Z] 21:17:52     INFO -         if with_help:
[task 2017-10-12T21:17:52.276Z] 21:17:52     INFO -             for arg in obj.dependencies:
[task 2017-10-12T21:17:52.276Z] 21:17:52     INFO -                 if self._missing_help_dependency(arg):
[task 2017-10-12T21:17:52.277Z] 21:17:52     INFO -                     raise ConfigureError(
[task 2017-10-12T21:17:52.277Z] 21:17:52     INFO -                         "`%s` depends on '--help' and `%s`. "
[task 2017-10-12T21:17:52.277Z] 21:17:52     INFO -                         "`%s` must depend on '--help'"
[task 2017-10-12T21:17:52.277Z] 21:17:52     INFO -                         % (obj.name, arg.name, arg.name))
[task 2017-10-12T21:17:52.277Z] 21:17:52     INFO -         elif ((self._help or need_help_dependency) and
[task 2017-10-12T21:17:52.278Z] 21:17:52     INFO -               self._missing_help_dependency(obj)):
[task 2017-10-12T21:17:52.278Z] 21:17:52     INFO -             raise ConfigureError("Missing @depends for `%s`: '--help'" %
[task 2017-10-12T21:17:52.278Z] 21:17:52     INFO - >                                obj.name)
[task 2017-10-12T21:17:52.278Z] 21:17:52     INFO - E           ConfigureError: Missing @depends for `proguard_jar_default`: '--help'
[task 2017-10-12T21:17:52.279Z] 21:17:52     INFO - ../python/mozbuild/mozbuild/configure/lint.py:116: ConfigureError
Flags: needinfo?(nalexander)
This is looking healthy on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f4721981ef6aad2e4f8f1d7ddcf72c7b69ece9a.  (I hope that's enough.)
Flags: needinfo?(nalexander)

Comment 43

a year ago
mozreview-review
Comment on attachment 8917942 [details]
Bug 1352599 - Follow-up: Fix Python lint errors and tests.

https://reviewboard.mozilla.org/r/188844/#review194518

LGTM with issues addressed. I didn't verify it passes flake8, since automation will do that.

::: commit-message-4885f:3
(Diff revision 2)
> +Bug 1352599 - Follow-up: Fix Python lint errors and tests. r=chmanchester
> +
> +This required to extend the fragile test sandbox yet further, but it's

This ^is^ required.

::: build/moz.configure/java.configure:79
(Diff revision 2)
>  @imports('os')
> -def proguard_jar_default():
> +def proguard_jar_default(_):
>      # By default, look for proguard.jar in the location to which `mach
>      # bootstrap` or `mach artifact toolchain` will install Proguard.
> -    mozbuild_state_dir = os.environ.get('MOZBUILD_STATE_PATH',
> -                                        os.path.expanduser(os.path.join('~', '.mozbuild')))
> +    default = os.path.expanduser(os.path.join('~', '.mozbuild'))
> +    mozbuild_state_dir = os.environ.get('MOZBUILD_STATE_PATH', default)

You could
```
from mozboot.util import get_state_dir
return os.path.join(get_state_dir()[0], ...)
```
here to avoid code duplication. I'm not terribly fussed though, so up to you.

::: python/mozbuild/mozbuild/test/configure/test_checks_configure.py:495
(Diff revision 2)
>          keytool = mozpath.abspath('/usr/bin/keytool')
> +        proguard_jar = mozpath.abspath('/path/to/proguard.jar')
> +        old_proguard_jar = mozpath.abspath('/path/to/old_proguard.jar')
> +
> +        def mock_valid_java(_, args):
> +            print(args)

Did you mean to leave this in? `mock_valid_javac` doesn't dump its arguments.

::: python/mozbuild/mozbuild/test/configure/test_checks_configure.py:503
(Diff revision 2)
> +                    stdout = \
> +                         'ProGuard, version 5.3.3' + \
> +                         'Usage: java proguard.ProGuard [options ...]'
> +                else:
> +                    stdout = \
> +                         'ProGuard, version 5.3.2' + \

The progard version variant based on jar path is confusing. Maybe document the expectations with a comment?

If the idea is just to return something which won't match expectations if it's called with another jarfile, maybe a better `else` would be a more explicit 'unknown jar file' message?


You might also prefer:
```python
# Comment explaining versioning expectations.
if args[1] == progard_jar:
    version = '5.3.3'
else:
    version = '5.4.2'    
stdout = \
    'Proguard version {}' \
    'Usage: java proguard.Progard [options ...]' \
    .format(version)
```
Attachment #8917942 - Flags: review+
(Assignee)

Comment 44

a year ago
mozreview-review
Comment on attachment 8917942 [details]
Bug 1352599 - Follow-up: Fix Python lint errors and tests.

https://reviewboard.mozilla.org/r/188844/#review194584

::: build/moz.configure/java.configure:79
(Diff revision 2)
>  @imports('os')
> -def proguard_jar_default():
> +def proguard_jar_default(_):
>      # By default, look for proguard.jar in the location to which `mach
>      # bootstrap` or `mach artifact toolchain` will install Proguard.
> -    mozbuild_state_dir = os.environ.get('MOZBUILD_STATE_PATH',
> -                                        os.path.expanduser(os.path.join('~', '.mozbuild')))
> +    default = os.path.expanduser(os.path.join('~', '.mozbuild'))
> +    mozbuild_state_dir = os.environ.get('MOZBUILD_STATE_PATH', default)

I culted this from a `moz.configure` elsewhere in the tree, and I quite like the grep for `MOZBUILD_STATE_PATH`, so I'll keep this for now.

::: python/mozbuild/mozbuild/test/configure/test_checks_configure.py:495
(Diff revision 2)
>          keytool = mozpath.abspath('/usr/bin/keytool')
> +        proguard_jar = mozpath.abspath('/path/to/proguard.jar')
> +        old_proguard_jar = mozpath.abspath('/path/to/old_proguard.jar')
> +
> +        def mock_valid_java(_, args):
> +            print(args)

I did not!

::: python/mozbuild/mozbuild/test/configure/test_checks_configure.py:503
(Diff revision 2)
> +                    stdout = \
> +                         'ProGuard, version 5.3.3' + \
> +                         'Usage: java proguard.ProGuard [options ...]'
> +                else:
> +                    stdout = \
> +                         'ProGuard, version 5.3.2' + \

Done!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8917942 - Attachment is obsolete: true

Comment 49

a year ago
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/366262985fa8
Part 1: Add a Proguard toolchain task for Android builds. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/4f50c65e62c3
Part 2: Add PROGUARD_JAR configure option. r=chmanchester

Comment 50

a year ago
mozreview-review
Comment on attachment 8914531 [details]
Bug 1352599 - Part 2: Add PROGUARD_JAR configure option.

https://reviewboard.mozilla.org/r/185846/#review194596
Attachment #8914531 - Flags: review?(giles) → review+
https://hg.mozilla.org/mozilla-central/rev/366262985fa8
https://hg.mozilla.org/mozilla-central/rev/4f50c65e62c3
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Updated

a year ago
Depends on: 1408643

Updated

a year ago
Depends on: 1408644
(Assignee)

Updated

9 months ago
Blocks: 1440428
You need to log in before you can comment on or make changes to this bug.