Last Comment Bug 618381 - Add an API serial number to jsapi.h
: Add an API serial number to jsapi.h
Status: REOPENED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-10 11:05 PST by Colin Walters
Modified: 2014-07-24 11:07 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add version serial (795 bytes, patch)
2010-12-10 11:05 PST, Colin Walters
no flags Details | Diff | Splinter Review
updated patch (3.75 KB, patch)
2010-12-10 13:51 PST, Colin Walters
no flags Details | Diff | Splinter Review
updated for comments (4.29 KB, patch)
2010-12-10 15:46 PST, Colin Walters
no flags Details | Diff | Splinter Review
actually include new mozilla-js.pc.in (4.72 KB, patch)
2010-12-10 16:56 PST, Colin Walters
no flags Details | Diff | Splinter Review

Description Colin Walters 2010-12-10 11:05:23 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.200 Safari/534.10
Build Identifier: 

This will allow embedders to more easily determine whether they will compile with the current spidermonkey version.

Reproducible: Always
Comment 1 Colin Walters 2010-12-10 11:05:53 PST
Created attachment 496864 [details] [diff] [review]
add version serial
Comment 2 Colin Walters 2010-12-10 11:06:31 PST
The main thing is that SpiderMonkey developers will now need to watch for incompatible jsapi.h changes, and remember to bump this number.  (And code reviewers should check for it)
Comment 3 Paul Biggar 2010-12-10 11:12:28 PST
I think we established in IRC that this is a bad idea, because multiple branches will get confused. We're discussing a solution in IRC now.
Comment 4 Paul Biggar 2010-12-10 11:36:21 PST
OK, we seem to have concluded on taking a manually increasing counter, which is only changed on tracemonkey for API/ABI changes.


So this shouldn't be updated in m-c, just on TM. The responsibility is with the programmer who changes the API to bump the number.


Other potential solutions were timestamp, days since 2000, date stamp; these are hard to do automatically in a #define.

The difficulty with a single manually increasing counter is that we have multiple branches, etc. The solution is to only increase it on TM tip. This means M-C shouldn't be taking patches that increase the number, but I think that's pretty close to current policy anyway.

API changes are currently expensive enough that increasing a number is no harm (we email the js-engine list in those cases). ABI changes can be harder to spot, and I think the solution there is to not commit to getting that right every time.
Comment 5 Paul Biggar 2010-12-10 12:15:15 PST
I think the remaining work here is:

- use this number in all places: release version, tarball version, .so version.
- add a minor number in case we want to release more than once per API version
- add a 'make package' target (I think that's the right name in the unix nomenclature)
- post to the js-engine list (CC maintainers who have posted bugs like bug 506890 in the past) to make sure we haven't missed something
- get review
- add a new bug for hosting the tarball on mozilla.org
- optionally release
Comment 6 Colin Walters 2010-12-10 13:51:58 PST
Created attachment 496913 [details] [diff] [review]
updated patch

This patch is on top of the last one, because I don't know how to do "git rebase" in mercurial yet.  See commit log for changes.
Comment 7 Paul Biggar 2010-12-10 14:14:52 PST
To rebase, use mercurial queues and do `hg qfold`.


I had a different intention for the version number than what you've implemented (apologies for being unclear). I was my intention that we have a single version tuple for all uses. So .so number would be the same as JSAPI_SERIAL_NUMBER and PACKAGE_VERSION.

It's a little unusual in terms of how things are normally packaged, but it means things are really really simple for us, and I'm not sure of a downside except that the numbers wouldn't increase linearly (is that a real problem?)
Comment 8 Mike Hommey [:glandium] 2010-12-10 14:30:50 PST
(In reply to comment #4)
> OK, we seem to have concluded on taking a manually increasing counter, which is
> only changed on tracemonkey for API/ABI changes.
> 
> 
> So this shouldn't be updated in m-c, just on TM. The responsibility is with the
> programmer who changes the API to bump the number.

Technically, you don't need to bump library SO version when changing API. You need to do so when changing ABI (which, most of the time, also involves API changes, but the converse is not true).
Comment 9 Mike Hommey [:glandium] 2010-12-10 14:32:46 PST
While talking about making spidermonkey a better behaved library, bug 554088 really needs to be addressed too.
Comment 10 Paul Biggar 2010-12-10 14:37:29 PST
(In reply to comment #8)
> Technically, you don't need to bump library SO version when changing API. You
> need to do so when changing ABI (which, most of the time, also involves API
> changes, but the converse is not true).

It's expensive to keep track of whether we have broken the ABI, and I don't think we're going to commit to that. If we change the API we have almost certainly broken the ABI, and it is very cheap to know that.

The question is: is that useful to embedders and library folks? I think yes (not as useful as a stable ABI, but again, we won't get there).
Comment 11 Colin Walters 2010-12-10 14:42:43 PST
(In reply to comment #9)
> While talking about making spidermonkey a better behaved library, bug 554088
> really needs to be addressed too.

Good point, yes.  I don't want to block this one on that though.

(In reply to comment #10)
>
> It's expensive to keep track of whether we have broken the ABI, and I don't
> think we're going to commit to that. If we change the API we have almost
> certainly broken the ABI, and it is very cheap to know that.

The point of JSAPI_VERSION_SERIAL is it's only bumped on *incompatible* API, not just "API changes" which could be just plain additions.  Which basically always means ABI has changed, and if not, oh well.

Someone was suggesting there be JSAPI_VERSION_MAJOR and JSAPI_VERSION_MINOR but personally (as an embedder) I don't need that - JSAPI_VERSION_MINOR will be effectively encoded in mozilla-js.pc.
Comment 12 Mike Hommey [:glandium] 2010-12-10 14:43:21 PST
The problem I see with bumping on API changes is that it's going to be dumped too often.

Anyways, I'm not entirely sure most embedders need more than a few stable entry points in the library. ABI compatibility could be kept for these few.
Comment 13 Colin Walters 2010-12-10 14:46:31 PST
(In reply to comment #7)
> To rebase, use mercurial queues and do `hg qfold`.

Trying this now.

> I had a different intention for the version number than what you've implemented
> (apologies for being unclear). I was my intention that we have a single version
> tuple for all uses. So .so number would be the same as JSAPI_SERIAL_NUMBER and
> PACKAGE_VERSION.

Hmm...I need concrete examples here.  You're saying they're all the same number?  Or that one would be derived from the others?
Comment 14 Mike Hommey [:glandium] 2010-12-10 14:46:50 PST
There could also be a need to separate friend api and public api, and keep separate versioning for both.
Comment 15 Paul Biggar 2010-12-10 14:51:55 PST
(In reply to comment #12)
> The problem I see with bumping on API changes is that it's going to be dumped
> too often.
> 
> Anyways, I'm not entirely sure most embedders need more than a few stable entry
> points in the library. ABI compatibility could be kept for these few.

(In reply to comment #14)
> There could also be a need to separate friend api and public api, and keep
> separate versioning for both.

The current stance of the spidermonkey developers (AFAICT) on ABI compatibility is "we're not willing to put in the work to be compatible". The stance on API compatibility is "we'll alert users when we make incompatible changes". The goal of this bug is to work within those constraints (trying to reach beyond them is what derailed bug 506890). The only burden we want to add is something really really cheap, such as "increment some number on an incompatible API change".
Comment 16 Paul Biggar 2010-12-10 14:53:34 PST
(In reply to comment #12)
> The problem I see with bumping on API changes is that it's going to be dumped
> too often.

Can you explain what "too often" means? Is there a disadvantage (other than aesthetics) to increasing the number non-linearly?
Comment 17 Paul Biggar 2010-12-10 14:56:04 PST
(In reply to comment #11)
> Someone was suggesting there be JSAPI_VERSION_MAJOR and JSAPI_VERSION_MINOR but
> personally (as an embedder) I don't need that - JSAPI_VERSION_MINOR will be
> effectively encoded in mozilla-js.pc.

That was me. I don't see a good reason not to have this, and it may provide us some flexibility in the future.

I don't understand how mozilla-js.pc provides this?
Comment 18 Paul Biggar 2010-12-10 14:59:33 PST
(In reply to comment #13)
> (In reply to comment #7)
> > To rebase, use mercurial queues and do `hg qfold`.
> 
> Trying this now.
> 
> > I had a different intention for the version number than what you've implemented
> > (apologies for being unclear). I was my intention that we have a single version
> > tuple for all uses. So .so number would be the same as JSAPI_SERIAL_NUMBER and
> > PACKAGE_VERSION.
> 
> Hmm...I need concrete examples here.  You're saying they're all the same
> number?  Or that one would be derived from the others?

I'm saying all the same number.

So spidermonkey-127.3.tar.bz has JSAPI_MAJOR_NUMBER 127 and JSAPI_MINOR_NUMBER 3 and installs libmozjs.127.3.so.
Comment 19 Mike Hommey [:glandium] 2010-12-10 15:05:16 PST
(In reply to comment #16)
> (In reply to comment #12)
> > The problem I see with bumping on API changes is that it's going to be dumped
> > too often.
> 
> Can you explain what "too often" means? Is there a disadvantage (other than
> aesthetics) to increasing the number non-linearly?

Every single bump means all software depending on the library needs to be rebuilt, even when the ABI is compatible. Bumping for all API changes means bumping for cases where it's unnecessary. For a practical example, see the latest security release: it adds symbols, which technically doesn't need a SO version change. If you bump in that case, it means all software depending on the js library on distros stable branches need to be rebuilt. For a security update, it's quite a burden.
Comment 20 Colin Walters 2010-12-10 15:11:35 PST
(In reply to comment #19)
>
> So spidermonkey-127.3.tar.bz has JSAPI_MAJOR_NUMBER 127 and JSAPI_MINOR_NUMBER
> 3 and installs libmozjs.127.3.so.

So JSAPI_MAJOR_NUMBER is equivalent to the patch's JSAPI_VERSION_SERIAL, right?
Then when is JSAPI_MINOR_NUMBER incremented?  When a new tarball release is made?
Comment 21 Paul Biggar 2010-12-10 15:20:10 PST
(In reply to comment #20)
> (In reply to comment #19)
> >
> > So spidermonkey-127.3.tar.bz has JSAPI_MAJOR_NUMBER 127 and JSAPI_MINOR_NUMBER
> > 3 and installs libmozjs.127.3.so.
> 
> So JSAPI_MAJOR_NUMBER is equivalent to the patch's JSAPI_VERSION_SERIAL, right?
> Then when is JSAPI_MINOR_NUMBER incremented?  When a new tarball release is
> made?

JSAPI_VERSION_SERIAL in jsapi.h would be replaced by JSAPI_MAJOR_NUMBER and JSAPI_MINOR_NUMBER. There would be no PACKAGE_VERSION (I guess we'd grep jsapi to make this).

JSAPI_MINOR_VERSION is incremented when a new tarball release is made, if the JSAPI_MAJOR_NUMBER has not been incremented.
Comment 22 Paul Biggar 2010-12-10 15:27:23 PST
(In reply to comment #19)
> Every single bump means all software depending on the library needs to be
> rebuilt, even when the ABI is compatible. Bumping for all API changes means
> bumping for cases where it's unnecessary. For a practical example, see the
> latest security release: it adds symbols, which technically doesn't need a SO
> version change. If you bump in that case, it means all software depending on
> the js library on distros stable branches need to be rebuilt. For a security
> update, it's quite a burden.

You're right, we should only increase the MAJOR_NUMBER for an incompatible change (I said that somewhere, but maybe not here, sorry).

Are we OK to increase MINOR_NUMBER (as in libmozjs.5.MINOR_NUMBER) whenever we like?
Comment 23 Colin Walters 2010-12-10 15:31:11 PST
(In reply to comment #22)
>
> Are we OK to increase MINOR_NUMBER (as in libmozjs.5.MINOR_NUMBER) whenever we
> like?

I'm not yet touching versioning of libmozjs.so; let's do that in a second patch.
Comment 24 Mike Hommey [:glandium] 2010-12-10 15:40:09 PST
(In reply to comment #23)
> I'm not yet touching versioning of libmozjs.so; let's do that in a second
> patch.

The patch for versioning mostly exists in Debian, except it's not !unix aware.
Comment 25 Mike Hommey [:glandium] 2010-12-10 15:40:48 PST
(In reply to comment #24)
> The patch for versioning mostly exists in Debian, except it's not !unix aware.

It also doesn't integrate with packager.mk.
Comment 26 Paul Biggar 2010-12-10 15:42:53 PST
(In reply to comment #23)
> > Are we OK to increase MINOR_NUMBER (as in libmozjs.5.MINOR_NUMBER) whenever we
> > like?

Sorry, that was a question for Mike. I didn't know if there was a problem with incrementing the minor version of a .so.

 
> I'm not yet touching versioning of libmozjs.so; let's do that in a second
> patch.

I don't think we want this patch without that, it gets us too little.
Comment 27 Colin Walters 2010-12-10 15:46:28 PST
Created attachment 496974 [details] [diff] [review]
updated for comments
Comment 28 Mike Hommey [:glandium] 2010-12-10 15:47:40 PST
(In reply to comment #26)
> (In reply to comment #23)
> > > Are we OK to increase MINOR_NUMBER (as in libmozjs.5.MINOR_NUMBER) whenever we
> > > like?
> 
> Sorry, that was a question for Mike. I didn't know if there was a problem with
> incrementing the minor version of a .so.

There isn't.
Comment 29 Mike Hommey [:glandium] 2010-12-10 15:57:10 PST
Comment on attachment 496974 [details] [diff] [review]
updated for comments

>* Remove the Firefox option --enable-shared-js; we expect
>  people who were formerly using that option to now use
>  separate SpiderMonkey releases.

Please don't.
Comment 30 Paul Biggar 2010-12-10 16:08:35 PST
Can you elaborate please?
Comment 31 Colin Walters 2010-12-10 16:10:53 PST
(In reply to comment #26)
>
> > I'm not yet touching versioning of libmozjs.so; let's do that in a second
> > patch.
> 
> I don't think we want this patch without that, it gets us too little.

I'm not sure; this patch alone would help me a lot both as a Fedora developer and a spidermonkey embedder in GNOME 3.  I am wary of bundling too much into this patch and not getting anything in =/ 

But anyways I'll take a look at .so versioning now.
Comment 32 Colin Walters 2010-12-10 16:56:12 PST
Created attachment 496995 [details] [diff] [review]
actually include new mozilla-js.pc.in

Fix to actually include mozilla-js.pc.in, and add JSAPI_VERSION_MAJOR to the .pc file too as a variable for embedder convenience in autoconf.
Comment 33 Mike Hommey [:glandium] 2010-12-11 00:45:44 PST
(In reply to comment #30)
> Can you elaborate please?

I still want libxul.so depending on an external libmozjs.so, and I don't need an external spidermonkey package. At least for the moment.
Comment 34 Paul Biggar 2010-12-11 07:35:23 PST
(In reply to comment #33)
> (In reply to comment #30)
> > Can you elaborate please?
> 
> I still want libxul.so depending on an external libmozjs.so, and I don't need
> an external spidermonkey package. At least for the moment.

I guess we shouldn't get rid of it then.
Comment 35 Paul Biggar 2010-12-14 15:47:57 PST
(In reply to comment #32)
> Created attachment 496995 [details] [diff] [review]
> actually include new mozilla-js.pc.in
> 
> Fix to actually include mozilla-js.pc.in, and add JSAPI_VERSION_MAJOR to the
> .pc file too as a variable for embedder convenience in autoconf.

I took a look at this and it looks pretty much right. I guess we need to wait on having a versioned binary though, or else there isn't much benefit to it. Are you working on that?
Comment 36 Colin Walters 2011-01-07 15:36:52 PST
(In reply to comment #33)
> (In reply to comment #30)
> > Can you elaborate please?
> 
> I still want libxul.so depending on an external libmozjs.so, and I don't need
> an external spidermonkey package. At least for the moment.

I want exactly the opposite though; have libmozjs private to xulrunner, and split off spidermonkey4 as a separate package.

The reason this needs to be necessary is that there are multiple "embeddings" in SpiderMonkey parlance I (Fedora) needs to care about:

* gnome-shell/gjs
* libproxy-mozjs

(along with a variety of less important stuff like games)

Which I want to be desynchronized from Firefox, because we can't jump and immediately port all of these things when Firefox needs a security update that possibly touches SpiderMonkey.

None of the spidermonkey consumers are security-relevant really; e.g. for gnome-shell extensions are trusted.  XULRunner is a different situation; obviously, Thunderbird may be vulnerable to whatever is in Firefox too, because of HTML email (yes, ewww...).

If you really want to link everything together, then I think basically we need a separate .so and separate .pc file name for SpiderMonkey's split tarball.

How about spidermonkey.so and spidermonkey.pc, which makes it clear it's a standalone project, whereas "libmozjs.so" is more obviously whatever Firefox/XULRunner is using.
Comment 37 Paul Biggar 2011-01-07 16:28:32 PST
(In reply to comment #36)
> (In reply to comment #33)
> > (In reply to comment #30)
> > > Can you elaborate please?
> > 
> > I still want libxul.so depending on an external libmozjs.so, and I don't need
> > an external spidermonkey package. At least for the moment.
>
> 
> I want exactly the opposite though; have libmozjs private to xulrunner, and
> split off spidermonkey4 as a separate package.

When you two say "I", it would help me if you could say what user-group you represent: embedders, linux distributions, etc?

What are the use cases for both approaches? I'd like to avoid adding more ways to do things.
Comment 38 Paul Biggar 2011-01-07 16:33:32 PST
Recent discussions in Mozilla have us moving to a 3 month release cycle for Firefox, after FF4. That affects this bug considerably.

I think this bug was largely predicated on the idea that Firefox is released too infrequently to make it the basis of SpiderMonkey source packages. This created a requirement to have a versioning scheme which was not tied to Firefox or Gecko version numbers.

Is a separate versioning scheme still useful in light of this? If so, do we require all the parts discussed above, or just a subset?
Comment 39 Mike Hommey [:glandium] 2011-01-08 00:59:23 PST
(In reply to comment #37)
> > > I still want libxul.so depending on an external libmozjs.so, and I don't need
> > > an external spidermonkey package. At least for the moment.
> >
> > 
> > I want exactly the opposite though; have libmozjs private to xulrunner, and
> > split off spidermonkey4 as a separate package.
> 
> When you two say "I", it would help me if you could say what user-group you
> represent: embedders, linux distributions, etc?

His "I" is Fedora, mine is Debian. Which means different distros have different goals and needs.
Comment 40 Wolfgang Rosenauer [:wolfiR] 2011-01-08 01:15:56 PST
"I" as openSUSE, would wish to see a standalone spidermonkey improved for better embedding for other apps but at the moment I'm building an external libmozjs.so (which is still private as it's not in ld's path but still usable for embedders to use on their own risk).
So please do changes in spidermonkey only w/o m-c behaviour changes.
Comment 41 Wesley W. Garland 2011-03-24 12:06:23 PDT
One possible solution to this problem has been incorporated into bug 628723 (js185src) and outlined in the README.
Comment 42 Paul Biggar 2011-03-24 12:28:39 PDT
I think this bug is no longer required or wanted, based on lack of responses to comment 38. Please correct me and repoen if I'm wrong.
Comment 43 Colin Walters 2011-04-17 06:41:44 PDT
Now that bug 628723 is done, can we revisit this?  To repeat:

All I want is in jsapi.h:

#define JSAPI_VERSION_SERIAL (42)

Where 42 is incremented any time an incompatible change is made to the API.  This would make my life a good bit easier with very little pain for Spidermonkey hackers.

The serial number is definitely distinct from the package version, because it can be useful for me to keep my embedding in sync with even mercurial tip.
Comment 44 Paul Biggar 2011-04-17 07:06:04 PDT
I haven't been following what has happened in other bugs such that this is relevant again. Can you give a quick summary?

I think I'd also like to hear a response to comment 38?
Comment 45 Wesley W. Garland 2011-04-17 07:29:44 PDT
How do we handle the case where API-breaking changes are pushed out of order from tracemonkey to mozilla-central?  Or an interm change is backed out?
Comment 46 Paul Biggar 2011-04-17 08:18:13 PDT
(In reply to comment #45)
> How do we handle the case where API-breaking changes are pushed out of order
> from tracemonkey to mozilla-central?

I'm pretty sure this can't happen, can it?


>  Or an interm change is backed out?

If the only change is being backed out, then it can go back to the previous number (the automatic merge will handle this). If the number is changed in both parents, then it's no harm to go to the larger of the two numbers. The automatic merge will fail in this case, so we'll need to resolve this manually anyway.
Comment 47 Colin Walters 2011-04-18 10:37:45 PDT
(In reply to comment #44)
> I haven't been following what has happened in other bugs such that this is
> relevant again. Can you give a quick summary?

This bug got scope-creeped significantly, then none of it happened here, and instead some if it happened from bug 628723.

> I think I'd also like to hear a response to comment 38?

As I said in comment 43, a version serial is useful for me to try to support pre-releases of spidermonkey (there are people that actually run hg/git daily OS images).
Comment 48 Paul Biggar 2011-04-18 10:51:22 PDT
(In reply to comment #47)
> (In reply to comment #44)
> > I haven't been following what has happened in other bugs such that this is
> > relevant again. Can you give a quick summary?
> 
> This bug got scope-creeped significantly, then none of it happened here, and
> instead some if it happened from bug 628723.

Ah, that was me, sorry :(


> As I said in comment 43, a version serial is useful for me to try to support
> pre-releases of spidermonkey (there are people that actually run hg/git daily
> OS images).

I can see how/when this number is going to be updated, but I don't fully understand how it's going to be used. Is it going to be:

#if JSAPI_NUM > 42
  something
#else
  something else
#endif

If so, is there a reason that using a configure script would not be more appropriate?

(I understood the use when this number was going into a .so name, or being used as a release number, but I'm not sure of the use now).
Comment 49 Colin Walters 2011-04-18 11:06:30 PDT
(In reply to comment #48)
> 
> I can see how/when this number is going to be updated, but I don't fully
> understand how it's going to be used. Is it going to be:
> 
> #if JSAPI_NUM > 42
>   something
> #else
>   something else
> #endif

Yeah, I might do that in certain cases, but:

> If so, is there a reason that using a configure script would not be more
> appropriate?

I do use a configure script - the problem is that certain types of changes are a *pain in the butt* to detect statically.  See for example:

http://git.gnome.org/browse/gjs/tree/configure.ac#n211

My recent change to add an argument to JS_BufferIsCompileUnit falls into the same area.
Comment 50 Paul Biggar 2011-04-18 11:18:56 PDT
I'll take your word for it :)

The next steps are:

- refresh the patch, including a comment describing exactly when it needs to be incremented
- pre-announce the plan on js-internals so no-one gets a shock
- get review
- get super-review from brendan
- announce it's availability to js-engine

Note You need to log in before you can comment on or make changes to this bug.