Closed Bug 718391 Opened 9 years ago Closed 9 years ago

Ifdef jsshell packaging with MOZ_PACKAGE_JSSHELL

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla13

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(1 file, 1 obsolete file)

According to glandium, we could use a MOZ_PACKAGE_JSSHELL option in configure to enable/disable jsshell packaging.
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #0)
> According to glandium, we could use a MOZ_PACKAGE_JSSHELL option in
> configure to enable/disable jsshell packaging.

in mozconfig, not configure ;)
This WFM. Just stick it in all the nightly mozconfigs, since they're all in the tree now.
Attached patch Patch v1 (obsolete) — Splinter Review
So, with this patch packaging the js shell is opt-in and is done by our build bots for release, debug and nightly builds. It is not done for special builds (like qt or shark), nor for l10n builds or rpm builds. If that happen to be needed, I guess we can just enabled it.

With this patch, I save 20% of |make package| time (goes from ~41 secs to ~33 secs).
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #604106 - Flags: review?(mh+mozilla)
Comment on attachment 604106 [details] [diff] [review]
Patch v1

Review of attachment 604106 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the packager.mk part, and rs+ for the rest, but I'm not sure if the buildbots are doing make -f client.mk package or make -C objdir package. If the latter, your changes won't work (you'll then need to add an AC_SUBST in configure.in, and an assignment in config/autoconf.mk)
Did you check with the js people if they wanted a js shell built for mobile too (most notably android)?
Attachment #604106 - Flags: review?(mh+mozilla)
Attachment #604106 - Flags: review?(catlee)
Attachment #604106 - Flags: review+
Comment on attachment 604106 [details] [diff] [review]
Patch v1

David, what do you think of the targets where the js shell is packaged? Do you think we are missing some or packaging sometimes when it's not needed?
Attachment #604106 - Flags: review?(dmandelin)
Comment on attachment 604106 [details] [diff] [review]
Patch v1

So according to ted (on IRC), JS people were not involved in js shell packaging stuff so I'm canceling the review request, David. Feel free to review the patch or re-assign the review to yourself if you want to have a look.

Also, I will remove the js shell packaging for mobile. I'm doing a try run to see if the js shell is packaged by simply adding the var to the env.
Attachment #604106 - Flags: review?(dmandelin)
(In reply to Mike Hommey [:glandium] from comment #4)
> r+ for the packager.mk part, and rs+ for the rest, but I'm not sure if the
> buildbots are doing make -f client.mk package or make -C objdir package. If
> the latter, your changes won't work (you'll then need to add an AC_SUBST in
> configure.in, and an assignment in config/autoconf.mk)

It's unfortunately the later :(
Comment on attachment 604106 [details] [diff] [review]
Patch v1

Review of attachment 604106 [details] [diff] [review]:
-----------------------------------------------------------------

does this work as-is without using 'mk_add_options'? it should be testable on try.
Attached patch Patch v2Splinter Review
This is working, tested on try with mobile bulids (no js shell) and linux builds (with js shell): https://tbpl.mozilla.org/?tree=Try&rev=5cf2520be55e (you can grep for "shell...".
Attachment #604106 - Attachment is obsolete: true
Attachment #604106 - Flags: review?(catlee)
Attachment #604538 - Flags: review?(catlee)
Attachment #604538 - Flags: review?(catlee) → review+
Attachment #604538 - Flags: checkin+
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/cd70fe030a54
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 1214705
Blocks: 1229777
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.