1426558 - Make autospider builds not rely on system libnspr

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1426558 - Make autospider builds not rely on system libnspr.

The only reason autospider builds succeed in running tests at the moment
is that there is a libnspr4 library installed at the system level on
Centos that is binary compatible with what the js shell requires.

While on the long run we should just avoid depending on libnspr4 at all,
in the short term, we should make the effort to make those tests use the
libnspr4 present in dist/bin.

For the tests executed from js/src/Makefile.in, it turns out there is
already a level of wrapping that does that, relying on run-mozilla.sh,
which is only installed on gecko builds. Installing it on standalone js
builds solve half the problem.

The other half is addressed by setting LD_LIBRARY_PATH before invoking
the js shell in the various places it's invoked.

Review commit: https://reviewboard.mozilla.org/r/208966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208966/

Comment 2

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

Comment on attachment 8938215 [details]
Bug 1426558 - Make autospider builds not rely on system libnspr.

https://reviewboard.mozilla.org/r/208966/#review214670

::: js/src/devtools/automation/autospider.py:459
(Diff revision 1)
> +    libpath = [os.path.join(OBJDIR, 'dist', 'bin')]
> +    if 'LD_LIBRARY_PATH' in os.environ:
> +        libpath.append(os.environ['LD_LIBRARY_PATH'])
> +    test_env['LD_LIBRARY_PATH'] = ':'.join(libpath)

This looks like it will only handle the jsapi-tests binary, not the invocations of the JS shell.

I think this would be better done way up above, next to where it is setting up MOZ_UPLOAD_DIR. 'env' is an alias for os.environ. Whether it was a good idea to alias it is another question, but as long as it is, it would be better to be consistent.

::: js/src/devtools/rootAnalysis/analyze.py:35
(Diff revision 1)
> +    libpath = [os.path.dirname(config['js'])]
> +    if 'LD_LIBRARY_PATH' in os.environ:
> +        libpath.append(os.environ['LD_LIBRARY_PATH'])
> +    e['LD_LIBRARY_PATH'] = ':'.join(libpath)

To be consistent, I'd rather this were

    bindir = os.path.dirname(config['js'])
    e['LD_LIBRARY_PATH'] = ':'.join(p for p in (e.get('LD_LIBRARY_PATH'), bindir) if p)

Either that, or expand out the PATH setting above similar to how you have it here. (Or make a function, or whatever. Just so long as it's consistent.)

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8938215 [details]
Bug 1426558 - Make autospider builds not rely on system libnspr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208966/diff/1-2/

Comment 4

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

Comment on attachment 8938215 [details]
Bug 1426558 - Make autospider builds not rely on system libnspr.

https://reviewboard.mozilla.org/r/208966/#review214686

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e075bee2a52
Make autospider builds not rely on system libnspr. r=sfink

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8e075bee2a52