Ifdef jsshell packaging with MOZ_PACKAGE_JSSHELL

RESOLVED FIXED in mozilla13

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 1 bug)

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 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

5 years ago
Created attachment 604106 [details] [diff] [review]
Patch v1

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

5 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

5 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

5 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

5 years ago
Created attachment 604538 [details] [diff] [review]
Patch v2

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)

Updated

5 years ago
Attachment #604538 - Flags: review?(catlee) → review+
(Assignee)

Updated

5 years ago
Attachment #604538 - Flags: checkin+
(Assignee)

Updated

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

Updated

a year ago
Blocks: 1214705
Blocks: 1229777
You need to log in before you can comment on or make changes to this bug.