Last Comment Bug 718391 - Ifdef jsshell packaging with MOZ_PACKAGE_JSSHELL
: Ifdef jsshell packaging with MOZ_PACKAGE_JSSHELL
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: 1214705 707569 1229777
  Show dependency treegraph
 
Reported: 2012-01-16 03:45 PST by Mounir Lamouri (:mounir)
Modified: 2015-12-02 07:34 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (8.69 KB, patch)
2012-03-08 10:03 PST, Mounir Lamouri (:mounir)
mh+mozilla: review+
Details | Diff | Splinter Review
Patch v2 (10.26 KB, patch)
2012-03-09 14:39 PST, Mounir Lamouri (:mounir)
catlee: review+
mounir: checkin+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-01-16 03:45:15 PST
According to glandium, we could use a MOZ_PACKAGE_JSSHELL option in configure to enable/disable jsshell packaging.
Comment 1 Mike Hommey [:glandium] 2012-01-16 04:52:14 PST
(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 ;)
Comment 2 Ted Mielczarek [:ted.mielczarek] 2012-01-17 07:40:16 PST
This WFM. Just stick it in all the nightly mozconfigs, since they're all in the tree now.
Comment 3 Mounir Lamouri (:mounir) 2012-03-08 10:03:27 PST
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).
Comment 4 Mike Hommey [:glandium] 2012-03-08 10:10:31 PST
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)?
Comment 5 Mounir Lamouri (:mounir) 2012-03-08 10:16:51 PST
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?
Comment 6 Mounir Lamouri (:mounir) 2012-03-08 10:32:14 PST
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.
Comment 7 Mounir Lamouri (:mounir) 2012-03-09 06:46:27 PST
(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 8 Chris AtLee [:catlee] 2012-03-09 10:00:25 PST
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.
Comment 9 Mounir Lamouri (:mounir) 2012-03-09 14:39:05 PST
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...".
Comment 10 Matt Brubeck (:mbrubeck) 2012-03-12 13:42:32 PDT
https://hg.mozilla.org/mozilla-central/rev/cd70fe030a54

Note You need to log in before you can comment on or make changes to this bug.