Closed Bug 1387207 Opened 7 years ago Closed 7 years ago

Permared Android build bustage on Gecko 56 since the merge to Beta

Categories

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

55 Branch
defect
Not set
normal

Tracking

(firefox56+ fixed, firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 + fixed
firefox57 --- fixed

People

(Reporter: RyanVM, Assigned: kikuo)

References

Details

Attachments

(1 file)

[Tracking Requested - why for this release]: Busted builds, closed Beta tree.

+++ This bug was initially created as a clone of Bug #1368925 +++

Same as the last cycle...

https://treeherder.mozilla.org/logviewer.html#?job_id=120785385&repo=mozilla-beta
Flags: needinfo?(kikuo)
We're working on a more permanent fix in bug 1378410, but IMO if the workaround from the last cycle still applies, we can go with that for 56.
It'll require a revised patch if we want to go that route. The previous changeset has conflicts cross the board for 56.
Weird, we're waiting for Bug 1365505 to be uplifted, so that the source code of Exoplayer could be included while building beta. So in this case, it's still in nightly-only env [1].

But, even MOZ_ANDROID_HLS_SUPPORT is not set yet, those busted files [2] should not be included in build coverage and it is green in "B" stage.

I'm not familiar with how "Deps" check works.
My speculation is that those busted files such as "GeckoHlsPlayer.java" are still included during "Deps" check because they are not explicitly excluded in gradle build system [3].

I need to exclude these files specifically in build.gradle too.
Nick, is that right ? 


[1] https://hg.mozilla.org/releases/mozilla-beta/file/c78102e7a5c2/mobile/android/moz.configure#l84
[2] http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/mobile/android/base/moz.build#466-473
[3] http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/mobile/android/geckoview/build.gradle#77
Flags: needinfo?(kikuo) → needinfo?(nalexander)
Comment on attachment 8893681 [details]
Bug 1387207 - Exclude code of GeckoHLS-related components if ExoPlayer source code is not included.

To fix the permared Android gradle build bustage, this patch should be uplifted to 56 Beta.
Flags: needinfo?(nalexander)
Looks like bug 1365505 took care of the immediate issue on Beta. Not sure if you still want to land the patch in this bug in light of that or not.
Comment on attachment 8893681 [details]
Bug 1387207 - Exclude code of GeckoHLS-related components if ExoPlayer source code is not included.

https://reviewboard.mozilla.org/r/164798/#review170836

::: mobile/android/geckoview/build.gradle:78
(Diff revision 1)
>          main {
>              java {
>                  srcDir "${topsrcdir}/mobile/android/geckoview/src/thirdparty/java"
>  
>                  if (!mozconfig.substs.MOZ_ANDROID_HLS_SUPPORT) {
> -                    exclude 'com/google/android/exoplayer2/**'
> +                    exclude "com/google/android/exoplayer2/**"

In Gradle, double quotes enable substitutions (like `${var}`) while single quotes do not.  Therefore, prefer single quotes if you are _not_ substituting, like here.
Attachment #8893681 - Flags: review?(nalexander) → review+
Comment on attachment 8893681 [details]
Bug 1387207 - Exclude code of GeckoHLS-related components if ExoPlayer source code is not included.

https://reviewboard.mozilla.org/r/164798/#review170836

> In Gradle, double quotes enable substitutions (like `${var}`) while single quotes do not.  Therefore, prefer single quotes if you are _not_ substituting, like here.

Thanks for the reivew.
Pushed by kikuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/87b6d99a490b
Exclude code of GeckoHLS-related components if ExoPlayer source code is not included. r=nalexander
Comment on attachment 8893681 [details]
Bug 1387207 - Exclude code of GeckoHLS-related components if ExoPlayer source code is not included.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1341990, Bug 1368954.

[User impact if declined]:
It would not impact to users.
If declined, once we decide to turn off Fennec HLS in beta again (e.g. rollback Bug 1365505), we'll still get busted for 'deps' check on try.

Considering code correctness logically, if we want to exclude the source of exoplayer by the 'MOZ_ANDROID_HLS_SUPPORT', we should also exclude those GeckoHls-related java implementation.

[Is this code covered by automated tests?]:
Yes, it would be covered while building Fennec.

[Has the fix been verified in Nightly?]:
Yes, nightly try result is green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c8cac65ac38 

Try results on Beta.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd93b24f5e081f2545b8b266630447cca81d8703
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e83d9fd36a5fe71c86cc43637ebc615ba587a905

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It actually fixed a mistake that we didn't exclude some related files which should not be included in beta when MOZ_ANDROID_HLS_SUPPORT is not defined in beta. It's a correction, and not risky.

[String changes made/needed]:
None
Attachment #8893681 - Flags: approval-mozilla-beta?
Assignee: nobody → kikuo
https://hg.mozilla.org/mozilla-central/rev/87b6d99a490b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment on attachment 8893681 [details]
Bug 1387207 - Exclude code of GeckoHLS-related components if ExoPlayer source code is not included.

Fix for failing tests, let's uplift this for beta 2.
Attachment #8893681 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 57 → mozilla57
You need to log in before you can comment on or make changes to this bug.