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

RESOLVED FIXED

Status

()

Firefox for Android
Build Config & IDE Support
RESOLVED FIXED
11 months ago
9 months ago

People

(Reporter: philor, Assigned: kikuo)

Tracking

55 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55+ fixed, firefox56 wontfix)

Details

MozReview Requests

()

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 months ago
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.

Updated

11 months ago
Blocks: 1350250
(Assignee)

Comment 1

11 months ago
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 ?
No longer blocks: 1350250
Flags: needinfo?(philringnalda)
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.
Blocks: 1350250
Tracking 55+ for this permared issue.
tracking-firefox55: ? → +
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?
Flags: needinfo?(nchen)
(Reporter)

Comment 5

11 months ago
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."
Flags: needinfo?(philringnalda)
(Assignee)

Comment 6

11 months ago
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.
Currently there is no way to make JNI bindings conditional. But for the next cycle, we can probably add preprocessing support for JNI bindings.
Flags: needinfo?(nchen)
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.
Flags: needinfo?(kikuo)
(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.
(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?
Flags: needinfo?(nchen)
Possibly-ignorant question, is preffing an option here?
(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.
(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?
Flags: needinfo?(nchen)
(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.
(Assignee)

Comment 15

11 months ago
Created attachment 8874305 [details]
Reversed_patch_for_Bug1350241.patch

A patch to revert Bug 1350241 in Beta.
(Assignee)

Comment 16

11 months ago
Created attachment 8874307 [details]
Reversed_patch_for_Bug1350250.patch

A patch to revert Bug 1350250 in Beta.
(Assignee)

Comment 17

11 months ago
Created attachment 8874308 [details]
Reversed_patch_for_Bug1350253.patch

A patch to revert Bug 1350253 in Beta.
(Assignee)

Comment 18

11 months ago
Comment on attachment 8874308 [details]
Reversed_patch_for_Bug1350253.patch

generated JNI wrappers should be re-generated.
Attachment #8874308 - Attachment is obsolete: true
Attachment #8874308 - Attachment is patch: false
(Assignee)

Comment 19

11 months ago
Comment on attachment 8874305 [details]
Reversed_patch_for_Bug1350241.patch

generated JNI wrappers should be re-generated ed.
Attachment #8874305 - Attachment is obsolete: true
Attachment #8874305 - Attachment is patch: false
(Assignee)

Comment 20

11 months ago
Comment on attachment 8874307 [details]
Reversed_patch_for_Bug1350250.patch

generated JNI wrappers should be re-generated ed.
Attachment #8874307 - Attachment is obsolete: true
Attachment #8874307 - Attachment is patch: false
Comment hidden (mozreview-request)
(Assignee)

Comment 22

11 months ago
(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 ?
Flags: needinfo?(kikuo)
(Assignee)

Comment 23

11 months ago
(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 ?
Flags: needinfo?(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.
Flags: needinfo?(ryanvm)

Comment 25

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/1db6ab82ac84
status-firefox55: affected → fixed
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.
Flags: needinfo?(kikuo)
(Assignee)

Comment 27

11 months ago
(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.
Flags: needinfo?(kikuo)
Thanks, sounds like we can go ahead and close this bug then.
Assignee: nobody → kikuo
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox56: --- → wontfix
Resolution: --- → FIXED
Depends on: 1378410
Blocks: 1387207
You need to log in before you can comment on or make changes to this bug.