1368925 - Permared build bustage on Android when Gecko 55 merges to beta on 2017-06-12 due to bug 1350241 and bug 1350253

Status: RESOLVED FIXED

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

Description

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bd51e245a622 updated GeneratedJNIWrappers.cpp/.h based on a "if CONFIG['MOZ_ANDROID_HLS_SUPPORT']" (which means ifdef NIGHTLY_BUILD) condition, so when we merge to mozilla-beta in less than two weeks, all the Fennec builds are going to burn, telling the nobody who has access to a machine to do so that they need to go to the objdir they don't have and run make -C /home/worker/workspace/build/src/obj-firefox/mobile/android/base update-generated-wrappers and push the resulting changes to mozilla-beta (with CLOSED TREE in the commit message, because we will be sitting closed because of this).

[Tracking Requested - why for this release]: merge build bustage, closed tree, delayed b1.

Comment 1

[Kilik Kuo [:kikuo] (inactive)]

Hi Phil, thanks for notification.

Bug 1350241 and Bug 1350253 are not necessary in Gecko 55 beta, we're targeting to get the feature tested in Gecko 56 nightly first.
Is it possible to back out these 2 bugs first on Beta ?

Comment 2

[James Cheng[:JamesCheng]]

Hi Phil,
Bug 1350250 also need to backout with the same reason as Kilik mentioned.

Bug 1350241, Bug 1350253 and Bug 1350250 these three bugs are built on nightly only so please backout these three bugs when you want to merge into beta.

Thanks.

Comment 3

[Marcia Knous [:marcia]]

Tracking 55+ for this permared issue.

Comment 4

[Nick Alexander :nalexander [he/him]]

jchen: how can we handle conditional native code in the JNI wrappers?

It seems like the plan is to back out the tickets on beta so this might not be urgent, but maybe we have a story for this already?

Comment 5

[Phil Ringnalda (:philor)]

Not that anyone has actually *agreed* to that plan, since "backout these three bugs when you want to merge into beta" isn't really any sort of an option. Merge to beta is a script that releng runs, 90% or more of the time either while I'm asleep or after I've gone to work, so practically speaking that plan is "merge build bustage to beta, and then hope that maybe someone will be around who realizes what the build bustage which is keeping the tree closed means and what they should back out to clear it."

Comment 6

[Kilik Kuo [:kikuo] (inactive)]

I've filled Bug 1368954 to refactor the part which affect the generated JNI wrappers if we are not confident enough about shipping our HLS implementation with ExoPlayer when Gekco 56 is merging to beta.

As nick said, I don't know if there's a way to handle conditional generated JNI wrappers. or maybe there's already a bug for that.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Currently there is no way to make JNI bindings conditional. But for the next cycle, we can probably add preprocessing support for JNI bindings.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

It's very unfortunate that we don't have a way to make this code conditional. In general, that runs afoul of our policies with respect to how new features should be landing on Nightly. It'll be great if that gets addressed during the 56 cycle.

Kilik, if our only real option here is to backout post-uplift, can you please prepare a patch for that ASAP so it can be tested and verified ready before the uplift? Also, be prepared to have to rebase it quickly between now and the uplift for whatever else happens to break it along the way. With Dawn being the new reality, we can't afford long delays in getting 55b1 created after the uplift.

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> It's very unfortunate that we don't have a way to make this code
> conditional. In general, that runs afoul of our policies with respect to how
> new features should be landing on Nightly. It'll be great if that gets
> addressed during the 56 cycle.

I agree, and you can see that I (and everyone else) got caught out: we haven't seen this exact situation yet.  I will spend a minute to see if we can finesse the situation.

> Kilik, if our only real option here is to backout post-uplift, can you
> please prepare a patch for that ASAP so it can be tested and verified ready
> before the uplift? Also, be prepared to have to rebase it quickly between
> now and the uplift for whatever else happens to break it along the way. With
> Dawn being the new reality, we can't afford long delays in getting 55b1
> created after the uplift.

In the meantime, Kilik, please prep these patches.

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #9)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> > It's very unfortunate that we don't have a way to make this code
> > conditional. In general, that runs afoul of our policies with respect to how
> > new features should be landing on Nightly. It'll be great if that gets
> > addressed during the 56 cycle.
> 
> I agree, and you can see that I (and everyone else) got caught out: we
> haven't seen this exact situation yet.  I will spend a minute to see if we
> can finesse the situation.

I was hoping there might be a small number of @WrapForJNI annotations guarded by MOZ_ANDROID_HLS_SUPPORT, but I see very many:

File: GeckoHlsDemuxerWrapper.java
16:37:import org.mozilla.gecko.annotation.WrapForJNI;
43:10:        @WrapForJNI(calledFrom = "gecko")
47:10:        @WrapForJNI
51:10:        @WrapForJNI
77:6:    @WrapForJNI
83:6:    @WrapForJNI(calledFrom = "gecko")
89:6:    @WrapForJNI
96:6:    @WrapForJNI
121:6:    @WrapForJNI
138:6:    @WrapForJNI
160:6:    @WrapForJNI
174:6:    @WrapForJNI
180:6:    @WrapForJNI
186:6:    @WrapForJNI // Called when native object is destroyed.

File: GeckoAudioInfo.java
8:37:import org.mozilla.gecko.annotation.WrapForJNI;
11:2:@WrapForJNI

File: GeckoHlsSample.java
11:37:import org.mozilla.gecko.annotation.WrapForJNI;
25:6:    @WrapForJNI
28:6:    @WrapForJNI
31:6:    @WrapForJNI
34:6:    @WrapForJNI
39:6:    @WrapForJNI
46:6:    @WrapForJNI
51:6:    @WrapForJNI

File: GeckoVideoInfo.java
8:37:import org.mozilla.gecko.annotation.WrapForJNI;
11:2:@WrapForJNI

That makes it much less attractive to try to have dummy entries for when !MOZ_ANDROID_HLS_SUPPORT.  But we could do that: have a stub implementation of these things that just declares the WrapForJNI entries.

Alternately, we could check two versions of the JNI declarations into the tree, and make the build system switch between them based on MOZ_ANDROID_HLS_SUPPORT.  Not pleasant, but then -- this whole system isn't pleasant.  jchen, what do you think?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Possibly-ignorant question, is preffing an option here?

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> Possibly-ignorant question, is preffing an option here?

Sort of.  We pay a reasonable APK price for shipping the code -- ~400kb, IIRC -- so we'd prefer to not ship the code if at all possible.  But if necessary we can always ship the code and just disable it at runtime, avoiding this issue.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Nick Alexander :nalexander from comment #10)
> Alternately, we could check two versions of the JNI declarations into the
> tree, and make the build system switch between them based on
> MOZ_ANDROID_HLS_SUPPORT.  Not pleasant, but then -- this whole system isn't
> pleasant.  jchen, what do you think?

I prefer this second option. We can have a second copy of "GeneratedJNIWrappers" just for comparison purposes, so we don't produce the "JNI binding changed" error; is this what you had in mind?

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #13)
> (In reply to Nick Alexander :nalexander from comment #10)
> > Alternately, we could check two versions of the JNI declarations into the
> > tree, and make the build system switch between them based on
> > MOZ_ANDROID_HLS_SUPPORT.  Not pleasant, but then -- this whole system isn't
> > pleasant.  jchen, what do you think?
> 
> I prefer this second option. We can have a second copy of
> "GeneratedJNIWrappers" just for comparison purposes, so we don't produce the
> "JNI binding changed" error; is this what you had in mind?

That is what I had in mind, although I assumed that we would also compile the flag-specific version (into mozglue) as appropriate.  I suppose the wrapper code is dynamic, so it doesn't actually need to change if the referenced Java symbols are not pressent.

Comment 15

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Reversed_patch_for_Bug1350241.patch

A patch to revert Bug 1350241 in Beta.

Comment 16

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Reversed_patch_for_Bug1350250.patch

A patch to revert Bug 1350250 in Beta.

Comment 17

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Reversed_patch_for_Bug1350253.patch

A patch to revert Bug 1350253 in Beta.

Comment 18

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8874308 [details]
Reversed_patch_for_Bug1350253.patch

generated JNI wrappers should be re-generated.

Comment 19

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8874305 [details]
Reversed_patch_for_Bug1350241.patch

generated JNI wrappers should be re-generated ed.

Comment 20

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8874307 [details]
Reversed_patch_for_Bug1350250.patch

generated JNI wrappers should be re-generated ed.

Comment 21

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1368925 - Reverse patch for Bug 1350241, Bug 1350250, Bug 1350253 to avoid PermanRed build bustage in Gecko 55 Beta.

Review commit: https://reviewboard.mozilla.org/r/145672/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145672/

Comment 22

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> It's very unfortunate that we don't have a way to make this code
> conditional. In general, that runs afoul of our policies with respect to how
> new features should be landing on Nightly. It'll be great if that gets
> addressed during the 56 cycle.
> 
> Kilik, if our only real option here is to backout post-uplift, can you
> please prepare a patch for that ASAP so it can be tested and verified ready
> before the uplift? Also, be prepared to have to rebase it quickly between
> now and the uplift for whatever else happens to break it along the way. With
> Dawn being the new reality, we can't afford long delays in getting 55b1
> created after the uplift.

Hi Ryan,

I attached this patch for reverting Bug 1350241, Bug 1350250, Bug 1350253 in Beta.
A try run based on this revision => https://hg.mozilla.org/try/rev/8a3aa1701537

I think this patch is not going to land in nightly. What should I do to test this in Beta ?

Comment 23

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Kilik Kuo [:kikuo] from comment #22)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> > It's very unfortunate that we don't have a way to make this code
> > conditional. In general, that runs afoul of our policies with respect to how
> > new features should be landing on Nightly. It'll be great if that gets
> > addressed during the 56 cycle.
> > 
> > Kilik, if our only real option here is to backout post-uplift, can you
> > please prepare a patch for that ASAP so it can be tested and verified ready
> > before the uplift? Also, be prepared to have to rebase it quickly between
> > now and the uplift for whatever else happens to break it along the way. With
> > Dawn being the new reality, we can't afford long delays in getting 55b1
> > created after the uplift.
> 
> Hi Ryan,
> 
> I attached this patch for reverting Bug 1350241, Bug 1350250, Bug 1350253 in
> Beta.
> A try run based on this revision =>
> https://hg.mozilla.org/try/rev/8a3aa1701537
> 
> I think this patch is not going to land in nightly. What should I do to test
> this in Beta ?

I'm not familiar with the post-uplift work-flow, or do you need 3 different reverse patches against each bug ?
So that when the uplift is in process, the script can merge the patch which leads to red build bustage and then apply the reversed-patch right after ?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

(In reply to Kilik Kuo [:kikuo] from comment #22)
> I think this patch is not going to land in nightly. What should I do to test
> this in Beta ?

We can let Phil add it to the Try pushes to ensure that it works as expected.

(In reply to Kilik Kuo [:kikuo] from comment #23)
> So that when the uplift is in process, the script can merge the patch which
> leads to red build bustage and then apply the reversed-patch right after ?

A single roll-up patch is fine. And yes, it'll need to be manually landed on Beta after the 55 uplift next week.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1db6ab82ac84

Comment 26

[Ryan VanderMeulen [:RyanVM]]

I'm leaving this bug open for now because I'm not sure if there's follow-up work you wanted to do for 56 in this bug or not. Feel free to close it if not.

Comment 27

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)
> I'm leaving this bug open for now because I'm not sure if there's follow-up
> work you wanted to do for 56 in this bug or not. Feel free to close it if
> not.

Thanks Ryan, I'm working on Bug 1368954 to prevent the situation in case we keep the feature nightly only if 56 nightly is going to beta.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Thanks, sounds like we can go ahead and close this bug then.