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

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: RyanVM, Assigned: kikuo)

Tracking

55 Branch
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox56+ fixed, firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
[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.
(Reporter)

Comment 2

a year ago
It'll require a revised patch if we want to go that route. The previous changeset has conflicts cross the board for 56.
(Assignee)

Comment 3

a year ago
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 hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
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)
(Reporter)

Comment 7

a year ago
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 hidden (mozreview-request)
(Assignee)

Comment 10

a year ago
mozreview-review-reply
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.

Comment 11

a year ago
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
(Assignee)

Comment 12

a year ago
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)

Updated

a year ago
Assignee: nobody → kikuo

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/87b6d99a490b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
tracking-firefox56: ? → +
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+
(Reporter)

Comment 15

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/8c9f02a4d6ad
status-firefox56: affected → fixed
You need to log in before you can comment on or make changes to this bug.