Last Comment Bug 812265 - js/src needs versioning added to build system to support spidermonkey releases
: js/src needs versioning added to build system to support spidermonkey releases
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Ian Stakenvicius
:
:
Mentors:
Depends on: 835551 838915
Blocks: sm.embedding
  Show dependency treegraph
 
Reported: 2012-11-15 11:26 PST by Ian Stakenvicius
Modified: 2013-04-26 18:47 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
js-version2.patch (5.58 KB, patch)
2012-11-15 11:26 PST, Ian Stakenvicius
mh+mozilla: review-
dmandelin: feedback+
Details | Diff | Splinter Review
Update version related definitions (2.21 KB, patch)
2012-11-16 05:49 PST, Ian Stakenvicius
dmandelin: review-
Details | Diff | Splinter Review
immediate patch for 17ESR (minimal change, JS_VERSION bump) (1.90 KB, patch)
2012-11-20 07:35 PST, Ian Stakenvicius
dmandelin: review+
Details | Diff | Splinter Review
trunk patch to remove JS_VERSION defines (7.11 KB, patch)
2012-11-20 09:46 PST, Ian Stakenvicius
no flags Details | Diff | Splinter Review
fixed patch for trunk to remove JS_VERSION (8.74 KB, patch)
2012-11-20 10:06 PST, Ian Stakenvicius
dmandelin: review+
Details | Diff | Splinter Review
setup versioning and pkg-config support in build system (6.65 KB, patch)
2012-11-30 13:43 PST, Ian Stakenvicius
no flags Details | Diff | Splinter Review
correct shared library versioning (3.80 KB, patch)
2013-02-01 17:15 PST, darkxst
no flags Details | Diff | Splinter Review
correct shared library versioning (for non-mozilla builds only) (3.88 KB, patch)
2013-02-02 17:14 PST, darkxst
no flags Details | Diff | Splinter Review
correct library versioning with ABI versions (for non-mozilla builds only) (3.93 KB, patch)
2013-02-02 21:43 PST, darkxst
no flags Details | Diff | Splinter Review
correct library versioning with ABI versions (for non-mozilla builds only) (4.01 KB, patch)
2013-02-07 15:31 PST, darkxst
no flags Details | Diff | Splinter Review
correct library versioning with ABI versions (fix hardcoded static library names) (5.16 KB, patch)
2013-02-12 18:50 PST, darkxst
mh+mozilla: feedback-
Details | Diff | Splinter Review
a couple more minor fixes (1.58 KB, patch)
2013-02-12 22:08 PST, darkxst
mh+mozilla: feedback-
Details | Diff | Splinter Review
use REAL_LIBRARY for the install target (895 bytes, patch)
2013-02-13 14:58 PST, darkxst
no flags Details | Diff | Splinter Review
fix MOZ_JS_LIBS to use correct library name (2.37 KB, patch)
2013-02-13 15:00 PST, darkxst
mh+mozilla: review+
Details | Diff | Splinter Review
install versioned static library (804 bytes, patch)
2013-02-14 13:49 PST, darkxst
no flags Details | Diff | Splinter Review
setup versioning and pkg-config support in build system v3 (8.21 KB, patch)
2013-02-18 15:10 PST, darkxst
mh+mozilla: review-
Details | Diff | Splinter Review
fix MOZ_JS_LIBS to use correct library name (renamed variable) (3.25 KB, patch)
2013-02-19 14:47 PST, darkxst
mh+mozilla: review+
Details | Diff | Splinter Review
setup versioning and pkg-config support in build system v3a (7.97 KB, patch)
2013-02-19 15:28 PST, darkxst
mh+mozilla: review+
Details | Diff | Splinter Review
add versioning to js-config (2.75 KB, patch)
2013-02-19 20:48 PST, darkxst
no flags Details | Diff | Splinter Review
setup versioning and pkg-config support in build system v3b (7.96 KB, patch)
2013-02-25 13:21 PST, darkxst
no flags Details | Diff | Splinter Review
add versioning to js and js-config (2.79 KB, patch)
2013-02-25 13:25 PST, darkxst
mh+mozilla: review-
Details | Diff | Splinter Review
add versioning to js and js-config (2.79 KB, patch)
2013-02-26 01:48 PST, darkxst
mh+mozilla: review-
Details | Diff | Splinter Review
add versioning to js and js-config (3.57 KB, patch)
2013-02-26 02:45 PST, darkxst
mh+mozilla: review+
Details | Diff | Splinter Review
add versioning to js and js-config (3.59 KB, patch)
2013-02-26 03:05 PST, darkxst
no flags Details | Diff | Splinter Review
Full versioning patch, for debugging. (23.18 KB, patch)
2013-04-23 17:38 PDT, Sean Stangl [:sstangl]
no flags Details | Diff | Splinter Review
[Part 1/4] Remove JS_VERSION. (9.57 KB, patch)
2013-04-24 15:29 PDT, Sean Stangl [:sstangl]
sstangl: review+
Details | Diff | Splinter Review
[Part 2/4] Setup versioning and pkg-config support. (10.07 KB, patch)
2013-04-24 15:31 PDT, Sean Stangl [:sstangl]
mh+mozilla: review+
Details | Diff | Splinter Review
[Part 3/4] Use version number in shell and js-config filenames. (4.45 KB, patch)
2013-04-24 15:33 PDT, Sean Stangl [:sstangl]
mh+mozilla: review+
Details | Diff | Splinter Review
[Part 4/4] Fix MOZ_JS_LIBS (3.19 KB, patch)
2013-04-24 15:34 PDT, Sean Stangl [:sstangl]
mh+mozilla: review+
Details | Diff | Splinter Review
Follow-up: Temporarily comment out standalone JS executable renaming. (947 bytes, patch)
2013-04-25 18:25 PDT, Sean Stangl [:sstangl]
luke: review+
Details | Diff | Splinter Review

Description Ian Stakenvicius 2012-11-15 11:26:05 PST
Created attachment 682092 [details] [diff] [review]
js-version2.patch

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

Steps to reproduce:

Currently the build system for spidermonkey (m-c/js/src) doesn't have any versioning info available in it, so if a spidermonkey release is made it has to be patched separately in order to define the release version.

The following patch adjusts configure to set version vars from milestone.txt (same as all other sections of the codebase), sets the LIBRARY_NAME to contain major.minor version, installs headers into a version-specific subdir, adjusts js-config generation appropriately for all changes, and adds generation of a pkg-config .pc file (currently installed only on linux, but please add for all arches that are appropriate).

The NSPR minimum version is also set in a variable rather than directly in the AM check, so that the same check can be used within the pkg-config.pc file that is generated.

This patch dovetails nicely with the linux symbol versioning patch in bug 809430 , and also addresses bug 481878.  It may also be a precondition for bug 520849.

Marking this for trunk, but it will need to be applied for the 17 branch if there is going to be a spidermonkey-17 release, because no release is possible with the code as it stands.
Comment 1 Ian Stakenvicius 2012-11-15 11:54:56 PST
bug 479475 might be easier with this, too, possibly
Comment 2 Ian Stakenvicius 2012-11-16 05:49:32 PST
Created attachment 682442 [details] [diff] [review]
Update version related definitions

The old JS_VERSION stuff needs to be deprecated in order for consumers to properly use the new versioning scheme.  This patch adjust js_version.h for that.  I took a guess that the legacy defines and such should be kept.  

If instead it would be better to completely gut this file and only provide feature-based #defines according to what the current snapshot of a codebase provides, then we should do that, assuming the VCS can appropriately handle the history/versioning for the various tagged releases.

Note that I have no idea what JS_HAS_* should be defined for the current codebase, so that part needs to be updated -- I just copied what was set for JS_VERSION=185
Comment 3 David Mandelin [:dmandelin] 2012-11-16 16:43:36 PST
Comment on attachment 682092 [details] [diff] [review]
js-version2.patch

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

Thanks! Yeah, it'll be better to get version numbers to just match Gecko.
Comment 4 David Mandelin [:dmandelin] 2012-11-16 16:46:59 PST
Comment on attachment 682442 [details] [diff] [review]
Update version related definitions

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

We can't take this patch as is, because at least js.cpp needs a small update to account for not having JS_VERSION: http://mxr.mozilla.org/mozilla-central/ident?i=JS_VERSION&tree=mozilla-central&filter=

Otherwise, I think it's good. And in fact, I think gutting the file and just using the 185 HAS_* defs is even better, too.
Comment 5 Ian Stakenvicius 2012-11-16 18:03:47 PST
(In reply to David Mandelin [:dmandelin] from comment #4)
> Comment on attachment 682442 [details] [diff] [review]
> Update version related definitions
> 
> Review of attachment 682442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We can't take this patch as is, because at least js.cpp needs a small update
> to account for not having JS_VERSION:
> http://mxr.mozilla.org/mozilla-central/ident?i=JS_VERSION&tree=mozilla-
> central&filter=
> 
> Otherwise, I think it's good. And in fact, I think gutting the file and just
> using the 185 HAS_* defs is even better, too.

Yes, I saw that line, for some reason I thought that if JS_VERSION is unset then the #if would be false so it wouldn't matter...  If the patch is approved in principle i'd recommend adding:

--- a/js/src/shell/js.cpp 
+++ b/js/src/shell/js.cpp 
@@ -2154,7 +2154,9 @@
 {
     char version[20] = "\n";
-#if JS_VERSION < 150
+#ifdef JS_VERSION
+# if JS_VERSION < 150
     sprintf(version, " for version %d\n", JS_VERSION);
+# endif
 #endif
     fprintf(gOutFile, "built on %s at %s%s", __DATE__, __TIME__, version);
     *vp = JSVAL_VOID;

...to this patch.

To the idea of stripping JS_VERSION; if we remove that from jsversion.h we may also want to strip out all reference to it in the rest of the code and build system as well?  I haven't checked through the rest of m-c to see if JS_VERSION is ever used, yet, although i'm not sure if or why it would be..  Anyways pls let me know how I should proceed.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-11-19 14:52:53 PST
Going forward I think it's great getting rid of JS_VERSION entirely, and having the MOZJS stuff as the replacement.  In the short run, especially if we want to spin up a release based on ESR17 (and I think pretty much everyone wants that), we need something that can get approvals for, and land, on an ESR branch.  Changing how JS_VERSION works seems a bit much for there.

Where does that leave us in the short run?  In the short run, I think probably we should just change JS_VERSION to 186 in jsversion.h and live with it for now.  We should then start educating people -- in JS release notes, perhaps, and in all release announcements -- that JS_VERSION is going away and people should stop using it.

As far as the second patch goes in the future when we remove JS_VERSION completely, it seems to me that version()'s output should change to use the MOZJS version macros.  We might even consider making that change on ESR17, even in advance of JS_VERSION going away, but it's probably not super-important to do so.

Anyway.  I think it's probably too late to do anything about JS_VERSION right now, for the very next release.  But we should start on it immediately on trunk, so it gets done before the next ESR.

That helpful at all?  This all is a bit painful, to be sure.  :-(  But once we get it done this time, hopefully it'll be done, and we won't have to do something like this for every future release, which can be run more like clockwork.
Comment 7 Ian Stakenvicius 2012-11-20 05:30:21 PST
... unfortunate, but OK.  So what I'll do is make two patches -- one th handle the immediate release and be targetted for 17ESR, that will just bump JS_VERSION, and the other targetted for trunk and can be possibly backported to the rest of the tags, that will strip out all JS_VERSION stuffs from the code and build system.
Comment 8 Wesley W. Garland 2012-11-20 07:33:58 PST
Getting rid of JS_VERSION in this context isn't anywhere near a show-stopper, IMHO: embedders should /only/ be using it to select the dialect of JS available to script. 

It is not, and has never been, a useful tool for selecting C API. For this reason, I think we can safely leave it at JS 1.8.5, or JS 1.8.6 if we want to reflect that this engine exposes new language features not available in JS 1.8.5.

I have seen embedders use JS_VERSION selection to limit functionality (i.e. turn off generators in MT embeddings), or to try and fix global symbol collisions (bad choice IMO).

IIUC, JS_VERSION was a lot more important 10-15 years ago when JS_VERSION_LATEST was 120 and that broke code written for 100, 110, or 130 that depended on non-strict equality.  JS_VERSION was selected from script with <SCRIPT language="JavaScript1.1">.   It would be interesting to see if that is even supported in Firefox.

So, my vote is "rip it out to simplify the engine", but there is absolutely no need to hurry about it, and the value doesn't even need to change from 1.8.5 AFAIC.
Comment 9 Ian Stakenvicius 2012-11-20 07:35:44 PST
Created attachment 683594 [details] [diff] [review]
immediate patch for 17ESR (minimal change, JS_VERSION bump)

As per Waldo's suggestion, this patch provides the most minimal changes (note, unsure as of now if JS_HAS_* needs to be adjusted; that will need to be done by someone who knows)

Aimed for 17ESR codebase
Comment 10 Ian Stakenvicius 2012-11-20 09:46:54 PST
Created attachment 683640 [details] [diff] [review]
trunk patch to remove JS_VERSION defines

This one's the target change we want to get to eventually, that drops all the deprecated JS_VERSION stuffs
Comment 11 Ian Stakenvicius 2012-11-20 10:06:52 PST
Created attachment 683649 [details] [diff] [review]
fixed patch for trunk to remove JS_VERSION

oops, wrong patch; this one contains all changes
Comment 12 Mike Hommey [:glandium] 2012-11-22 08:31:36 PST
Comment on attachment 682092 [details] [diff] [review]
js-version2.patch

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

::: js/src/configure.in
@@ +222,5 @@
> +AC_DEFINE_UNQUOTED(MOZILLA_UAVERSION,"$MOZILLA_UAVERSION")
> +
> +# Separate version into components for use in shared object naming etc
> +changequote(,)
> +MOZJS_MAJOR_VERSION=`echo $MOZILLA_VERSION | sed "s|\(^[0-9]*\)\.[0-9]*.*|\1|"`

| $AWK -F\. '{print $1}' would be clearer.

@@ +224,5 @@
> +# Separate version into components for use in shared object naming etc
> +changequote(,)
> +MOZJS_MAJOR_VERSION=`echo $MOZILLA_VERSION | sed "s|\(^[0-9]*\)\.[0-9]*.*|\1|"`
> +MOZJS_MINOR_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.\([0-9]*\).*|\1|"`
> +MOZJS_PATCH_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.[0-9]*[^0-9]*||"`

You're not using this variable.

@@ +3230,3 @@
>  if test -n "$MOZ_NATIVE_NSPR"; then
> +    # piggy back on $MOZ_NATIVE_NSPR to set a variable for the nspr check for js.pc
> +    NSPR_PKGCONF_CHECK="nspr >= $NSPR_MINVER"

nspr is a private dependency whether it's the system library or not. This should be in the .pc file unconditionally.

::: js/src/js.pc.in
@@ +4,5 @@
> +
> +Name: SpiderMonkey @MOZILLA_VERSION@
> +Description: The Mozilla library for JavaScript @JS_VERSION@
> +Version: @MOZILLA_VERSION@
> +Requires: @NSPR_PKGCONF_CHECK@

Requires.private

::: js/src/Makefile.in
@@ +34,4 @@
>  TEST_DIRS += tests
>  
>  MODULE		    = js
> +LIBRARY_NAME	    = mozjs-@MOZJS_MAJOR_VERSION@.@MOZJS_MINOR_VERSION@@MOZJS_ALPHA@

I don't really care that much, but the libname-version.so form is usually not preferred, at least on GNU/Linux systems.

@@ +836,5 @@
> +	-e 's|$(at)LIBRARY_NAME$(at)|$(LIBRARY_NAME)|' \
> +	-e 's|$(at)NSPR_CFLAGS$(at)|$(NSPR_CFLAGS)|' \
> +	-e 's|$(at)NSPR_PKGCONF_CHECK$(at)|$(NSPR_PKGCONF_CHECK)|' \
> +	-e 's|$(at)JS_CONFIG_LIBS$(at)|$(JS_CONFIG_LIBS)|' \
> +	-e 's|$(at)MOZ_JS_LIBS$(at)|$(MOZ_JS_LIBS)|'

Using Preprocessor.py would be better.

@@ +847,5 @@
>  install:: $(INSTALLED_HEADERS)
> +	$(SYSINSTALL) $^ $(DESTDIR)$(includedir)/$(LIBRARY_NAME)
> +
> +install:: $(PKG_CONFIG_FILE)
> +	$(SYSINSTALL) $(IFLAGS1) $^ $(DESTDIR)$(libdir)/pkgconfig

The command will also run, and probably fail, with $^ being empty, on non-Linux systems.

That being said, you should use the generic install/copy rule:

PKG_CONFIG_FILES := $(LIBRARY_NAME).pc
PKG_CONFIG_DEST := $(DESTDIR)$(libdir)/pkgconfig
PKG_CONFIG_TARGET := install
INSTALL_TARGET += PKG_CONFIG

This needs to go before the rules.mk include.
Comment 13 Ian Stakenvicius 2012-11-22 08:45:32 PST
(In reply to Mike Hommey [:glandium] from comment #12)
> Comment on attachment 682092 [details] [diff] [review]
> js-version2.patch
> 
> Review of attachment 682092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/configure.in
> @@ +222,5 @@
> > +AC_DEFINE_UNQUOTED(MOZILLA_UAVERSION,"$MOZILLA_UAVERSION")
> > +
> > +# Separate version into components for use in shared object naming etc
> > +changequote(,)
> > +MOZJS_MAJOR_VERSION=`echo $MOZILLA_VERSION | sed "s|\(^[0-9]*\)\.[0-9]*.*|\1|"`
> 
> | $AWK -F\. '{print $1}' would be clearer.
> 

*nod*

> @@ +224,5 @@
> > +# Separate version into components for use in shared object naming etc
> > +changequote(,)
> > +MOZJS_MAJOR_VERSION=`echo $MOZILLA_VERSION | sed "s|\(^[0-9]*\)\.[0-9]*.*|\1|"`
> > +MOZJS_MINOR_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.\([0-9]*\).*|\1|"`
> > +MOZJS_PATCH_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.[0-9]*[^0-9]*||"`
> 
> You're not using this variable.
> 

Not yet.  These variables are going to be the basis for packages that build against spidermoneky to check for versioning in the code.  IE, what JS_VERSION used to provide.  They're here so that they don't need to be constantly defined by hand prior to a release.


> @@ +3230,3 @@
> >  if test -n "$MOZ_NATIVE_NSPR"; then
> > +    # piggy back on $MOZ_NATIVE_NSPR to set a variable for the nspr check for js.pc
> > +    NSPR_PKGCONF_CHECK="nspr >= $NSPR_MINVER"
> 
> nspr is a private dependency whether it's the system library or not. This
> should be in the .pc file unconditionally.
> 

If it's not the system library then it's statically linked, so the .pc file doesn't need it then afaik.  I will check on this, though.  The main need for this line was to ensure the same NSPR_MINVER would be specified in the .pc file as was used at build time, so it only needs to be updated in one place.


> ::: js/src/js.pc.in
> @@ +4,5 @@
> > +
> > +Name: SpiderMonkey @MOZILLA_VERSION@
> > +Description: The Mozilla library for JavaScript @JS_VERSION@
> > +Version: @MOZILLA_VERSION@
> > +Requires: @NSPR_PKGCONF_CHECK@
> 
> Requires.private
> 

... that hasn't been in any .pc file for previous spidermonkeys.  Again, will evaluate and adjust.


> ::: js/src/Makefile.in
> @@ +34,4 @@
> >  TEST_DIRS += tests
> >  
> >  MODULE		    = js
> > +LIBRARY_NAME	    = mozjs-@MOZJS_MAJOR_VERSION@.@MOZJS_MINOR_VERSION@@MOZJS_ALPHA@
> 
> I don't really care that much, but the libname-version.so form is usually
> not preferred, at least on GNU/Linux systems.

A design decision made by the JSAPI folks.  Current spidermonkey is 'libmozjs185.so', proposed update is 'libmozjs187.so', etc.  This just changes it to use the new versioning scheme.  Also please see bug 481878 .


> @@ +836,5 @@
> > +	-e 's|$(at)LIBRARY_NAME$(at)|$(LIBRARY_NAME)|' \
> > +	-e 's|$(at)NSPR_CFLAGS$(at)|$(NSPR_CFLAGS)|' \
> > +	-e 's|$(at)NSPR_PKGCONF_CHECK$(at)|$(NSPR_PKGCONF_CHECK)|' \
> > +	-e 's|$(at)JS_CONFIG_LIBS$(at)|$(JS_CONFIG_LIBS)|' \
> > +	-e 's|$(at)MOZ_JS_LIBS$(at)|$(MOZ_JS_LIBS)|'
> 
> Using Preprocessor.py would be better.

Probably.  Sed is already being used nearly verbatum for 'js-config', though; mirroring current practices i expect would be better and more acceptable to getting this in ESR17 and such, no?


> 
> @@ +847,5 @@
> >  install:: $(INSTALLED_HEADERS)
> > +	$(SYSINSTALL) $^ $(DESTDIR)$(includedir)/$(LIBRARY_NAME)
> > +
> > +install:: $(PKG_CONFIG_FILE)
> > +	$(SYSINSTALL) $(IFLAGS1) $^ $(DESTDIR)$(libdir)/pkgconfig
> 
> The command will also run, and probably fail, with $^ being empty, on
> non-Linux systems.
> 
> That being said, you should use the generic install/copy rule:
> 
> PKG_CONFIG_FILES := $(LIBRARY_NAME).pc
> PKG_CONFIG_DEST := $(DESTDIR)$(libdir)/pkgconfig
> PKG_CONFIG_TARGET := install
> INSTALL_TARGET += PKG_CONFIG
> 
> This needs to go before the rules.mk include.

I had grabbed that from the existing spidermonkey release, however i very much agree that your suggestion is much cleaner.

Will update patch and re-submit.
Comment 14 Mike Hommey [:glandium] 2012-11-22 09:24:55 PST
(In reply to Ian Stakenvicius from comment #13)
> Not yet.  These variables are going to be the basis for packages that build
> against spidermoneky to check for versioning in the code.  IE, what
> JS_VERSION used to provide.  They're here so that they don't need to be
> constantly defined by hand prior to a release.

I was only talking about the last one. Are you going to use it?

> If it's not the system library then it's statically linked.

I doubt it.

> > ::: js/src/Makefile.in
> > @@ +34,4 @@
> > >  TEST_DIRS += tests
> > >  
> > >  MODULE		    = js
> > > +LIBRARY_NAME	    = mozjs-@MOZJS_MAJOR_VERSION@.@MOZJS_MINOR_VERSION@@MOZJS_ALPHA@
> > 
> > I don't really care that much, but the libname-version.so form is usually
> > not preferred, at least on GNU/Linux systems.
> 
> A design decision made by the JSAPI folks.  Current spidermonkey is
> 'libmozjs185.so', proposed update is 'libmozjs187.so', etc.  This just
> changes it to use the new versioning scheme.  Also please see bug 481878 .

Err, sorry, I meant to remove this comment, but forgot.

> > Using Preprocessor.py would be better.
> 
> Probably.  Sed is already being used nearly verbatum for 'js-config',
> though; mirroring current practices i expect would be better and more
> acceptable to getting this in ESR17 and such, no?

I don't expect that to be a problem for approval.

> 
> 
> > 
> > @@ +847,5 @@
> > >  install:: $(INSTALLED_HEADERS)
> > > +	$(SYSINSTALL) $^ $(DESTDIR)$(includedir)/$(LIBRARY_NAME)
> > > +
> > > +install:: $(PKG_CONFIG_FILE)
> > > +	$(SYSINSTALL) $(IFLAGS1) $^ $(DESTDIR)$(libdir)/pkgconfig
> > 
> > The command will also run, and probably fail, with $^ being empty, on
> > non-Linux systems.
> > 
> > That being said, you should use the generic install/copy rule:
> > 
> > PKG_CONFIG_FILES := $(LIBRARY_NAME).pc
> > PKG_CONFIG_DEST := $(DESTDIR)$(libdir)/pkgconfig
> > PKG_CONFIG_TARGET := install
> > INSTALL_TARGET += PKG_CONFIG
> > 
> > This needs to go before the rules.mk include.
> 
> I had grabbed that from the existing spidermonkey release, however i very
> much agree that your suggestion is much cleaner.
> 
> Will update patch and re-submit.

Sadly, in 17, the _TARGET variable is not supported :-/ It was added in bug 784262. I /think/ it would be reasonable to backport the necessary bit, which would look like this:

diff --git a/config/rules.mk b/config/rules.mk
index 9b02a55..3ca9416 100644
--- a/config/rules.mk
+++ b/config/rules.mk
@@ -1654,17 +1654,17 @@ endif
 # FOO_DEST := target_path
 # INSTALL_TARGETS += FOO
 define install_file_template
-libs:: $(2)/$(notdir $(1))
+$(or $(3),libs):: $(2)/$(notdir $(1))
 $(2)/$(notdir $(1)): $(1) $$(call mkdir_deps,$(2))
-       $(INSTALL) $(3) $$< $${@D}
+       $(INSTALL) $(4) $$< $${@D}
 endef
 $(foreach category,$(INSTALL_TARGETS),\
   $(if $($(category)_DEST),,$(error Missing $(category)_DEST))\
   $(foreach file,$($(category)_FILES),\
-    $(eval $(call install_file_template,$(file),$($(category)_DEST),$(IFLAGS1)))\
+    $(eval $(call install_file_template,$(file),$($(category)_DEST),$($(category)_TARGET),$(IFLAGS1)))\
   )\
   $(foreach file,$($(category)_EXECUTABLES),\
-    $(eval $(call install_file_template,$(file),$($(category)_DEST),$(IFLAGS2)))\
+    $(eval $(call install_file_template,$(file),$($(category)_DEST),$($(category)_TARGET),$(IFLAGS2)))\
   )\
 )
Comment 15 Ian Stakenvicius 2012-11-22 10:34:48 PST
(In reply to Mike Hommey [:glandium] from comment #14)
> (In reply to Ian Stakenvicius from comment #13)
> > Not yet.  These variables are going to be the basis for packages that build
> > against spidermoneky to check for versioning in the code.  IE, what
> > JS_VERSION used to provide.  They're here so that they don't need to be
> > constantly defined by hand prior to a release.
> 
> I was only talking about the last one. Are you going to use it?
> 

Personally, I don't know if PATCH_VERSION will be of any use.  I don't expect so.  However, it will depend on whether or not the API or ABI changes from one patch version to another.  Since i'm an innocent bystander rather than a mozilla dev, I figured i'd cover all bases in case this possibly occurrs.


> 
> > 
> > 
> > > 
> > > @@ +847,5 @@
> > > >  install:: $(INSTALLED_HEADERS)
> > > > +	$(SYSINSTALL) $^ $(DESTDIR)$(includedir)/$(LIBRARY_NAME)
> > > > +
> > > > +install:: $(PKG_CONFIG_FILE)
> > > > +	$(SYSINSTALL) $(IFLAGS1) $^ $(DESTDIR)$(libdir)/pkgconfig
> > > 
> > > The command will also run, and probably fail, with $^ being empty, on
> > > non-Linux systems.
> > > 
> > > That being said, you should use the generic install/copy rule:
> > > 
> > > PKG_CONFIG_FILES := $(LIBRARY_NAME).pc
> > > PKG_CONFIG_DEST := $(DESTDIR)$(libdir)/pkgconfig
> > > PKG_CONFIG_TARGET := install
> > > INSTALL_TARGET += PKG_CONFIG
> > > 
> > > This needs to go before the rules.mk include.
> > 
> > I had grabbed that from the existing spidermonkey release, however i very
> > much agree that your suggestion is much cleaner.
> > 
> > Will update patch and re-submit.
> 
> Sadly, in 17, the _TARGET variable is not supported :-/ It was added in bug
> 784262. I /think/ it would be reasonable to backport the necessary bit,
> which would look like this:  [ Snip! ]
> 

...should I be including that patch as an attachment to this bug or does the comment suffice?
Comment 16 Ian Stakenvicius 2012-11-30 13:43:24 PST
Created attachment 687270 [details] [diff] [review]
setup versioning and pkg-config support in build system

Updated patch with recommendations by glandium.  Needs his proposed patch for generic _TARGET support.

Still unsure about Requires.private , made nspr always part of Requires tho.
Comment 17 Ted Mielczarek [:ted.mielczarek] 2013-01-16 10:57:46 PST
Comment on attachment 687270 [details] [diff] [review]
setup versioning and pkg-config support in build system

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

If glandium is happy with the .pc bits then I'm fine. I just have a few comments. Sorry about the delay.

::: js/src/configure.in
@@ +223,5 @@
> +# Separate version into components for use in shared object naming etc
> +changequote(,)
> +MOZJS_MAJOR_VERSION=`echo $MOZILLA_VERSION | sed "s|\(^[0-9]*\)\.[0-9]*.*|\1|"`
> +MOZJS_MINOR_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.\([0-9]*\).*|\1|"`
> +MOZJS_PATCH_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.[0-9]*[^0-9]*||"`

This is likely to be empty most of the time. Is that okay?

@@ +235,5 @@
> +AC_DEFINE_UNQUOTED(MOZJS_MAJOR_VERSION,"$MOZJS_MAJOR_VERSION")
> +AC_DEFINE_UNQUOTED(MOZJS_MINOR_VERSION,"$MOZJS_MINOR_VERSION")
> +AC_SUBST(MOZJS_MAJOR_VERSION)
> +AC_SUBST(MOZJS_MINOR_VERSION)
> +AC_SUBST(MOZJS_PATCH_VERSION)

Do you expect to really use this somewhere? Is this just for sanity's sake?

@@ +34,4 @@
>  TEST_DIRS += tests
>  
>  MODULE		    = js
> +LIBRARY_NAME	    = mozjs-@MOZJS_MAJOR_VERSION@.@MOZJS_MINOR_VERSION@@MOZJS_ALPHA@

Do we have buy-in to ship mozjs like this as part of the Firefox build? (I guess we only really ship mozjs standalone on Windows anyway.) Does this break the Windows build, since it doesn't use js-config to figure out what to link to?

@@ +771,3 @@
>  js-config: js-config.in Makefile $(DEPTH)/config/autoconf.mk $(topsrcdir)/config/config.mk $(topsrcdir)/config/rules.mk
> +	$(RM) $@.tmp
> +	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(JS_CONFIG_SUBSTITUTIONS) $< > $@.tmp \

You can go even farther here and use PP_TARGETS and avoid writing a rule at all:
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#1591

@@ +777,5 @@
>  SDK_BINARY = js-config
>  
> +
> +$(LIBRARY_NAME).pc: js.pc.in js-config
> +	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(JS_CONFIG_SUBSTITUTIONS) $< > $@

Same here, you can use PP_TARGETS.
Comment 18 Ian Stakenvicius 2013-01-16 12:06:43 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #17)
> ::: js/src/configure.in
> @@ +223,5 @@
> > +# Separate version into components for use in shared object naming etc
> > +changequote(,)
> > +MOZJS_MAJOR_VERSION=`echo $MOZILLA_VERSION | sed "s|\(^[0-9]*\)\.[0-9]*.*|\1|"`
> > +MOZJS_MINOR_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.\([0-9]*\).*|\1|"`
> > +MOZJS_PATCH_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.[0-9]*[^0-9]*||"`
> 
> This is likely to be empty most of the time. Is that okay?
> 

I figured PATCH and ALPHA would be most of the time, yes.  What it does do, though, is allow a (linux) developer to have installed both a released version as well as the patched/alpha version from m-c without having a conflict.  IE, say someone's starting development of something using 21.0a and they get it working, then when 21.0 is released they may want to keep their working version around while they update the code to work properly with 21.0 final.

> @@ +235,5 @@
> > +AC_DEFINE_UNQUOTED(MOZJS_MAJOR_VERSION,"$MOZJS_MAJOR_VERSION")
> > +AC_DEFINE_UNQUOTED(MOZJS_MINOR_VERSION,"$MOZJS_MINOR_VERSION")
> > +AC_SUBST(MOZJS_MAJOR_VERSION)
> > +AC_SUBST(MOZJS_MINOR_VERSION)
> > +AC_SUBST(MOZJS_PATCH_VERSION)
> 
> Do you expect to really use this somewhere? Is this just for sanity's sake?
> 

(see above)


> @@ +34,4 @@
> >  TEST_DIRS += tests
> >  
> >  MODULE		    = js
> > +LIBRARY_NAME	    = mozjs-@MOZJS_MAJOR_VERSION@.@MOZJS_MINOR_VERSION@@MOZJS_ALPHA@
> 
> Do we have buy-in to ship mozjs like this as part of the Firefox build? (I
> guess we only really ship mozjs standalone on Windows anyway.) Does this
> break the Windows build, since it doesn't use js-config to figure out what
> to link to?


mozjs ships separately for Windows?  From what I saw from the build system, even on Windows, it looked like the js stuff was included into libxul for firefox and other mozilla products...  If it doesn't, why doesn't it?

Or are you referring here to a pre-built spidermonkey for windows?  That I haven't taken into consideration yet, although IIRC there is a bug open about adding version information for the windows DLL, from long ago..   I'll look into it.


> 
> @@ +771,3 @@
> >  js-config: js-config.in Makefile $(DEPTH)/config/autoconf.mk $(topsrcdir)/config/config.mk $(topsrcdir)/config/rules.mk
> > +	$(RM) $@.tmp
> > +	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(JS_CONFIG_SUBSTITUTIONS) $< > $@.tmp \
> 
> You can go even farther here and use PP_TARGETS and avoid writing a rule at
> all:
> http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#1591
> 
> @@ +777,5 @@
> >  SDK_BINARY = js-config
> >  
> > +
> > +$(LIBRARY_NAME).pc: js.pc.in js-config
> > +	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(JS_CONFIG_SUBSTITUTIONS) $< > $@
> 
> Same here, you can use PP_TARGETS.


As this patch isn't meant just for HEAD (ie, the goal here is to support an ESR17 release of spidermonkey in addition to future releases), I would prefer not to go the PP_TARGETS route since that doesn't exist in ESR17's rules.mk
Comment 19 Ted Mielczarek [:ted.mielczarek] 2013-01-16 12:26:48 PST
(In reply to Ian Stakenvicius from comment #18)
> mozjs ships separately for Windows?  From what I saw from the build system,
> even on Windows, it looked like the js stuff was included into libxul for
> firefox and other mozilla products...  If it doesn't, why doesn't it?

http://mxr.mozilla.org/mozilla-central/source/configure.in#7987

The PGO linker runs out of address space when we link JS into libxul on Windows.

> As this patch isn't meant just for HEAD (ie, the goal here is to support an
> ESR17 release of spidermonkey in addition to future releases), I would
> prefer not to go the PP_TARGETS route since that doesn't exist in ESR17's
> rules.mk

Okay, please file a followup on that.
Comment 20 Ian Stakenvicius 2013-01-16 13:09:49 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #19)
> (In reply to Ian Stakenvicius from comment #18)
> > mozjs ships separately for Windows?  From what I saw from the build system,
> > even on Windows, it looked like the js stuff was included into libxul for
> > firefox and other mozilla products...  If it doesn't, why doesn't it?
> 
> http://mxr.mozilla.org/mozilla-central/source/configure.in#7987
> 
> The PGO linker runs out of address space when we link JS into libxul on
> Windows.
> 

*OH*.  That makes this a higher priority...  

At any given moment, though, since the source of the MOZJS_VERSION_* values is what's recorded in config/milestone.txt , and it should always match the value defined in the base config/milestone.txt , we should be able to ensure windows builds link against the correct LIBRARY_VERSION for any given snapshot of the source tree, right?
Comment 21 darkxst 2013-01-16 15:42:23 PST
Ian, we currently have one other minor versioning patch used for creating our tarballs. Would probably make sense to wrap this into your patch set, with the new variables.

http://people.ubuntu.com/~ricotz/mozjs/moz188-fix-version.patch
Comment 22 Ted Mielczarek [:ted.mielczarek] 2013-01-16 18:02:46 PST
(In reply to Ian Stakenvicius from comment #20)
> At any given moment, though, since the source of the MOZJS_VERSION_* values
> is what's recorded in config/milestone.txt , and it should always match the
> value defined in the base config/milestone.txt , we should be able to ensure
> windows builds link against the correct LIBRARY_VERSION for any given
> snapshot of the source tree, right?

While true, I think it would be simpler to just skip putting the version in the library name in this case. We could always do this when OS_TARGET is WINNT, or we could always do this when building JS as part of Firefox (I can't recall if we have a handy make variable for that).
Comment 23 Ian Stakenvicius 2013-01-17 06:14:49 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #22)
> (In reply to Ian Stakenvicius from comment #20)
> > At any given moment, though, since the source of the MOZJS_VERSION_* values
> > is what's recorded in config/milestone.txt , and it should always match the
> > value defined in the base config/milestone.txt , we should be able to ensure
> > windows builds link against the correct LIBRARY_VERSION for any given
> > snapshot of the source tree, right?
> 
> While true, I think it would be simpler to just skip putting the version in
> the library name in this case. We could always do this when OS_TARGET is
> WINNT, or we could always do this when building JS as part of Firefox (I
> can't recall if we have a handy make variable for that).

When libmozjs is embedded into libxul, all the linking is done in the main build system rather than via the js/src/ build system, right?  Perhaps the same should be done in mozilla-products builds even when building it as a standalone library?

Curious, though -- what happens on Windows if a user has say, thunderbird-19 and firefox-20 , in terms of the external mozjs?  They will be different, won't that cause some possible conflicts?
Comment 24 Ian Stakenvicius 2013-01-17 06:15:46 PST
(In reply to darkxst from comment #21)
> Ian, we currently have one other minor versioning patch used for creating
> our tarballs. Would probably make sense to wrap this into your patch set,
> with the new variables.
> 
> http://people.ubuntu.com/~ricotz/mozjs/moz188-fix-version.patch

I completely missed that function -- absolutely, this should be updated.  Thanks for pointing it out!
Comment 25 Ted Mielczarek [:ted.mielczarek] 2013-01-17 06:18:39 PST
(In reply to Ian Stakenvicius from comment #23)
> When libmozjs is embedded into libxul, all the linking is done in the main
> build system rather than via the js/src/ build system, right?  Perhaps the
> same should be done in mozilla-products builds even when building it as a
> standalone library?

We simply link libjs_static into libxul in those cases. Not linking the shared library in js/src would make things more complicated, 

> Curious, though -- what happens on Windows if a user has say, thunderbird-19
> and firefox-20 , in terms of the external mozjs?  They will be different,
> won't that cause some possible conflicts?

All our releases use their own mozjs and don't install it as a system-wide library, so there's no issue there.
Comment 26 darkxst 2013-01-31 21:53:58 PST
Ian, I tested these patches against esr17 snapshot, a few minor issues.

Your first patch removes JS_VERSION, but then the second patch tries to use this in the pkgconfig file. This causes the python preprocessor script to spit the dummy.

secondly (even after removing JS_VERSION from .pc.in file) the pkgconfig file does not appear to get installed.
Comment 27 darkxst 2013-02-01 17:15:38 PST
Created attachment 709302 [details] [diff] [review]
correct shared library versioning

To be distro-friendly shared libraries should have a version suffix
that indicates ABI/API compatibility.

This was lifted from the Wes' libname patches from the js185 release,
with only minor changes. It is applied on top of Ian's versioning patches.

Incidentally this should also resolve the concerns with windows library names when building mozilla products, however it may not work out for windows embedders just yet, since I don't think it will install the .dll file.
Comment 28 darkxst 2013-02-02 16:14:43 PST
Comment on attachment 687270 [details] [diff] [review]
setup versioning and pkg-config support in build system

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

::: js/src/configure.in
@@ +483,5 @@
> +# just install .pc file for linux, so far
> +PKG_CONFIG_FILES := $(LIBRARY_NAME).pc
> +PKG_CONFIG_DEST := $(DESTDIR)$(libdir)/pkgconfig
> +PKG_CONFIG_TARGET := install
> +INSTALL_TARGET += PKG_CONFIG

This should be 'INSTALL_TARGETS'
Comment 29 darkxst 2013-02-02 17:14:44 PST
Created attachment 709431 [details] [diff] [review]
correct shared library versioning (for non-mozilla builds only)

Slight update on previous version to allow correct versioning for windows embedders.

Mozilla builds such as firefox appear to set MOZILLA_OFFICIAL=1, so when this is set we won't touch the LIBRARY_NAME. This should avoid any issues with mozilla windows building linking to the static library.
Comment 30 darkxst 2013-02-02 21:43:50 PST
Created attachment 709445 [details] [diff] [review]
correct library versioning with ABI versions (for non-mozilla builds only)

another small update to explicity declare the static library name, this saves having to use a mv command after SYSINSTALL. It will also avoid the breakage in make install caused by the move target being specified as 2 files 'lib.a lib.a.desc'. 

Which by the way I have no idea what that lib.a.desc file is, but presumably its used for the window builds and not linux?
Comment 31 Mike Hommey [:glandium] 2013-02-02 22:30:54 PST
Comment on attachment 709431 [details] [diff] [review]
correct shared library versioning (for non-mozilla builds only)

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

::: js/src/Makefile.in
@@ +39,2 @@
>  MODULE		    = js
> +ifneq (,$(MOZILLA_OFFICIAL))

MOZILLA_OFFICIAL is not a proper knob for this. There are too many things not building with MOZILLA_OFFICIAL, starting with developers.
Comment 32 darkxst 2013-02-03 00:34:33 PST
is there something better to use? Do the MOZ_APP_* macros suffer the same problem?
Comment 33 Ian Stakenvicius 2013-02-04 06:10:24 PST
(In reply to darkxst from comment #27 and others)
> Created attachment 709302 [details] [diff] [review]
> correct shared library versioning
> 
> To be distro-friendly shared libraries should have a version suffix
> that indicates ABI/API compatibility.
> 
> This was lifted from the Wes' libname patches from the js185 release,
> with only minor changes. It is applied on top of Ian's versioning patches.
> 
> Incidentally this should also resolve the concerns with windows library
> names when building mozilla products, however it may not work out for
> windows embedders just yet, since I don't think it will install the .dll
> file.


Thanks for the analysis and the patches.  Your work is much appreciated.

Regarding the version suffix -- I had reached concensus with the varoius members of #jsapi on irc.mozilla.org that we shouldn't bother with the version suffix, because mozilla's codebase (and therefore a separate spidermoney) will never enforce it.  The codebase as of each release is essentially to be treated as independent and will be neiter backward-compatible nor forward-compatible with the others.  The codebase also moves too fast (a new release every 6 weeks or something??) for it to be worthwhile trying to use the version suffix to indicate minor patch or security fixes.  So given this reasoning, it was decided to just leave it off.

The issue you found with the pc.in file I had thoought I'd fixed; which could mean that I have a newer patchset than what is attached to this bug.  I'll check my tree and compare and update anything appropriately.

Your work towards building different LIBRARY_* based on windows/other and so forth  looks promising too, I'll also look into alternatives to what you did with MOZILLA_OFFICIAL -- although i am uncertain if this is pertinent, since the version#'s are common throughout the entire mozilla codebase and as such should be easy enough to specify at link time.

The JS_CONFIG_MOZ_JZ_LIBS is a good idea for abstraction, but will need additional contents (for instance, one needs to add at lease nspr to that, if nspr is external).
Comment 34 darkxst 2013-02-04 13:21:48 PST
Just trying to keep things moving forward ;)

re Version suffix, this was mostly just to keep the distro's happy, but I guess they could just as easily fix that with their packaging scripts. It does leave one possibility though, say a distro decided to track esr releases for a particular version. I assume API would be stable, but do these guarantee ABI Compatibility?
Comment 35 Ted Mielczarek [:ted.mielczarek] 2013-02-06 17:34:20 PST
Comment on attachment 709445 [details] [diff] [review]
correct library versioning with ABI versions (for non-mozilla builds only)

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

I don't think we want this patch. I think we should just put the version numbers in the library filename and be done with it. If distros want to use soversioning they can do it themselves.
Comment 36 Ted Mielczarek [:ted.mielczarek] 2013-02-06 17:44:35 PST
So I filed bug 838915 on adding an easy way to determine if we're doing a standalone spidermonkey build. If we add that then you can put anything you want inside that ifdef (like adding version numbers to library names) without worrying about breaking the Firefox build. I don't know if I'll have time to patch that right away, but I'll get to it.
Comment 37 Mike Hommey [:glandium] 2013-02-06 23:48:43 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #35)
> I don't think we want this patch. I think we should just put the version
> numbers in the library filename and be done with it. If distros want to use
> soversioning they can do it themselves.

I've carried a patch doing that for a while, and it's annoying to keep patches that long, because they occasionally conflict. Also, it doesn't help making things binary compatible between distros, although in the mozjs case, it's mostly a theoretical concern.
Comment 38 Ted Mielczarek [:ted.mielczarek] 2013-02-07 10:27:05 PST
Okay, I will defer to your expertise.
Comment 39 darkxst 2013-02-07 15:31:16 PST
Created attachment 711564 [details] [diff] [review]
correct library versioning with ABI versions (for non-mozilla builds only)

update to use JS_STANDALONE from bug 838915

also update JS_CONFIG_MOZ_JS_LIBS to match exactly what was done in configure.in. For JS_STANDALONE=0 this will provide the same result as MOZ_JS_LIBS. 

Also MOZ_JS_LIBS is now unused, should I remove that from configure.in?
Comment 40 darkxst 2013-02-07 15:44:33 PST
(In reply to Ian Stakenvicius from comment #33)
> (In reply to darkxst from comment #27 and others)
> > Created attachment 709302 [details] [diff] [review]
> > correct shared library versioning

> 
> The JS_CONFIG_MOZ_JZ_LIBS is a good idea for abstraction, but will need
> additional contents (for instance, one needs to add at lease nspr to that,
> if nspr is external).

Doesnt package config takes care of nspr via Requires?. 

For js-config, both JS_CONFIG_LIBS and JS_CONFIG_MOZ_JS_LIBS get output, so that covers nspr also.
Comment 41 darkxst 2013-02-10 15:07:49 PST
I end up with a leftover ".mkdir.done" timestamp installed on the system, when using the generic install target to install the pkgconfig file.
Comment 42 Landry Breuil (:gaston) 2013-02-11 23:01:52 PST
(In reply to Mike Hommey [:glandium] from comment #37)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #35)
> > I don't think we want this patch. I think we should just put the version
> > numbers in the library filename and be done with it. If distros want to use
> > soversioning they can do it themselves.
> 
> I've carried a patch doing that for a while, and it's annoying to keep
> patches that long, because they occasionally conflict. Also, it doesn't help
> making things binary compatible between distros, although in the mozjs case,
> it's mostly a theoretical concern.

I second that, we (openbsd)'ve been carrying patches here to set out own versionning (from SO_VERSION in the env) since a while , see http://www.openbsd.org/cgi-bin/cvsweb/ports/devel/spidermonkey/patches/patch-Makefile_in?rev=1.2 and http://www.openbsd.org/cgi-bin/cvsweb/ports/devel/spidermonkey/patches/patch-configure_in?rev=1.2 for example.
Comment 43 darkxst 2013-02-12 18:50:19 PST
Created attachment 713245 [details] [diff] [review]
correct library versioning with ABI versions (fix hardcoded static library names)

There where a couple of instances where the static library name was hard coded into sub-Makefiles. This update fixes that.
Comment 44 darkxst 2013-02-12 22:08:32 PST
Created attachment 713284 [details] [diff] [review]
a couple more minor fixes

These are two more minor fixes.

first one avoids installing libjs.a.desc file on non windows targets (since I don't think its needed)

second one patches the INSTALL_TARGET rule to remove the '.mkdir.done' timestamp
Comment 45 Mike Hommey [:glandium] 2013-02-13 08:09:52 PST
Comment on attachment 713245 [details] [diff] [review]
correct library versioning with ABI versions (fix hardcoded static library names)

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

I don't think this is needed.
Comment 46 Mike Hommey [:glandium] 2013-02-13 08:15:31 PST
Comment on attachment 713284 [details] [diff] [review]
a couple more minor fixes

>-LIBRARY			:= $(REAL_LIBRARY) $(REAL_LIBRARY).$(LIBS_DESC_SUFFIX)
>+LIBRARY			:= $(REAL_LIBRARY)
>+ifeq ($(OS_ARCH),WINNT)
>+LIBRARY += $(REAL_LIBRARY).$(LIBS_DESC_SUFFIX)
>+endif

You certainly don't want to do that. You want to change the install rule in Makefile.in to use REAL_LIBRARY, instead.

>@@ -1636,6 +1636,7 @@ define install_file_template
> $(or $(3),libs):: $(2)/$(notdir $(1))
> $(2)/$(notdir $(1)): $(1) $$(call mkdir_deps,$(2))
> 	$(INSTALL) $(4) $$< $${@D}
>+	rm -f $(2)/.mkdir.done

This is certainly not something we can do, that will make us rebuild a whole lot of things on incremental builds
Comment 47 Mike Hommey [:glandium] 2013-02-13 08:18:04 PST
Ian, are you working on updating your patch with Ted's comments?
Comment 48 darkxst 2013-02-13 12:09:14 PST
(In reply to Mike Hommey [:glandium] from comment #45)
> Comment on attachment 713245 [details] [diff] [review]
> correct library versioning with ABI versions (fix hardcoded static library
> names)
> 
> Review of attachment 713245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think this is needed.

which bit exactly? should I just leave STATIC_LIBRARY_NAME=js_static?
Comment 49 Mike Hommey [:glandium] 2013-02-13 12:12:29 PST
(In reply to darkxst from comment #48)
> (In reply to Mike Hommey [:glandium] from comment #45)
> > I don't think this is needed.
> 
> which bit exactly?

The whole patch.
Comment 50 darkxst 2013-02-13 12:42:49 PST
(In reply to Mike Hommey [:glandium] from comment #49)
> (In reply to darkxst from comment #48)
> > (In reply to Mike Hommey [:glandium] from comment #45)
> > > I don't think this is needed.
> > 
> > which bit exactly?
> 
> The whole patch.

I thought you wanted to keep the soname versioning stuff?
Comment 51 darkxst 2013-02-13 12:46:58 PST
(In reply to Mike Hommey [:glandium] from comment #46)
> Comment on attachment 713284 [details] [diff] [review]
> a couple more minor fixes
> 
> >-LIBRARY			:= $(REAL_LIBRARY) $(REAL_LIBRARY).$(LIBS_DESC_SUFFIX)
> >+LIBRARY			:= $(REAL_LIBRARY)
> >+ifeq ($(OS_ARCH),WINNT)
> >+LIBRARY += $(REAL_LIBRARY).$(LIBS_DESC_SUFFIX)
> >+endif
> 
> You certainly don't want to do that. You want to change the install rule in
> Makefile.in to use REAL_LIBRARY, instead.
> 
OK

> >@@ -1636,6 +1636,7 @@ define install_file_template
> > $(or $(3),libs):: $(2)/$(notdir $(1))
> > $(2)/$(notdir $(1)): $(1) $$(call mkdir_deps,$(2))
> > 	$(INSTALL) $(4) $$< $${@D}
> >+	rm -f $(2)/.mkdir.done
> 
> This is certainly not something we can do, that will make us rebuild a whole
> lot of things on incremental builds

So what can we do here? Currently end up with the timestamp being installed onto the system. i.e. $(libdir)/pkgconfig/.mkdir.done, surely that should not happen.
Comment 52 Mike Hommey [:glandium] 2013-02-13 14:17:14 PST
(In reply to darkxst from comment #50)
> I thought you wanted to keep the soname versioning stuff?

Ian's patch adds soname versioning.
Comment 53 darkxst 2013-02-13 14:58:18 PST
Created attachment 713676 [details] [diff] [review]
use REAL_LIBRARY for the install target
Comment 54 darkxst 2013-02-13 15:00:39 PST
Created attachment 713678 [details] [diff] [review]
fix MOZ_JS_LIBS to use correct library name

This is required or else js-config will be broken due to hardcoded library names in MOZ_JS_LIBS.
Comment 55 darkxst 2013-02-14 13:49:54 PST
Created attachment 714081 [details] [diff] [review]
install versioned static library

Use versioned static library name to avoid any name conflicts with multiple spidermonkey versions installed.

This should be included in Ian's patch.
Comment 56 Jason Orendorff [:jorendorff] 2013-02-18 12:37:13 PST
Are the four latest patches in this bug ready for review?
Comment 57 darkxst 2013-02-18 15:10:31 PST
Created attachment 715259 [details] [diff] [review]
setup versioning and pkg-config support in build system v3

This is Ian's patch rebased against trunk and minor fixes
  - use JS_STANDALONE for library naming
  - fix INSTALL_TARGETS typo
  - remove JS_VERSION and add -include RequiredDefines.h (Bug 835551) to packageconfig file
  - squashed attachments 713676 and 714081 into this patch
Comment 58 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-18 15:32:13 PST
Comment on attachment 715259 [details] [diff] [review]
setup versioning and pkg-config support in build system v3

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

Not that you should trust anything I say technically or review-wise here, but this looks pretty nice to me.  :-)

::: js/src/Makefile.in
@@ +37,2 @@
>  LIBRARY_NAME	    = mozjs
> +MODULE		    = js

Total nitpickery, but I'd much rather read |ifdef JS_STANDALONE| than an ifndef.  It might be a different matter in non-js/src files, but here I think we understand the if-we're-compiling-standalone distinction well enough to understand what's being done.
Comment 59 Mike Hommey [:glandium] 2013-02-18 22:39:52 PST
Comment on attachment 713678 [details] [diff] [review]
fix MOZ_JS_LIBS to use correct library name

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

r+ with the variable name changed.

::: js/src/Makefile.in
@@ +811,5 @@
>  # - OS_LIBS includes libraries selected by the configure script.
>  # - EXTRA_LIBS includes libraries selected by this Makefile.
>  JS_CONFIG_LIBS=$(EXTRA_DSO_LDOPTS) $(OS_LIBS) $(EXTRA_LIBS)
> +ifdef GNU_CC
> +MOZ_JS_LIBS=-L$(libdir) -l$(LIBRARY_NAME)

Since MOZ_JS_LIBS has a different use outside js/src (and is used in js/src/config/rules.mk), it would be better to rename it while you're here.

An issue that could be subject of a followup is that js-config's --prefix and --libdir don't have an effect. But that's not exactly a new issue.
Comment 60 Mike Hommey [:glandium] 2013-02-18 23:00:12 PST
Comment on attachment 715259 [details] [diff] [review]
setup versioning and pkg-config support in build system v3

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

::: js/src/Makefile.in
@@ +37,4 @@
>  LIBRARY_NAME	    = mozjs
> +MODULE		    = js
> +else
> +MODULE		    = js-@MOZJS_MAJOR_VERSION@.@MOZJS_MINOR_VERSION@@MOZJS_ALPHA@

No need to change MODULE

@@ +604,5 @@
> +# just install .pc file for linux, so far
> +PKG_CONFIG_FILES := $(LIBRARY_NAME).pc
> +PKG_CONFIG_DEST := $(DESTDIR)$(libdir)/pkgconfig
> +PKG_CONFIG_TARGET := install
> +INSTALL_TARGETS += PKG_CONFIG

I know I'm the one who said to use INSTALL_TARGETS stuff here, but until we have something proper to avoid the .mkdir.done directory, we should just stick to:

install:: $(LIBRARY_NAME).pc
        $(SYSINSTALL) $^ $(DESTDIR)$(libdir)/pkgconfig

@@ +939,5 @@
>  
> +install:: $(REAL_LIBRARY) $(SHARED_LIBRARY) $(IMPORT_LIBRARY)
> +ifneq (,$(REAL_LIBRARY))
> +	$(SYSINSTALL) $(REAL_LIBRARY) $(DESTDIR)$(libdir)
> +	mv -f $(DESTDIR)$(libdir)/$(REAL_LIBRARY) $(subst $(STATIC_LIBRARY_NAME),$(LIBRARY_NAME),$(DESTDIR)$(libdir)/$(REAL_LIBRARY))

Come to think of it, I'm not sure we should install the static library at all.

::: js/src/configure.in
@@ +224,5 @@
> +MOZILLA_UAVERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir -uaversion`
> +
> +AC_DEFINE_UNQUOTED(MOZILLA_VERSION,"$MOZILLA_VERSION")
> +AC_DEFINE_UNQUOTED(MOZILLA_VERSION_U,$MOZILLA_VERSION)
> +AC_DEFINE_UNQUOTED(MOZILLA_UAVERSION,"$MOZILLA_UAVERSION")

You're not using any of these three afaict. Only MOZILLA_VERSION is used, but as an AC_SUBST (already in configure.in), not an AC_DEFINE.

@@ +239,5 @@
> +  MOZJS_ALPHA=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.[0-9\.]*\([^0-9]\).*|\1|"`
> +  changequote([,])
> +fi
> +AC_DEFINE_UNQUOTED(MOZJS_MAJOR_VERSION,"$MOZJS_MAJOR_VERSION")
> +AC_DEFINE_UNQUOTED(MOZJS_MINOR_VERSION,"$MOZJS_MINOR_VERSION")

Likewise, these are used as AC_SUBSTs, not AC_DEFINEs.

::: js/src/js-config.in
@@ +107,4 @@
>  fi
>  
>  if test "$echo_cflags" = "yes"; then
> +    echo "-I$includedir/$LIBRARY_NAME $NSPR_CFLAGS"

Missing RequiredDefines.h?

::: js/src/js.pc.in
@@ +5,5 @@
> +
> +Name: SpiderMonkey @MOZILLA_VERSION@
> +Description: The Mozilla library for JavaScript
> +Version: @MOZILLA_VERSION@
> +Requires: @NSPR_PKGCONF_CHECK@

Requires.private if we ship the static library, or just remove the Requires line if we don't.

@@ +7,5 @@
> +Description: The Mozilla library for JavaScript
> +Version: @MOZILLA_VERSION@
> +Requires: @NSPR_PKGCONF_CHECK@
> +Libs: -L${libdir} -l@LIBRARY_NAME@
> +Cflags: -include ${includedir}/@LIBRARY_NAME@/js/RequiredDefines.h -I${includedir}/@LIBRARY_NAME@

Are we really installing headers under $includedir/$LIBRARY_NAME/js ?
Comment 61 darkxst 2013-02-19 01:20:39 PST
(In reply to Mike Hommey [:glandium] from comment #60)
> Comment on attachment 715259 [details] [diff] [review]
> setup versioning and pkg-config support in build system v3
> 
> Review of attachment 715259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/Makefile.in
> @@ +37,4 @@
> >  LIBRARY_NAME	    = mozjs
> > +MODULE		    = js
> > +else
> > +MODULE		    = js-@MOZJS_MAJOR_VERSION@.@MOZJS_MINOR_VERSION@@MOZJS_ALPHA@
> 
> No need to change MODULE
Previously in 185 release, headers were installed to /usr/include/js, discussed on IRC (w/ Waldo and sstangl), that it might be better to tack the version number onto that (/usr/include/js17), rather than using the library name (mozjs17). However I guess it hardly matters, since for the most part that should be transparent via pkgconfig or js-config.

> 
> 
> @@ +939,5 @@
> >  
> > +install:: $(REAL_LIBRARY) $(SHARED_LIBRARY) $(IMPORT_LIBRARY)
> > +ifneq (,$(REAL_LIBRARY))
> > +	$(SYSINSTALL) $(REAL_LIBRARY) $(DESTDIR)$(libdir)
> > +	mv -f $(DESTDIR)$(libdir)/$(REAL_LIBRARY) $(subst $(STATIC_LIBRARY_NAME),$(LIBRARY_NAME),$(DESTDIR)$(libdir)/$(REAL_LIBRARY))
> 
> Come to think of it, I'm not sure we should install the static library at
> all.

Well I don't know if anyone uses the static library, I suppose its unlikely on linux, but I do wonder about windows? I don't really have much of a broader idea of what various other projects use/do with spidermonkey, I'm really just limited to the GNOME side.


> @@ +7,5 @@
> > +Description: The Mozilla library for JavaScript
> > +Version: @MOZILLA_VERSION@
> > +Requires: @NSPR_PKGCONF_CHECK@
> > +Libs: -L${libdir} -l@LIBRARY_NAME@
> > +Cflags: -include ${includedir}/@LIBRARY_NAME@/js/RequiredDefines.h -I${includedir}/@LIBRARY_NAME@
> 
> Are we really installing headers under $includedir/$LIBRARY_NAME/js ?

yes, the headers from /js/public are all included elsewhere with |include "js/header"|, I don't think we can do anything about that in the short-term, given these patches are meant to land on esr17. See https://hg.mozilla.org/mozilla-central/rev/c06650ce0770 for Waldo's comments on the structure etc.


I will address your other comments and upload a new patch.
Comment 62 Mike Hommey [:glandium] 2013-02-19 01:40:47 PST
(In reply to darkxst from comment #61)
> (In reply to Mike Hommey [:glandium] from comment #60)
> > Comment on attachment 715259 [details] [diff] [review]
> > setup versioning and pkg-config support in build system v3
> > 
> > Review of attachment 715259 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/Makefile.in
> > @@ +37,4 @@
> > >  LIBRARY_NAME	    = mozjs
> > > +MODULE		    = js
> > > +else
> > > +MODULE		    = js-@MOZJS_MAJOR_VERSION@.@MOZJS_MINOR_VERSION@@MOZJS_ALPHA@
> > 
> > No need to change MODULE
> Previously in 185 release, headers were installed to /usr/include/js,
> discussed on IRC (w/ Waldo and sstangl), that it might be better to tack the
> version number onto that (/usr/include/js17),

Ah yes, indeed.

> > @@ +939,5 @@
> > >  
> > > +install:: $(REAL_LIBRARY) $(SHARED_LIBRARY) $(IMPORT_LIBRARY)
> > > +ifneq (,$(REAL_LIBRARY))
> > > +	$(SYSINSTALL) $(REAL_LIBRARY) $(DESTDIR)$(libdir)
> > > +	mv -f $(DESTDIR)$(libdir)/$(REAL_LIBRARY) $(subst $(STATIC_LIBRARY_NAME),$(LIBRARY_NAME),$(DESTDIR)$(libdir)/$(REAL_LIBRARY))
> > 
> > Come to think of it, I'm not sure we should install the static library at
> > all.
> 
> Well I don't know if anyone uses the static library, I suppose its unlikely
> on linux, but I do wonder about windows? I don't really have much of a
> broader idea of what various other projects use/do with spidermonkey, I'm
> really just limited to the GNOME side.

I doubt anyone does "make install" on windows.
Comment 63 darkxst 2013-02-19 14:47:40 PST
Created attachment 715732 [details] [diff] [review]
fix MOZ_JS_LIBS to use correct library name (renamed variable)

renamed variable to JS_CONFIG_MOZ_JS_LIBS
Comment 64 darkxst 2013-02-19 15:28:26 PST
Created attachment 715758 [details] [diff] [review]
setup versioning and pkg-config support in build system v3a

Addresses Mark's comments, 
  - AC_DEFINE's are intended to allow embedders compile-time version checks
  - left the installation of static library for now, until confirmed we can remove it.
Comment 65 darkxst 2013-02-19 20:48:20 PST
Created attachment 715868 [details] [diff] [review]
add versioning to js-config
Comment 66 Mike Hommey [:glandium] 2013-02-25 02:26:39 PST
Comment on attachment 715758 [details] [diff] [review]
setup versioning and pkg-config support in build system v3a

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

r=me with these fixed.

::: js/src/Makefile.in
@@ +829,5 @@
>  # avoid trying to re-compute all that in the configure script, we just
>  # have the configure script generate this Makefile, and then invoke
>  # this rule.
> +
> +# set the various definitions that will be subsituted for js-config

substituted

@@ +856,5 @@
>  
>  SCRIPTS = js-config
>  SDK_BINARY = js-config
>  
> +$(LIBRARY_NAME).pc: js.pc.in js-config

Please remove the dependency on js-config
Comment 67 Mike Hommey [:glandium] 2013-02-25 02:33:39 PST
Comment on attachment 715868 [details] [diff] [review]
add versioning to js-config

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

Resetting r? pending answer about the make -C shell change.

::: js/src/Makefile.in
@@ +858,3 @@
>  	$(RM) $@.tmp
>  	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(JS_CONFIG_SUBSTITUTIONS) $< > $@.tmp \
> +	&& mv $(JS_CONFIG_NAME).tmp $@ && chmod +x $@

$@.tmp

@@ +864,2 @@
>  
> +$(LIBRARY_NAME).pc: js.pc.in $(JS_CONFIG_NAME)

No dependency on $(JS_CONFIG_NAME) here.

@@ +946,4 @@
>  ifneq (,$(IMPORT_LIBRARY))
>  	$(SYSINSTALL) $(IMPORT_LIBRARY) $(DESTDIR)$(libdir)
>  endif
> +	$(MAKE) -C shell

Why change this?
Comment 68 darkxst 2013-02-25 02:46:02 PST
(In reply to Mike Hommey [:glandium] from comment #67)
> Comment on attachment 715868 [details] [diff] [review]

> 
> @@ +946,4 @@
> >  ifneq (,$(IMPORT_LIBRARY))
> >  	$(SYSINSTALL) $(IMPORT_LIBRARY) $(DESTDIR)$(libdir)
> >  endif
> > +	$(MAKE) -C shell
> 
> Why change this?

Oops forget to mention this change, I discussed this on IRC with Waldo and Sstangl and it was suggested that rather than adding a version number to the js shell, we simply should not install it. Since in their view there is no reason to have it installed.
Comment 69 Landry Breuil (:gaston) 2013-02-25 02:48:25 PST
(In reply to darkxst from comment #68)
> (In reply to Mike Hommey [:glandium] from comment #67)
> > Comment on attachment 715868 [details] [diff] [review]
> 
> > 
> > @@ +946,4 @@
> > >  ifneq (,$(IMPORT_LIBRARY))
> > >  	$(SYSINSTALL) $(IMPORT_LIBRARY) $(DESTDIR)$(libdir)
> > >  endif
> > > +	$(MAKE) -C shell
> > 
> > Why change this?
> 
> Oops forget to mention this change, I discussed this on IRC with Waldo and
> Sstangl and it was suggested that rather than adding a version number to the
> js shell, we simply should not install it. Since in their view there is no
> reason to have it installed.

Hmm id have to disagree here - pretty sure some third-party rely on it to execute or parse javascript from a shell. Not installing it would be a regression..
Comment 70 Mike Hommey [:glandium] 2013-02-25 02:58:35 PST
Like Landry says. There is a definite need for a lightweight js shell that doesn't have all the dangerous debugging bits the js shell has, but I don't agree not installing the js shell is an option here.
Comment 71 darkxst 2013-02-25 03:02:33 PST
ok, so I will patch it to install it as 'js17' etc?
Comment 72 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-25 12:43:12 PST
In the short run, sure.  In the long run, we need to do something so people who want a barebones shell have it, without relying on something that's purely a SpiderMonkey repl for testing purposes.  I don't have much idea what that would be, exactly, or what needs it would have to fill, exactly.
Comment 73 darkxst 2013-02-25 13:21:39 PST
Created attachment 718044 [details] [diff] [review]
setup versioning and pkg-config support in build system v3b

Addresses the two minor issues in glandium's review
Comment 74 darkxst 2013-02-25 13:25:10 PST
Created attachment 718048 [details] [diff] [review]
add versioning to js and js-config

add versioning to the js shell so its installed to 'js17'
Comment 75 Mike Hommey [:glandium] 2013-02-26 01:01:56 PST
Comment on attachment 718048 [details] [diff] [review]
add versioning to js and js-config

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

::: js/src/configure.in
@@ +233,4 @@
>  MOZJS_MINOR_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.\([0-9]*\).*|\1|"`
>  MOZJS_PATCH_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.[0-9]*[^0-9]*||"`
>  IS_ALPHA=`echo $MOZILLA_VERSION | grep [ab]`
> +JS_CONFIG_NAME=js$MOZJS_MAJOR_VERSION-config

Come to think of it, this and the js shell rename should be conditional to the build being standalone.

@@ +4396,3 @@
>  # Produce the js-config script at configure time; see the comments for
>  # 'js-config' in Makefile.in.
>  AC_MSG_RESULT(invoking $MAKE to create js-config script)

You may want to replace js-config here too.

::: js/src/shell/Makefile.in
@@ +11,4 @@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
> +PROGRAM         = js$(MOZJS_MAJOR_VERSION)$(BIN_SUFFIX)

I'm pretty sure there are scripts using this program, so you'll have to fix them too.
Comment 76 darkxst 2013-02-26 01:48:17 PST
Created attachment 718327 [details] [diff] [review]
add versioning to js and js-config

Versioned js/js-config now conditional on JS_STANDALONE

This leaves one hardcoded reference to js$(bin_suffix), but its outside of the 'js/src/' tree so should not affect standalone build.
Comment 77 Mike Hommey [:glandium] 2013-02-26 01:51:44 PST
Comment on attachment 718327 [details] [diff] [review]
add versioning to js and js-config

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

Looks like you attached the wrong patch. It looks like the previous one.
Comment 78 darkxst 2013-02-26 02:45:42 PST
Created attachment 718346 [details] [diff] [review]
add versioning to js and js-config

oops, sorry about that. This is the right patch this time!
Comment 79 Mike Hommey [:glandium] 2013-02-26 02:53:58 PST
Comment on attachment 718346 [details] [diff] [review]
add versioning to js and js-config

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

r=me with the following changes:

::: js/src/configure.in
@@ +233,5 @@
>  MOZJS_MINOR_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.\([0-9]*\).*|\1|"`
>  MOZJS_PATCH_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.[0-9]*[^0-9]*||"`
>  IS_ALPHA=`echo $MOZILLA_VERSION | grep [ab]`
> +
> +if test "$JS_STANDALONE"; then

test -n

@@ +234,5 @@
>  MOZJS_PATCH_VERSION=`echo $MOZILLA_VERSION | sed "s|^[0-9]*\.[0-9]*[^0-9]*||"`
>  IS_ALPHA=`echo $MOZILLA_VERSION | grep [ab]`
> +
> +if test "$JS_STANDALONE"; then
> +JS_SHELL_NAME=js$MOZJS_MAJOR_VERSION$BIN_SUFFIX

At some point in the future, we'll likely change the build system to handle the BIN_SUFFIX part of PROGRAM definitions, so for future-proofing, it would be better not to make BIN_SUFFIX part of this variable.
Comment 80 darkxst 2013-02-26 03:05:07 PST
Created attachment 718352 [details] [diff] [review]
add versioning to js and js-config

Address remaining minor issues from glandium's review
Comment 81 Sean Stangl [:sstangl] 2013-04-23 17:36:40 PDT
In attempting to rebase the patches in this bug to m-c, the JS_CONFIG_SUBSTITUTIONS line now causes either inaction or a crash in config/Preprocessor.py:

- Executing configure calls Preprocessor.py with the following options, which do not replace any of the @VARIABLES@ in the js-config.h header:

/home/sstangl/dev/mozilla-inbound/js/src/dbg64/_virtualenv/bin/python ../config/Preprocessor.py -Dprefix="/usr/local" -Dexec_prefix="/usr/local" -Dincludedir="/usr/local/include" -Dlibdir="/usr/local/lib" -DMOZILLA_VERSION="23.0a1" -DLIBRARY_NAME="mozjs-23.0a" -DJS_CONFIG_LIBS=" -ldl  -lm -lz -ldl" -DJS_CONFIG_MOZ_JS_LIBS="-L/usr/local/lib -lmozjs-23.0a" -DMOZJS_MAJOR_VERSION="23" -DMOZJS_MINOR_VERSION="0" -DMOZJS_PATCH_VERSION="1" -DMOZJS_ALPHA="a" -DNSPR_CFLAGS="" -DNSPR_PKGCONF_CHECK="nspr" ../js-config.in > js23-config.tmp \
&& mv js23-config.tmp js23-config && chmod +x js23-config
../js-config.in: WARNING: no useful preprocessor directives found


- Manually executing the other invocation of Preprocessor.py via `make mozjs-23.0a.pc` causes the following crash:

/home/sstangl/dev/mozilla-inbound/js/src/dbg64/_virtualenv/bin/python ../config/Preprocessor.py -Dprefix="/usr/local" -Dexec_prefix="/usr/local" -Dincludedir="/usr/local/include" -Dlibdir="/usr/local/lib" -DMOZILLA_VERSION="23.0a1" -DLIBRARY_NAME="mozjs-23.0a" -DJS_CONFIG_LIBS=" -ldl  -lm -lz -ldl" -DJS_CONFIG_MOZ_JS_LIBS="-L/usr/local/lib -lmozjs-23.0a" -DMOZJS_MAJOR_VERSION="23" -DMOZJS_MINOR_VERSION="0" -DMOZJS_PATCH_VERSION="1" -DMOZJS_ALPHA="a" -DNSPR_CFLAGS="" -DNSPR_PKGCONF_CHECK="nspr" ../js.pc.in > mozjs-23.0a.pc
Traceback (most recent call last):
  File "../config/Preprocessor.py", line 492, in <module>
    main()
  File "../config/Preprocessor.py", line 477, in main
    pp.handleCommandLine(None, True)
  File "../config/Preprocessor.py", line 169, in handleCommandLine
    self.do_include(f, False)
  File "../config/Preprocessor.py", line 462, in do_include
    self.handleLine(l)
  File "../config/Preprocessor.py", line 241, in handleLine
    self.write(aLine)
  File "../config/Preprocessor.py", line 139, in write
    filteredLine = self.applyFilters(aLine)
  File "../config/Preprocessor.py", line 124, in applyFilters
    aLine = f[1](aLine)
  File "../config/Preprocessor.py", line 420, in filter_substitution
    return self.varsubst.sub(repl, aLine)
  File "../config/Preprocessor.py", line 418, in repl
    raise Preprocessor.Error(self, 'UNDEFINED_VAR', varname)
__main__.Error: ('/home/sstangl/dev/mozilla-inbound/js/src/js.pc.in', 11, 'UNDEFINED_VAR', 'MODULE')
make: *** [mozjs-23.0a.pc] Error 1
make: *** Deleting file `mozjs-23.0a.pc


Given that it's crashing now, and that this is the same code that worked on ESR17, and that there were a number of changes to Preprocessor.py recently (none of which looked suspicious to me) -- what would be the best way to perform a @VARIABLE@ substitution?
Comment 82 Sean Stangl [:sstangl] 2013-04-23 17:38:47 PDT
Created attachment 741103 [details] [diff] [review]
Full versioning patch, for debugging.

This is the full blob of all rebased patches from this bug. Applies to m-i.
Comment 83 Mike Hommey [:glandium] 2013-04-23 23:01:51 PDT
> what would be the best way to perform a @VARIABLE@ substitution?

Either add #filter substitution in the file, or add -Fsubstitution on the command line, but either way, there's another problem: lines beginning with # (including #!/bin/sh) are being removed, so you also need "--marker %" (which makes #filter become %filter)
Comment 84 Sean Stangl [:sstangl] 2013-04-24 15:29:15 PDT
Created attachment 741550 [details] [diff] [review]
[Part 1/4] Remove JS_VERSION.

Posting rebased patches for review, in an attempt to clean up and finally drive this bug to completion. When eventually pushing, I'll preserve the same authorship from the original patches into the rebased versions.

Part 1 is a slightly rebased version of the original patch from Comment 11. Does not touch the build system, and not much has changed, so carrying dmandelin's r+ from before the rebase.
Comment 85 Sean Stangl [:sstangl] 2013-04-24 15:31:07 PDT
Created attachment 741554 [details] [diff] [review]
[Part 2/4] Setup versioning and pkg-config support.

Rebased version of patch from Comment 73.
Comment 86 Sean Stangl [:sstangl] 2013-04-24 15:33:20 PDT
Created attachment 741555 [details] [diff] [review]
[Part 3/4] Use version number in shell and js-config filenames.

Rebased version of patch from Comment 80.
Comment 87 Sean Stangl [:sstangl] 2013-04-24 15:34:57 PDT
Created attachment 741556 [details] [diff] [review]
[Part 4/4] Fix MOZ_JS_LIBS

Rebased version of patch from Comment 63.
Comment 88 Mike Hommey [:glandium] 2013-04-25 01:39:14 PDT
Comment on attachment 741554 [details] [diff] [review]
[Part 2/4] Setup versioning and pkg-config support.

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

r=me with the following fixed:

::: js/src/js-config.in
@@ +108,4 @@
>  fi
>  
>  if test "$echo_cflags" = "yes"; then
> +    echo "-I$includedir/$MODULE/js/RequiredDefines.h -I$includedir/$MODULE $NSPR_CFLAGS"

-include $includedir/$MODULE/js/RequiredDefines.h
Comment 89 Mike Hommey [:glandium] 2013-04-25 01:42:04 PDT
Comment on attachment 741555 [details] [diff] [review]
[Part 3/4] Use version number in shell and js-config filenames.

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

::: js/src/configure.in
@@ +4432,4 @@
>  AC_OUTPUT([js-confdefs.h Makefile config/autoconf.mk config/emptyvars.mk])
>  
>  # Produce the js-config script at configure time; see the comments for
> +# 'js-*config' in Makefile.in.

js*-config
Comment 90 Mike Hommey [:glandium] 2013-04-25 01:43:50 PDT
Comment on attachment 741556 [details] [diff] [review]
[Part 4/4] Fix MOZ_JS_LIBS

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

::: js/src/js-config.in
@@ +14,2 @@
>  
> +FILENAME=`basename "$0"`

This line should be changed in patch 3/4 instead of here.
Comment 92 Sean Stangl [:sstangl] 2013-04-25 18:25:59 PDT
Created attachment 742126 [details] [diff] [review]
Follow-up: Temporarily comment out standalone JS executable renaming.

Apparently renaming the JS shell binary breaks workflows, so it has been requested that I back out the part that causes renaming until we figure out a robust way to let everyone keep typing "js".

I'd like to solve this via symlinks, with the actual executable named "js$MOZJS_MAJOR_VERSION", but it has also been suggested that we just always manually make the naming change when generating a standalone release.

Bikeshedding welcomed. That was not sarcasm.
Comment 94 Mike Hommey [:glandium] 2013-04-25 22:52:19 PDT
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.

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