Closed Bug 1352599 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox Build System :: Android Studio and Gradle Integration, enhancement, P1)

Product:
Firefox Build System
Component:
Android Studio and Gradle Integration
Type:
enhancement

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1352599 - Part 1: Add a Proguard toolchain task for Android builds.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1352599 - Part 2: Add PROGUARD_JAR configure option.
59 bytes, text/x-review-board-request
chmanchester
: review+
rillian
: review+
Details
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.
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)
: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)
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)
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+
Blocks: 1370119
No longer blocks: gradle-automation-v1
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 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.
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)
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.
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: nobody → nalexander
Status: NEW → ASSIGNED
Depends on: 1407672
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)
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 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+
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/f88b59de4952 Follow-up: Fix Python lint errors. r=Aryx
Blocks: 1408159
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+
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
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 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+
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!
Attachment #8917942 - Attachment is obsolete: true
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 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
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Depends on: 1408643
Depends on: 1408644
Blocks: 1440428
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 58 → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: