1973993 - Build, test failures using standalone SpiderMonkey installation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Xi Ruoyao]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/60.5 Safari/605.1.15

Steps to reproduce:

Install SpiderMonkey-140 and attempt to port gjs to it (https://gitlab.gnome.org/xry111/gjs/-/commits/xry111/mozjs-140).

Actual results:

gjs fails to build because BaseProfilingCatagory.h contains

#include "ProfilingCategoryList.h"

but ProfilingCategoryList.h is in js/ subdirectory, not the same directory as BaseProfilingCatagory.h.

Expected results:

BaseProfilingCatagory.h should contain #include "js/ProfilingCategoryList.h" instead.

Comment 1

[Steve Fink [:sfink] [:s:]]

Hm, this is strange.

The source location of BaseProfilingCategory.h is in mozglue/baseprofiler/public/. In my mozjs-140, the build puts a symlink from $objdir/dist/include/BaseProfilingCategory.h and NOT dist/include/js/BaseProfilingCategory.h.

ProfilingCategoryList.h is a generated file that gets written into both $objdir/dist/include/ and $objdir/dist/include/js/.

But that doesn't match the reported problem. For me, they are both under include/. Only ProfilingCategoryList.h is under js/. But BaseProflingCategory.h is getting found and then not finding ProfilingCategoryList.h, which would not happen in what I'm seeing; it's in both directories!

Oh wait, this is a build of mozjs. I wonder if an install would now do something different. Ah! Yes, now I have:

include/mozjs-140/BaseProfilingCategory.h
include/mozjs-140/js/ProfilingCategoryList.h

So that's the problem.

Your proposed solution of doing #include "js/ProfilingCategoryList.h" doesn't work for Firefox. BaseProflingCategory.h is not JS-specific so it should not include something JS-specific, even though in practice it's now the same file: some of the confusion here comes from https://bugzilla.mozilla.org/show_bug.cgi?id=1645464 which merged the category lists and just exposed all of the Firefox categories to js. js/ things can include stuff from the root include dir, but not the other way around.

Ok... I think what might work is re-cloning the category list into both locations during make install.

This path isn't tested in CI, which is part of the problem here. Nothing does the make install. For that, we'd need a sample embedding. Or perhaps we could rely on github actions in https://github.com/mozilla-spidermonkey/spidermonkey-embedding-examples/

Comment 2

[Steve Fink [:sfink] [:s:]]

I'm going to rename this bug to broaden it to include the issue reported on Conduit where mach is failing (for eg mach jit-test) because the package is now missing some required python files.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1973993 - Add in missing files for running mach r=#spidermonkey-reviewers

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1973993 - Install ProfilingCategoryList.h into both the js/ directory and the toplevel include directory r=#spidermonkey-reviewers

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1973993 - Switch autospider.py to using mach to run jit-test and jstests r=#spidermonkey-reviewers

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1973993 - Switch gdb test to using extra-args r=#spidermonkey-reviewers

Comment 7

[Philip Chimento [:ptomato]]

Any chance the rest of these patches could get reviewed? It's currently impossible to compile several of the embedder examples: https://github.com/mozilla-spidermonkey/spidermonkey-embedding-examples/pull/88

Comment 8

[Steve Fink [:sfink] [:s:]]

It's not a problem with reviewing; they've all been accepted. The problem is that I was getting failures in the arm64 task. Which I just figured out: make check-jstests passes additional flags (like --timeout=300) that mach jstests does not, so it was failing due to timeouts. I have a try push running on it right now.

Comment 9

[Philip Chimento [:ptomato]]

Oh thanks! I got confused, it looked like only the first one was accepted. But now I'm looking at it again and I can't figure out why I thought that.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b261f7e1b688
https://hg.mozilla.org/integration/autoland/rev/88943b2ea96c
Add in missing files for running mach r=spidermonkey-reviewers,jandem
https://github.com/mozilla-firefox/firefox/commit/236477038fa9
https://hg.mozilla.org/integration/autoland/rev/fd42d588fd7c
Install ProfilingCategoryList.h into both the js/ directory and the toplevel include directory r=spidermonkey-reviewers,jandem
https://github.com/mozilla-firefox/firefox/commit/bd39f98dcfd1
https://hg.mozilla.org/integration/autoland/rev/eb840014582d
Switch autospider.py to using mach to run jit-test and jstests r=spidermonkey-reviewers,arai
https://github.com/mozilla-firefox/firefox/commit/2337cebc88cb
https://hg.mozilla.org/integration/autoland/rev/f7f58f0966ec
Switch gdb test to using extra-args r=spidermonkey-reviewers,jandem

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/88943b2ea96c
https://hg.mozilla.org/mozilla-central/rev/fd42d588fd7c
https://hg.mozilla.org/mozilla-central/rev/eb840014582d
https://hg.mozilla.org/mozilla-central/rev/f7f58f0966ec

Comment 12

[Xi Ruoyao]

Can we backport the fix to 140 ESR? The external projects using a standalone SpiderMonkey library are most likely using a ESR.

Comment 13

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1973993 - Add in missing files for running mach r=#spidermonkey-reviewers

Original Revision: https://phabricator.services.mozilla.com/D256359

Comment 14

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1973993 - Add in missing files for running mach r=#spidermonkey-reviewers

Original Revision: https://phabricator.services.mozilla.com/D256359

Comment 15

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: SpiderMonkey embedders will have to patch.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: not needed
• Risk associated with taking this patch: low
• Explanation of risk level: This mostly affects the standalone spidermonkey archive, and testing. If it breaks something, it will be obvious.
• String changes made/needed: none
• Is Android affected?: no

Comment 16

[Steve Fink [:sfink] [:s:]]

I'm having trouble with this fancy new uplift thing. I would want all of these patches to be uplifted, not the 1 patch.

Comment 17

[Dianna Smith [:diannaS]]

Attachment: Bug 1973993 - Add in missing files for running mach r=#spidermonkey-reviewers

Original Revision: https://phabricator.services.mozilla.com/D256359

Comment 18

[Dianna Smith [:diannaS]]

Attachment: Bug 1973993 - Install ProfilingCategoryList.h into both the js/ directory and the toplevel include directory r=#spidermonkey-reviewers

Original Revision: https://phabricator.services.mozilla.com/D256360

Comment 19

[Dianna Smith [:diannaS]]

Attachment: Bug 1973993 - Switch autospider.py to using mach to run jit-test and jstests r=#spidermonkey-reviewers

Original Revision: https://phabricator.services.mozilla.com/D256361

Comment 20

[Dianna Smith [:diannaS]]

Attachment: Bug 1973993 - Switch gdb test to using extra-args r=#spidermonkey-reviewers

Original Revision: https://phabricator.services.mozilla.com/D256362

Comment 21

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: SpiderMonkey embedders will have to patch.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: not needed
• Risk associated with taking this patch: low
• Explanation of risk level: This mostly affects the standalone spidermonkey archive, and testing. If it breaks something, it will be obvious.
• String changes made/needed: none
• Is Android affected?: no

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/796480f28a7d
https://hg.mozilla.org/releases/mozilla-esr140/rev/a201f741c8f1
https://hg.mozilla.org/releases/mozilla-esr140/rev/fae670acae9a
https://hg.mozilla.org/releases/mozilla-esr140/rev/ce2c8d6d9daf