Closed
Bug 1256995
Opened 8 years ago
Closed 8 years ago
Move --with-gradle to mobile/android/moz.configure
Categories
(Firefox Build System :: Android Studio and Gradle Integration, defect)
Firefox Build System
Android Studio and Gradle Integration
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: chmanchester, Assigned: nalexander)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
This is well-isolated.
Assignee | ||
Comment 1•8 years ago
|
||
This also adds a GRADLE_FLAGS environment variable for use in automation. Manually tested. Review commit: https://reviewboard.mozilla.org/r/40837/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40837/
Attachment #8731796 -
Flags: review?(cmanchester)
Reporter | ||
Updated•8 years ago
|
Attachment #8731796 -
Flags: review?(cmanchester)
Reporter | ||
Comment 2•8 years ago
|
||
Comment on attachment 8731796 [details] MozReview Request: Bug 1256995 - Move --with-gradle to mobile/android/moz.configure. r?glandium https://reviewboard.mozilla.org/r/40837/#review37373 ::: mobile/android/gradle.configure:17 (Diff revision 1) > + help='Enable building mobile/android with Gradle ' > + '(argument: location of binary or wrapper (gradle/gradlew))') > + > +@depends('--with-gradle', check_build_environment) > +def gradle(value, build_env): > + set_config('MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE', bool(value)) I'm having problems locally when I set booleans in a set_config ("Exception: ('Unhandled type %s', <type 'bool'>)" when writing to autoconf.mk. We want to support this, I'll get something on file when we figure out what's going on here. ::: mobile/android/gradle.configure:20 (Diff revision 1) > + if len(value): > + gradle = value[0] Can just be `if value:`. ::: mobile/android/gradle.configure:23 (Diff revision 1) > + > + gradle = os.path.join(build_env['TOPSRCDIR'], 'gradlew') > + if len(value): > + gradle = value[0] > + > + # TODO: verify that $GRADLE is executable. This seems like a candidate for util.configure.
Assignee | ||
Comment 3•8 years ago
|
||
https://reviewboard.mozilla.org/r/40837/#review37373 > I'm having problems locally when I set booleans in a set_config ("Exception: ('Unhandled type %s', <type 'bool'>)" when writing to autoconf.mk. > > We want to support this, I'll get something on file when we figure out what's going on here. I originally had this as `'1' if value else '0'`, but I was fairly certain that `CONFIG['BLAH']` would be true if it were `'0'`. Should this be integers? > Can just be `if value:`. Why? This is distinguishing `--with-gradle` from `--with-gradle=/path/to/gradle`.
Reporter | ||
Comment 4•8 years ago
|
||
https://reviewboard.mozilla.org/r/40837/#review37373 > I originally had this as `'1' if value else '0'`, but I was fairly certain that `CONFIG['BLAH']` would be true if it were `'0'`. Should this be integers? We've been using '1', or not setting anything for values. I'm curious why this is working for you though. > Why? This is distinguishing `--with-gradle` from `--with-gradle=/path/to/gradle`. Ok, I see.
Reporter | ||
Comment 5•8 years ago
|
||
https://reviewboard.mozilla.org/r/40837/#review37387 Is gradle.configure included anywhere?
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8731796 [details] MozReview Request: Bug 1256995 - Move --with-gradle to mobile/android/moz.configure. r?glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40837/diff/1-2/
Attachment #8731796 -
Flags: review?(cmanchester)
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcc21b073d9e
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8731796 [details] MozReview Request: Bug 1256995 - Move --with-gradle to mobile/android/moz.configure. r?glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40837/diff/2-3/
Comment 9•8 years ago
|
||
https://reviewboard.mozilla.org/r/40837/#review37411 ::: mobile/android/gradle.configure:11 (Diff revision 3) > + > +# If --with-gradle is specified, build mobile/android with Gradle. If no > +# Gradle binary is specified, use the in tree Gradle wrapper. The wrapper > +# downloads and installs Gradle, which is good for local developers but not > +# good in automation. > +option('--with-gradle', nargs='?', default=None, you should't need default=None here. ::: mobile/android/gradle.configure:39 (Diff revision 3) > +# offline mode. > +option(env='GRADLE_FLAGS', default='', help='Flags to pass to Gradle.') > + > +@depends('GRADLE_FLAGS') > +def gradle_flags(value): > + set_config('GRADLE_FLAGS', value[0]) technically, you still need a if value here and in the other function because GRADLE_FLAGS could explicitly set to nothing and that would call the function with an empty NegativeOptionValue
Comment 10•8 years ago
|
||
(In reply to Chris Manchester (:chmanchester) from comment #2) > I'm having problems locally when I set booleans in a set_config ("Exception: > ('Unhandled type %s', <type 'bool'>)" when writing to autoconf.mk. Yeah, config_status only deals with strings and lists when creating the ALL_SUBSTS list for autoconf.mk.
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8731796 [details] MozReview Request: Bug 1256995 - Move --with-gradle to mobile/android/moz.configure. r?glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40837/diff/3-4/
Attachment #8731796 -
Attachment description: MozReview Request: Bug 1256995 - Move --with-gradle to mobile/android/moz.configure. r?chmanchester → MozReview Request: Bug 1256995 - Move --with-gradle to mobile/android/moz.configure. r?glandium
Attachment #8731796 -
Flags: review?(cmanchester) → review?(mh+mozilla)
Updated•8 years ago
|
Attachment #8731796 -
Flags: review?(mh+mozilla)
Comment 12•8 years ago
|
||
Comment on attachment 8731796 [details] MozReview Request: Bug 1256995 - Move --with-gradle to mobile/android/moz.configure. r?glandium https://reviewboard.mozilla.org/r/40837/#review37491 ::: build.gradle:12 (Diff revision 4) > topsrcdir = gradle.mozconfig.topsrcdir > topobjdir = gradle.mozconfig.topobjdir > } > > repositories { > + if (gradle.mozconfig.substs.GRADLE_MAVEN_REPOSITORY) { does this work when GRADLE_MAVEN_REPOSITORY is not in substs (which it is not if the env variable is not set or empty)? ::: mobile/android/gradle.configure:8 (Diff revision 4) > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +# If --with-gradle is specified, build mobile/android with Gradle. If no > +# Gradle binary is specified, use the in tree Gradle wrapper. The wrapper if not gradle binary is specified, or in case of --without-gradle ::: mobile/android/gradle.configure:20 (Diff revision 4) > +@depends('--with-gradle', check_build_environment) > +def gradle(value, build_env): > + if value: > + set_config('MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE', '1') > + > + gradle = os.path.join(build_env['TOPSRCDIR'], 'gradlew') could go in a else ::: mobile/android/mach_commands.py:67 (Diff revision 4) > # In automation, JAVA_HOME is set via mozconfig, which needs > # to be specially handled in each mach command. This turns > # $JAVA_HOME/bin/java into $JAVA_HOME. > java_home = os.path.dirname(os.path.dirname(self.substs['JAVA'])) > > + gradle_flags = shlex.split(self.substs.get('GRADLE_FLAGS', '')) Don't use shlex.split. Use mozbuild.shellutil.split.
Assignee | ||
Comment 13•8 years ago
|
||
https://reviewboard.mozilla.org/r/40837/#review37719 ::: build.gradle:12 (Diff revision 4) > topsrcdir = gradle.mozconfig.topsrcdir > topobjdir = gradle.mozconfig.topobjdir > } > > repositories { > + if (gradle.mozconfig.substs.GRADLE_MAVEN_REPOSITORY) { Yes, the test works. The reference below fails, hence the guard.
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8731796 [details] MozReview Request: Bug 1256995 - Move --with-gradle to mobile/android/moz.configure. r?glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40837/diff/4-5/
Attachment #8731796 -
Flags: review?(mh+mozilla)
Comment 15•8 years ago
|
||
Comment on attachment 8731796 [details] MozReview Request: Bug 1256995 - Move --with-gradle to mobile/android/moz.configure. r?glandium https://reviewboard.mozilla.org/r/40837/#review38083 ::: mobile/android/mach_commands.py:19 (Diff revision 5) > MachCommandBase, > MachCommandConditions as conditions, > ) > > +from mozbuild.shellutil import ( > + split as shellsplit, we usually import it as shell_split (with an underscore)
Attachment #8731796 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1b70e9fe9db0c1829208283251e8edc308eee5f3 Bug 1256995 - Move --with-gradle to mobile/android/moz.configure. r=glandium
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1b70e9fe9db0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f3431739c79d
Assignee | ||
Updated•7 years ago
|
Component: Build Config → Build Config & IDE Support
Product: Core → Firefox for Android
Target Milestone: mozilla48 → ---
Updated•5 years ago
|
Product: Firefox for Android → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•