1523544 - Package exoplayer into a dependency

Status: RESOLVED FIXED

(GeckoView :: General, defect, P3)

Description

[hafizmohd013]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36

Steps to reproduce:

1)compile error from org.mozilla.geckoview:geckoview-${geckoviewChannel}-x86_64:${geckoviewVersion}
2)error: cannot find symbol class Factory and incompatible types: SimpleExoPlayer cannot be converted to Player as well as error: incompatible types: <anonymous com.google.android.exoplayer2.Player.EventListener> cannot be converted to com.google.android.exoplayer2.ExoPlayer.EventListener

Actual results:

I already own use exoplayer2 using my library and seem that it is conflict with class jar under org.mozilla.geckoview:geckoview-${geckoviewChannel}-x86_64:${geckoviewVersion} which is contain library exoplayer2 also. The question is that how to exclude this library from interrupt with my current library.

Expected results:

Your library exoplayer2 should not interrupt with my exoplayer2. Please do some thing

Comment 1

[Nick Alexander :nalexander [he/him]]

Your library exoplayer2 should not interrupt with my exoplayer2. Please do some thing

This isn't how Android (really, Java) works. It's just not true that if you use library X then you can use part of library X's namespace without consequences.

The version of exoplayer2 that GeckoView is using is both old and has been modified, so it's really not possible to share a copy of that code with consumers. That means we can either:

1. rename the package space for GeckoView's exoplayer2;
2. make you, the consumer, rename the package space for your exoplayer2.

I think 1) makes GV a better ecosystem participant. This should be the long-term goal: it looks like a big find-and-replace, followed by some manual testing.

I think 2) is possible right now: look into "shadow JARs"; I've been impressed with https://imperceptiblethoughts.com/shadow/ (although I haven't used it). That is, mangle your namespace such that you avoid this problem, 'cuz GV isn't going to do this quickly. (But I'd review a patch, if one was put forward!)

N.b.: if GV ever stops vendoring exoplayer2 into the tree and starts consuming exoplayer2 from a Maven publication, we should start shadow-relocating our version of exoplayer2 (or accept that consumers will change the version out from underneath us, which isn't really a bad thing).

Thanks for the bug report -- this is a thing that the GV team haven't really needed to think of yet!

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Nils, do we know how many people are still even hitting the HLS path in Gecko these days? I'm wondering if we could just nuke this.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

I'm gonna morph this into packaging exoplayer into a gradle dependency (that we vendor in our maven) so that consumers can override it if they need to.

Comment 5

[[ex-Mozilla] Agi Sferro | :agi]

Steps for this:

• Move the content of mobile/android/geckoview/src/thirdparty/ to mobile/android/exoplayer2
• Create build.gradle for exoplayer2. I would mostly pick pieces from geckoview's one: https://searchfox.org/mozilla-central/source/mobile/android/geckoview/build.gradle The version for exoplayer2 should be something like <exoplayer version>-<geckoview version> to avoid not picking up changes in the fork. Mostly port over the android section and the publishing section, add needed dependencies et
• Add basic AndroidManifest.xml for exoplayer (an empty manifest element should work)
• Add project(path: ':exoplayer') dependency to geckoview here: https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/mobile/android/geckoview/build.gradle#219
• Add :exoplayer2 to https://searchfox.org/mozilla-central/source/settings.gradle (similar to e.g. :annotations)
• Fix geckoview's pom file here: https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/mobile/android/geckoview/build.gradle#429-433 it needs to point to the right exoplayer id name and group id (we can use org.mozilla.geckoview since it's our fork)

at this point you should be able to publish locally with

./gradlew geckoview:publishWithGeckoBinariesDebugPublicationToMavenLocal
./gradlew exoplayer2:publishDebugPublicationToMavenLocal

and we can test that everything works correctly.

I'm not super familar with the code that controls actually publishing to maven.mozilla.org, maybe :jlorenzo could help us with that?

jlorenzo: We're trying to add a new artifact to maven.mozilla.org similar to geckoview (it will be a dependency) do you know what we need to do for that? I think this file could be a start https://searchfox.org/mozilla-central/source/taskcluster/ci/beetmover-geckoview/kind.yml#42 but I'm not sure how everything is tied together.

nalexander: ni you in case you have objections about moving exoplayer to it's own library.

Comment 6

[Johan Lorenzo [:jlorenzo] - On PTO, back on May 13th]

Hey Agi!

Good news: the steps you described should allow you to expose the maven artifacts on the build task[1]. This exposition is done thanks to this line [2]. You may want to add a new block, because I guess exoplayer won't be output to {HG_REPO}/obj-build/gradle/build/mobile/android/geckoview/maven/.

Once the build task exposes these new artifacts, you should update the artifact manifest[3]. This manifest is used across the jobs (i.e.: the signing job and the beetmover one) to know where to find artifacts and what to do with them. Then, beetmover-geckoview/kind.yml will know what to do and you should not need to change it. For the context, "beetmover" is a pun and describe a task that moves bits to a public and visible place (like maven.mozilla.org).

There's a simple way to test everything together: just run mach try fuzzy --full , select beetmover-geckoview-android-x86_64-nightly/opt, and try will tell you know!

I'm happy to provide more information and to review your patch! 🙂

[1] For instance https://firefox-ci-tc.services.mozilla.com/tasks/QzYaF2cFS8uwi2uJWgp4zQ#artifacts, see the artifacts starting with public/build/maven
[2] https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/taskcluster/ci/build/android.yml#16-18
[3] https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/taskcluster/taskgraph/manifests/fennec_geckoview.yml
[4] https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/taskcluster/ci/beetmover-geckoview/kind.yml

Comment 7

[Nick Alexander :nalexander [he/him]]

nalexander: ni you in case you have objections about moving exoplayer to it's own library.

None whatsoever, that's a fancy version of 1) in https://bugzilla.mozilla.org/show_bug.cgi?id=1523544#c1.

I would note that there's no real advantage to a separate library, since you can't really make the consumer (GV itself) conditional in a reasonable fashion, and since you can't really parallelize the build of the two libraries. So a simpler in-between step is just a giant sed s/.../.../ and a little testing.

Comment 8

[Nikita Kozhemiakin]

agi: I would like to be assigned to this task. Right now I'm waiting for a commit access approval (https://bugzilla.mozilla.org/show_bug.cgi?id=1635727).

nalexander: So a simpler in-between step is just a giant sed s/.../.../ and a little testing.

I think that this is an easiest way that will not break anything. But I'm not sure what package name should be used instead of com.google.android.exoplayer2.

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

Nikita: we usually assign a task to first time contributors after they have submitted a patch.

We can use org.mozilla.thirdparty.com.google.android.exoplayer2

Comment 10

[Nikita Kozhemiakin]

Attachment: Bug 1523544 - Change exoplayer2 package name.

Comment 11

[Nikita Kozhemiakin]

Attachment: Bug 1523544 - Restore exoplayer2 source files formatting.

Depends on D74068

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/458d6d29d94a
Change exoplayer2 package name. r=geckoview-reviewers,agi

Comment 13

[Nick Alexander :nalexander [he/him]]

Nikita: thanks for doing this!

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/458d6d29d94a