Closed Bug 1339931 Opened 7 years ago Closed 7 years ago

Standalone SpiderMonkey ESRs should be parallel installable

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- fixed
firefox56 --- fixed

People

(Reporter: ptomato, Assigned: ptomato)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36

Steps to reproduce:

Packaging standalone SpiderMonkey for Linux distributions


Actual results:

Since ESR 38, the pkg-config file in SpiderMonkey has been named "js.pc" instead of "mozjs-38.pc" like it used to be named in 31, and the same for "js" (the shell executable) and "js-config". This prevents different ESRs from being installed on a system in parallel because they overwrite each others' files.


Expected results:

Several Linux distributions are already patching parallel installation back in independently. I would suggest backporting this in ESR 52, as the standalone SpiderMonkey pretty much requires these patches to be usable.

The pkg-config file should be named "mozjs-52.pc" or "js-52.pc" (I prefer the former because it's more specific and similar to the name in 31 and earlier), the shell binary should be named "js52" or similar, and the config script should be named "js52-config" or similar.

The attached patches are what I've been using in GNOME's jhbuild, and Fedora and Ubuntu have similar patches.
Attached patch mozjs38-pkg-config-version.patch (obsolete) — Splinter Review
Attached patch mozjs38-shell-version.patch (obsolete) — Splinter Review
I prefer the "mozjs-38.pc" naming, but wasn't the one to make this change. I would take these patches.

sfink, do you recall why it was changed?
Flags: needinfo?(sphink)
No clue. I skimmed through all of bug 812265, but my eyes glazed over after a while and I didn't notice anything specific to that decision. I'm going to needinfo glandium as another interested party, but I'm fine either way.
Flags: needinfo?(sphink) → needinfo?(mh+mozilla)
I'm not sure about the .pc filename tho, changing js shell executable's name would surely break workflow.
Would it be acceptable to call the shell executable js52 and then make a symlink called js only if not building standalone?
(In reply to philip.chimento from comment #7)
> Would it be acceptable to call the shell executable js52 and then make a
> symlink called js only if not building standalone?

bug 812265 comment #94 would answer that

> symlinks are not supported on windows. So I guess what could work is to have
> the final s$MOZJS_MAJOR_VERSION name set at make install time.
See https://bugzilla.mozilla.org/show_bug.cgi?id=812265#c92 for /some/ background.

Also note that the js executable hasn't really ever been meant to be distributed (except if things have changed in the past years, it's not secure and not meant to be).
Flags: needinfo?(mh+mozilla)
Having read the background there, and the request for bikeshedding, this would be my preferred bikeshed:

- Rename the .pc file and js-config to include the version
- Name the js shell to include the version by default
- Include a configure option --disable-standalone-build that would name the js shell "js"

The only one of the three that I care about personally is the .pc file, but be advised that downstream distro packagers will probably rename the js shell anyway because they don't want different SpiderMonkey ESRs all installing a "js" binary.

Alternatively, if you would rather not be distributing the js shell at all, how about skipping building it, and adding a configure option --enable-js-shell?
(In reply to philip.chimento from comment #10)
> Alternatively, if you would rather not be distributing the js shell at all,
> how about skipping building it, and adding a configure option
> --enable-js-shell?

There actually is a --enable/disable-js-shell option already. Its default is for the shell to be built when building JS standalone, and not to build it when building Firefox.

All in all, the problem is that there are two sets of people that would like opposite things from the default build of JS standalone: JS developers, who would want a js binary and don't really care about the .pc file, and linux distros, who would want a versioned js binary.

I guess the middle ground would be to have `make install` either not install the js binary, or install it with a versioned name, while keeping the binary unversioned in the build tree.
(In reply to Mike Hommey [:glandium] from comment #11)
> I guess the middle ground would be to have `make install` either not install
> the js binary, or install it with a versioned name, while keeping the binary
> unversioned in the build tree.

+1. IMHO the primary consumer of the JS tree build is the JS team (and related automation), so catering to their needs in the default output is reasonable and proper. But 'make install' is a great place to detect that we're in a distro or distro-like situation, and so as much of the translation as possible should happen there. The filename of the binary fits that model well.

The .pc file and js-config can be generated anytime it's most convenient, with whatever name works best for distros, since they won't get in the way regardless.
Comment on attachment 8837770 [details] [diff] [review]
mozjs38-pkg-config-version.patch

Review of attachment 8837770 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/Makefile.in
@@ +214,4 @@
>  $(JS_CONFIG_NAME): js-config
>     cp $^ $@
>  
> +$(JS_LIBRARY_NAME).pc: js.pc

This is going to not only add in the version, it'll also do js -> mozjs, won't it?
(In reply to Steve Fink [:sfink] [:s:] from comment #13)
> > +$(JS_LIBRARY_NAME).pc: js.pc
> 
> This is going to not only add in the version, it'll also do js -> mozjs,
> won't it?

Indeed, I suggested that for a few reasons: it's more specific, it's what Ubuntu and Fedora were already doing, and it's a continuation of the mozjs24.pc that shipped with ESR24.
Can we get this in place before the 52.x release?  I don't know why the slotting was removed but it's a definite problem.  These files -must- be concurrently installable for the different major versions because i.e. 45 isn't a replacement for 38, and in order to ensure consistency for any project that wants to use them the convention really SHOULD be defined by upstream, not packagers.

I would go so far as to say that 38.x and 45.x releases should be fixed with this as well, despite their age.

As to the patches themselves -- I would recommend combining the two patches into one, first off.  Secondly, we need to confirm that we don't need to modify any of the new moz.build stuff to ensure we get our end result -- js/src/moz.build is currently using js.pc and js-config literally rather than using variable names, in the 45.0.2 release tarball.
(In reply to Mike Hommey [:glandium] from comment #9)
> Also note that the js executable hasn't really ever been meant to be
> distributed (except if things have changed in the past years, it's not
> secure and not meant to be).

Do you want to file a bug against Debian's libmozjs24-bin then?
(In reply to Ian Stakenvicius from comment #15)
> I would go so far as to say that 38.x and 45.x releases should be fixed with
> this as well, despite their age.

+1, though I understand that this is pretty much against SpiderMonkey's release policy?

> we need to confirm that we don't need to
> modify any of the new moz.build stuff to ensure we get our end result --
> js/src/moz.build is currently using js.pc and js-config literally rather
> than using variable names, in the 45.0.2 release tarball.

I can certainly combine the two patches into one, but I haven't tried this on 45.x yet, so far I've dealt only with the old build system. Anyone can answer this question, or give me pointers on where to start looking?
Here's an updated patch, with what works for me building ESR52.

Asking for feedback but not review yet, because I'm not sure how to get the desired functionality in moz.build, where the `js` binary is renamed only on `make install`. Any suggestions?
Attachment #8837770 - Attachment is obsolete: true
Attachment #8837771 - Attachment is obsolete: true
Attachment #8884707 - Flags: feedback?(sfink)
Blocks: 1379541
Blocks: sm-embedding
Attachment #8884707 - Flags: feedback?(sfink)
Attachment #8889088 - Flags: review?(sphink)
OK, here's a version that only touches `make install`. It also skips installing the js shell binary altogether, since I understand from the comments here that it's not meant for distribution.
Blocks: 1379536
Comment on attachment 8889088 [details] [diff] [review]
Add major version to make parallel installable, and don't install js shell

Review of attachment 8889088 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/shell/Makefile.in
@@ -10,5 @@
>  
>  include $(topsrcdir)/config/rules.mk
> -
> -install:: $(PROGRAM)
> -	$(SYSINSTALL) $^ $(DESTDIR)$(bindir)

Hm. I know we claim that the js binary isn't intended for distribution. But it also seems like make install is only for embedders, embedders are developing stuff based on spidermonkey, and the shell is a very valuable tool during that development. I would be inclined to leave it in, and let packagers decide whether to package it up or not. (Maybe yes for a devel package, and no for a linkable package?) But maybe it's easier to just package up everything that make install produces?

You are an embedder. I am not. You are in a better position to decide what is more convenient than I am. I will support whichever decision you make, so r=me either way. (And it's not like it's hard to revert if someone else comes along with a different usage scenario.)
Attachment #8889088 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #21)
> Comment on attachment 8889088 [details] [diff] [review]
> Add major version to make parallel installable, and don't install js shell
> 
> Review of attachment 8889088 [details] [diff] [review]:
> -----------------------------------------------------------------
> Hm. I know we claim that the js binary isn't intended for distribution. But
> it also seems like make install is only for embedders, embedders are
> developing stuff based on spidermonkey, and the shell is a very valuable
> tool during that development. I would be inclined to leave it in, and let
> packagers decide whether to package it up or not. (Maybe yes for a devel
> package, and no for a linkable package?) But maybe it's easier to just
> package up everything that make install produces?
> 
> You are an embedder. I am not. You are in a better position to decide what
> is more convenient than I am. I will support whichever decision you make, so
> r=me either way. (And it's not like it's hard to revert if someone else
> comes along with a different usage scenario.)

That works for me too. I interpreted the earlier comments as "you should not be distributing this at all." I agree that it's better for the packagers to decide; personally I would install it in a devel package. For RPM and Debian packages, at least, it's quite easy to say "this file goes in the devel package." So, just to confirm, I'll do a rename as part of the install rule, just as with the other install rules, and not change anything in moz.build.

I'll attach another patch (probably tomorrow), by r=me do you mean I can just set r+ when uploading it?
(In reply to Philip Chimento [:ptomato] from comment #22)
> I'll attach another patch (probably tomorrow), by r=me do you mean I can
> just set r+ when uploading it?

Yes, exactly. Thank you!
Flags: needinfo?(sstangl)
When backporting to esr52, the changes will need to be applied to
js/src/Makefile.in rather than js/src/build/Makefile.in.
Attachment #8889088 - Attachment is obsolete: true
Attachment #8884707 - Attachment is obsolete: true
Attachment #8891623 - Flags: review+
Keywords: checkin-needed
Assignee: nobody → philip.chimento
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83612510f824
Add major version to make parallel installable. r=sfink
Keywords: checkin-needed
Comment on attachment 8891623 [details] [diff] [review]
Add major version to make parallel installable

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is needed for packaging of standalone SpiderMonkey by embedders (e.g. GNOME, downstream Linux distributions.) In practice, every embedder is currently patching this in their own way.
User impact if declined: None to Firefox users, but embedders will have to continue patching.
Fix Landed on Version: 57
Risk to taking this patch (and alternatives if risky): None, it should only affect "make install" which to my knowledge is not used at all by Firefox' build process.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8891623 - Flags: approval-mozilla-esr52?
https://hg.mozilla.org/mozilla-central/rev/83612510f824
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8891623 [details] [diff] [review]
Add major version to make parallel installable

This is needed for SpiderMonkey. Let's uplift it to ESR52.3.
Attachment #8891623 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.