Ifdef jsshell packaging with MOZ_PACKAGE_JSSHELL

RESOLVED FIXED in mozilla13

Status

defect
RESOLVED FIXED
8 years ago
Last year

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla13
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

8 years ago
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.
Assignee

Comment 3

7 years ago
Posted 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+
Assignee

Comment 5

7 years ago
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)
Assignee

Comment 6

7 years ago
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)
Assignee

Comment 7

7 years ago
(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.
Assignee

Comment 9

7 years ago
Posted 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+
Assignee

Updated

7 years ago
Attachment #604538 - Flags: checkin+
Assignee

Updated

7 years ago
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/cd70fe030a54
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED

Updated

4 years ago
Blocks: 1214705

Updated

4 years ago
Blocks: 1229777

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.