Closed Bug 1254873 Opened 4 years ago Closed 4 years ago

Make --disable-js-shell the default for non-js-standalone builds

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file)

No description provided.
The implementation is a bit circumvoluted, but we do need to share
options between the top-level and js/src configures, possibly with
different defaults, and to properly pass things down from one to
the other. Until we are further down the road and can actually merge
both configures, this is a necessary evil.

Review commit: https://reviewboard.mozilla.org/r/38889/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38889/
Attachment #8728279 - Flags: review?(cmanchester)
Blocks: 911216
Attachment #8728279 - Flags: review?(cmanchester)
This breaks the build on automation with:
/builds/slave/try-lx-00000000000000000000000/build/src/gcc/bin/objcopy --strip-debug dist/bin/jsapi-tests dist/test-stage/cppunittest/jsapi-tests
02:33:57     INFO -  /builds/slave/try-lx-00000000000000000000000/build/src/gcc/bin/objcopy: 'dist/bin/jsapi-tests': No such file
02:33:57     INFO -  gmake[1]: *** [stage-cppunittests] Error 1

This either needs further adjustments to mozconfigs, or adjustments to build rules. Or both.
Attachment #8728279 - Flags: review?(cmanchester)
(In reply to Mike Hommey [:glandium] from comment #2)
> This breaks the build on automation with:
> /builds/slave/try-lx-00000000000000000000000/build/src/gcc/bin/objcopy
> --strip-debug dist/bin/jsapi-tests dist/test-stage/cppunittest/jsapi-tests
> 02:33:57     INFO - 
> /builds/slave/try-lx-00000000000000000000000/build/src/gcc/bin/objcopy:
> 'dist/bin/jsapi-tests': No such file
> 02:33:57     INFO -  gmake[1]: *** [stage-cppunittests] Error 1
> 
> This either needs further adjustments to mozconfigs, or adjustments to build
> rules. Or both.

This is due to the jsapi-tests change from bug 1251324, which I reverted there, and leave for a followup.
Comment on attachment 8728279 [details]
MozReview Request: Bug 1254873 - Make --disable-js-shell the default for non-js-standalone builds

https://reviewboard.mozilla.org/r/38889/#review35695

::: js/moz.configure:11
(Diff revision 1)
> +def js_shell_default(build_project, help):
> +    return build_project == 'js'

We were asked not to turn off building C++ unit tests by default, because people wanted to know if they were making them not be able to build before pushing to try/landing.

Should we worry about something similar here?

::: toolkit/moz.configure:6
(Diff revision 1)
> +
> +include('../js/moz.configure')

I see this addresses my comment from bug 1251324. Can we move it there?
Attachment #8728279 - Flags: review?(cmanchester)
(In reply to Chris Manchester (:chmanchester) from comment #4)
> Comment on attachment 8728279 [details]
> MozReview Request: Bug 1254873 - Make --disable-js-shell the default for
> non-js-standalone builds
> 
> https://reviewboard.mozilla.org/r/38889/#review35695
> 
> ::: js/moz.configure:11
> (Diff revision 1)
> > +def js_shell_default(build_project, help):
> > +    return build_project == 'js'
> 
> We were asked not to turn off building C++ unit tests by default, because
> people wanted to know if they were making them not be able to build before
> pushing to try/landing.
> 
> Should we worry about something similar here?

The js shell is not used for browser tests, and don't matter on try unless you do JS engine changes, at which point you'd likely be building js standalone, thus having the shell opted-in.

> ::: toolkit/moz.configure:6
> (Diff revision 1)
> > +
> > +include('../js/moz.configure')
> 
> I see this addresses my comment from bug 1251324. Can we move it there?

bug 1251324 limits itself to making what currently is a hardcoded default in js/src as an optional thing. This bug is where we make it an option for browser builds.
(In reply to Mike Hommey [:glandium] from comment #5)
> (In reply to Chris Manchester (:chmanchester) from comment #4)
> > Comment on attachment 8728279 [details]
> > MozReview Request: Bug 1254873 - Make --disable-js-shell the default for
> > non-js-standalone builds
> > 
> > https://reviewboard.mozilla.org/r/38889/#review35695
> > 
> > ::: js/moz.configure:11
> > (Diff revision 1)
> > > +def js_shell_default(build_project, help):
> > > +    return build_project == 'js'
> > 
> > We were asked not to turn off building C++ unit tests by default, because
> > people wanted to know if they were making them not be able to build before
> > pushing to try/landing.
> > 
> > Should we worry about something similar here?
> 
> The js shell is not used for browser tests, and don't matter on try unless
> you do JS engine changes, at which point you'd likely be building js
> standalone, thus having the shell opted-in.

The question is whether someone working on C++ changes spanning js and browser code would do something that would prevent the js shell from building, but not find out until they push, because the js shell is build in automation. If this seems impossible, that's fine.

> 
> > ::: toolkit/moz.configure:6
> > (Diff revision 1)
> > > +
> > > +include('../js/moz.configure')
> > 
> > I see this addresses my comment from bug 1251324. Can we move it there?
> 
> bug 1251324 limits itself to making what currently is a hardcoded default in
> js/src as an optional thing. This bug is where we make it an option for
> browser builds.
(In reply to Chris Manchester (:chmanchester) from comment #6)
> The question is whether someone working on C++ changes spanning js and
> browser code would do something that would prevent the js shell from
> building, but not find out until they push, because the js shell is build in
> automation. If this seems impossible, that's fine.

The js shell is a small C++ shell linked with the js engine library. You can't break the js shell without breaking the js engine library or other things using the js engine library (xpcshell, libxul...) if you don't touch the small shell itself.
Comment on attachment 8728279 [details]
MozReview Request: Bug 1254873 - Make --disable-js-shell the default for non-js-standalone builds

https://reviewboard.mozilla.org/r/38889/#review35725

I think we'll need to put --enable-js-shell in automation mozconfigs for this to land, they build and package the js shell for jittests.
Attachment #8728279 - Flags: review+
(In reply to Chris Manchester (:chmanchester) from comment #8)
> Comment on attachment 8728279 [details]
> MozReview Request: Bug 1254873 - Make --disable-js-shell the default for
> non-js-standalone builds
> 
> https://reviewboard.mozilla.org/r/38889/#review35725
> 
> I think we'll need to put --enable-js-shell in automation mozconfigs for
> this to land, they build and package the js shell for jittests.

They package jit-tests, not the js shell. That's why I was getting the errors from comment 2 and bailed on skipping jit-tests when not enabling the js shell.
Err, so we are shipping the shell in jsshell*.zip, which I don't think we're actively using, but it's better to not break those archives we do ship. The interesting thing is that the build doesn't barf when it creates a jsshell.zip archive that only contains nspr... I'll file a bug about that. And will add --enable-js-shell to build/mozconfig.common.
Filed bug 1255279.
https://hg.mozilla.org/mozilla-central/rev/72e064358650
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
So, this means that js/src/jit-test/jit_test.py and js/src/tests/jstests.py won't run on my standard mozilla-central build anymore. What's the simplest way forward if I want to make changes to the JS engine and test them? (Adding the dev-doc-needed keyword, feel free to change to dev-doc-complete if this has already been documented.)

Does this make the local standard mozilla-central build more different than the builds that run in automation to execute the JS tests?
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(gps)
Keywords: dev-doc-needed
You want to add "ac_add_options --enable-js-shell" to your mozconfig.

I'm not sure the best place to document this :/
Flags: needinfo?(gps)
Flags: needinfo?(mh+mozilla)
Could we have at least announced this or something?  I discovered the lack of a shell today the hard way, when I needed it pretty urgently, and had to ask on IRC to find out what was going on....
(In reply to Boris Zbarsky [:bz] from comment #16)
> Could we have at least announced this or something?  I discovered the lack
> of a shell today the hard way, when I needed it pretty urgently, and had to
> ask on IRC to find out what was going on....

I sent an e-mail to dev-platform.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.