Closed Bug 1395688 Opened 2 years ago Closed 2 years ago

Fennec release builds no longer minify JS starting with 54

Categories

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

All
Android
enhancement
Not set

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(1 file)

I noticed we no longer minify our JS code somewhere between 53 and 54 releases.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Comment on attachment 8908428 [details]
Bug 1395688 - Update jsshell for minifying Fennec JS code;

https://reviewboard.mozilla.org/r/180064/#review185466

I'll trust you that this works.

However, I have some questions and requests.

1) can you explain how you deduced that it was a jsshell issue?
2) how could we check that this "hidden dependency" doesn't silently break us in the future?  Why didn't the APK size alerting tell us something changed?  (Did it, and we ignored it?)
3) how did you produce the new jsshell.tar.xz?  Did you manually build it, did you fetch it from a known-good source, did you upload to tooltool yourself?  (In the future, we could make this a toolchain task, so it's more clear where the input to the build is coming from.)

Thanks for jumping on this!
Attachment #8908428 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander [parental leave until September 15th] from comment #2)
> Comment on attachment 8908428 [details]
> Bug 1395688 - Update jsshell for minifying Fennec JS code;
> 
> https://reviewboard.mozilla.org/r/180064/#review185466
> 
> I'll trust you that this works.
> 
> However, I have some questions and requests.
> 
> 1) can you explain how you deduced that it was a jsshell issue?

Yeah, I should have explained more. Originally, I figured that assuming it's working correctly for desktop, the only difference between desktop and mobile is the host jsshell. I then pushed a new version of the jsshell, and confirmed on try that the issue _is_ due to an outdated jsshell.

> 2) how could we check that this "hidden dependency" doesn't silently break
> us in the future?  Why didn't the APK size alerting tell us something
> changed?  (Did it, and we ignored it?)

I think it's because we only watch Nightly for APK increases, and we don't minify Nightly code. I guess we can have some kind of test that looks at the minification results and tell us if we regress. Not exactly sure how it'd be implemented.

> 3) how did you produce the new jsshell.tar.xz?  Did you manually build it,
> did you fetch it from a known-good source, did you upload to tooltool
> yourself?  (In the future, we could make this a toolchain task, so it's more
> clear where the input to the build is coming from.)

I grabbed it from here [1], and repacked and uploaded to tooltool myself. Using the jsshell from 56.0 late beta seems to be a good compromise between the need for a recent version and the need for stability.

[1] https://archive.mozilla.org/pub/firefox/candidates/56.0b12-candidates/build1/jsshell-linux-x86_64.zip
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: waiting for lock on working directory of /repo/hg/mozilla/integration/autoland held by process '9415' on host 'hgssh4.dmz.scl3.mozilla.com/effffffc'
remote: got lock after 4 seconds
abort: push failed:
'repository changed while pushing - please try again'
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f90546e2947f
Update jsshell for minifying Fennec JS code; r=nalexander
https://hg.mozilla.org/mozilla-central/rev/f90546e2947f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 57 → mozilla57
You need to log in before you can comment on or make changes to this bug.