Closed Bug 1523544 Opened 2 years ago Closed 6 months ago

Package exoplayer into a dependency

Categories

(GeckoView :: General, defect, P3)

Unspecified
Android

Tracking

(firefox67 wontfix, firefox78 fixed)

RESOLVED FIXED
mozilla78
Tracking Status
firefox67 --- wontfix
firefox78 --- fixed

People

(Reporter: hafizmohd013, Assigned: enginegl.ec, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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

Component: Android Sync → General
Product: Firefox for Android → GeckoView
Summary: com.google.android.exoplayer2 → build conflict with com.google.android.exoplayer2

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!

Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: build conflict with com.google.android.exoplayer2 → Rename com.google.android.exoplayer2 in mobile/android to not conflict with consumer's exoplayer2 version
OS: Unspecified → Android
Priority: -- → P3

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.

Flags: needinfo?(drno)

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.

Summary: Rename com.google.android.exoplayer2 in mobile/android to not conflict with consumer's exoplayer2 version → Package exoplayer into a dependency
Mentor: agi

Steps for this:

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.

Flags: needinfo?(nalexander)
Flags: needinfo?(jlorenzo)

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

Flags: needinfo?(jlorenzo)

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.

Flags: needinfo?(nalexander)

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.

Flags: needinfo?(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

Flags: needinfo?(agi)
Assignee: nobody → enginegl.ec
Status: NEW → ASSIGNED

Depends on D74068

Attachment #9146429 - Attachment is obsolete: true
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/458d6d29d94a
Change exoplayer2 package name. r=geckoview-reviewers,agi

Nikita: thanks for doing this!

Flags: needinfo?(drno)
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
You need to log in before you can comment on or make changes to this bug.