Closed Bug 1348114 Opened 7 years ago Closed 7 years ago

./mach robocop should work --with-gradle

Categories

(Firefox for Android Graveyard :: Testing, enhancement, P3)

Product:
Firefox for Android Graveyard
Component:
Testing
Platform:
All
Android
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(1 file)

Bug 1348114 - Use correct default path for Robocop APK when building with Gradle.
59 bytes, text/x-review-board-request
nalexander
: review+
Details 
When locally running ./mach robocop, we're looking at the wrong location for the APK when building --with-gradle.

See also http://logs.glob.uno/?c=mozilla%23mobile#c621077.
Priority: -- → P3
Comment on attachment 8848791 [details]
Bug 1348114 - Use correct default path for Robocop APK when building with Gradle.

https://reviewboard.mozilla.org/r/121666/#review123784

::: testing/mochitest/mach_commands.py
(Diff revision 1)
>  
>          if not kwargs.get('robocopIni'):
>              kwargs['robocopIni'] = os.path.join(self.topobjdir, '_tests', 'testing',
>                                                  'mochitest', 'robocop.ini')
>  
> -        if not kwargs.get('robocopApk'):

Can you walk me through this?  I can't tell if this might break automation without doing some thinking, and I'm hoping you've already done this thinking.

::: testing/mochitest/mochitest_options.py:1029
(Diff revision 1)
>                      "Unable to find specified robocop .ini manifest '%s'" %
>                      options.robocopIni)
>              options.robocopIni = os.path.abspath(options.robocopIni)
>  
>              if not options.robocopApk and build_obj:
> +                if build_obj.substs['MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE']:

I think `MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE` is only subst-ed when `--enable-application=mobile/android` is given, which means this will break for Desktop users.  (I can tell this 'cuz it's in a `.configure` file in `mobile/android` -- see https://dxr.mozilla.org/mozilla-central/source/mobile/android/gradle.configure#20.)  Use `.substs.get('MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE')` to not require the key and return `None` if it's not defined, which is equivalent but less strict.

::: testing/mochitest/mochitest_options.py:1030
(Diff revision 1)
>                      options.robocopIni)
>              options.robocopIni = os.path.abspath(options.robocopIni)
>  
>              if not options.robocopApk and build_obj:
> +                if build_obj.substs['MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE']:
> +                    options.robocopApk = os.path.join(build_obj.topobjdir, 'dist', 'robocop.apk')

This will require a Gradle build and then a `mach package` to push it into `$OBJDIR/dist` each time.  If you find the APK as built by Gradle, then you won't need the `mach package` (and you'll always be "as fresh as Gradle", avoiding confusing version mismatches).  So it'd be better to figure out the path to the Gradle produced test APK.

However, there's a mismatch between Gradle and our build system here: most Gradle users build the `local` or `localOld` in the IDE, but I see from https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#238 and https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#480 that we always build `automation` when building with `--with-gradle`.  If I recall correctly, the `automation` Gradle test APK should work without `mach package`.

So: could you try finding the path to the `automation` APK, and see if that works, so we can build the tests with Gradle and avoid requiring `mach package` in this case.

Otherwise, tell me that's no good for some reason and this'll do nicely.
Attachment #8848791 - Flags: review?(nalexander) → review-
Comment on attachment 8848791 [details]
Bug 1348114 - Use correct default path for Robocop APK when building with Gradle.

https://reviewboard.mozilla.org/r/121666/#review123784

> Can you walk me through this?  I can't tell if this might break automation without doing some thinking, and I'm hoping you've already done this thinking.

My initial thought was that if possible it shouldn't be necessary to define a default path two different places - I originally modified mochitest_options.py and then wondered why it didn't have any effect.

Looking further, DXR shows `robocoApk` being used in only three files: mach_commands.py, runrobocop.py and mochitest_options.py. The call stack goes from [here](https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/testing/mochitest/mach_commands.py#462) to [here](https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/testing/mochitest/runrobocop.py#540) to `validate` and eventually ends up [here](https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/testing/mochitest/mochitest_options.py#1028):
>   /home/jan/Mozilla/mozilla-central/testing/mochitest/mach_commands.py(457)run_robocop()
>   
> -> return mochitest.run_robocop_test(self._mach_context, tests, 'robocop', **kwargs)
> 
>   /home/jan/Mozilla/mozilla-central/testing/mochitest/mach_commands.py(204)run_robocop_test()
>   
> -> return runrobocop.run_test_harness(parser, options)
> 
>   /home/jan/Mozilla/mozilla-central/objdir-droid/_tests/testing/mochitest/runrobocop.py(540)run_test_harness()
>   
> -> parser.validate(options)
> 
>   /home/jan/Mozilla/mozilla-central/objdir-droid/_tests/testing/mochitest/mochitest_options.py(1116)validate()
>   
> -> args = container.validate(self, args, self.context)
> 
> \> /home/jan/Mozilla/mozilla-central/objdir-droid/_tests/testing/mochitest/mochitest_options.py(1029)validate()

and I didn't spot any alternative codepaths, so it looks like all invocations of ./mach robocop go through mochitests_options.py. Therefore it should be safe to make that the single source of any default paths.

Also, looking through try logs and cross-checking with DXR it looks like automation passes in --robocop-apk anyway (compare [https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/testing/mozharness/configs/android/androidarm_4_3.py#208](https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/testing/mozharness/configs/android/androidarm_4_3.py#208)) and therefore doesn't rely on the default paths specified here.

> I think `MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE` is only subst-ed when `--enable-application=mobile/android` is given, which means this will break for Desktop users.  (I can tell this 'cuz it's in a `.configure` file in `mobile/android` -- see https://dxr.mozilla.org/mozilla-central/source/mobile/android/gradle.configure#20.)  Use `.substs.get('MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE')` to not require the key and return `None` if it's not defined, which is equivalent but less strict.

This gets checked from within an if block for `if options.robocopIni != ""`, so this should never be evaluated for Desktop users (unless they manually specify a robocop.ini path, which won't lead to anything usable anyway), but I've changed it just to be on the safe side.

> This will require a Gradle build and then a `mach package` to push it into `$OBJDIR/dist` each time.  If you find the APK as built by Gradle, then you won't need the `mach package` (and you'll always be "as fresh as Gradle", avoiding confusing version mismatches).  So it'd be better to figure out the path to the Gradle produced test APK.
> 
> However, there's a mismatch between Gradle and our build system here: most Gradle users build the `local` or `localOld` in the IDE, but I see from https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#238 and https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#480 that we always build `automation` when building with `--with-gradle`.  If I recall correctly, the `automation` Gradle test APK should work without `mach package`.
> 
> So: could you try finding the path to the `automation` APK, and see if that works, so we can build the tests with Gradle and avoid requiring `mach package` in this case.
> 
> Otherwise, tell me that's no good for some reason and this'll do nicely.

Automation APK hunted down and put to work on my phone - seems fine so far.
Comment on attachment 8848791 [details]
Bug 1348114 - Use correct default path for Robocop APK when building with Gradle.

https://reviewboard.mozilla.org/r/121666/#review124136

If it's green for you on try (including some Desktop targets), it's good for me.  Great work!
Attachment #8848791 - Flags: review?(nalexander) → review+
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/ec5d24c5c75f
Use correct default path for Robocop APK when building with Gradle. r=nalexander
https://hg.mozilla.org/mozilla-central/rev/ec5d24c5c75f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: