718391 - Ifdef jsshell packaging with MOZ_PACKAGE_JSSHELL

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mounir Lamouri (:mounir)]

According to glandium, we could use a MOZ_PACKAGE_JSSHELL option in configure to enable/disable jsshell packaging.

Comment 1

[Mike Hommey [:glandium]]

(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

[(not currently active) Ted Mielczarek]

This WFM. Just stick it in all the nightly mozconfigs, since they're all in the tree now.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: 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]]

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)]

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)]

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)]

(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]]

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)]

Attachment: 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)]

https://hg.mozilla.org/mozilla-central/rev/cd70fe030a54