Closed Bug 1256995 Opened 4 years ago Closed 4 years ago

Move --with-gradle to mobile/android/moz.configure

Categories

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

defect
Not set

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.
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)
Attachment #8731796 - Flags: review?(cmanchester)
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.
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`.
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.
https://reviewboard.mozilla.org/r/40837/#review37387

Is gradle.configure included anywhere?
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)
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/
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
(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.
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)
Attachment #8731796 - Flags: review?(mh+mozilla)
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.
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.
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 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: nobody → nalexander
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/1b70e9fe9db0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: Build Config → Build Config & IDE Support
Product: Core → Firefox for Android
Target Milestone: mozilla48 → ---
Product: Firefox for Android → Firefox Build System
You need to log in before you can comment on or make changes to this bug.