Ian Stakenvicius Assignee
	Description • 12 years ago

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.

	Ian Stakenvicius Assignee
	Comment 1 • 12 years ago

bug 479475 might be easier with this, too, possibly

	Ian Stakenvicius Assignee
	Comment 2 • 12 years ago

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

	David Mandelin [:dmandelin]
	Comment 3 • 12 years ago

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.

	David Mandelin [:dmandelin]
	Comment 4 • 12 years ago

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.

	Ian Stakenvicius Assignee
	Comment 5 • 12 years ago

(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.

	Jeff Walden [:Waldo]
	Comment 6 • 12 years ago

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.

	Ian Stakenvicius Assignee
	Comment 7 • 12 years ago

... 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.

	Wesley W. Garland
	Comment 8 • 12 years ago

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.

	Ian Stakenvicius Assignee
	Comment 9 • 12 years ago

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

	Ian Stakenvicius Assignee
	Comment 10 • 12 years ago

This one's the target change we want to get to eventually, that drops all the deprecated JS_VERSION stuffs

	Ian Stakenvicius Assignee
	Comment 11 • 12 years ago

oops, wrong patch; this one contains all changes

	Mike Hommey [:glandium]
	Comment 12 • 12 years ago

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.

	Ian Stakenvicius Assignee
	Comment 13 • 12 years ago

(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.

	Mike Hommey [:glandium]
	Comment 14 • 12 years ago

(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)))\
   )\
 )

	Ian Stakenvicius Assignee
	Comment 15 • 12 years ago

(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?

	Ian Stakenvicius Assignee
	Comment 16 • 12 years ago

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.

	(not currently active) Ted Mielczarek
	Comment 17 • 12 years ago

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.

	Ian Stakenvicius Assignee
	Comment 18 • 12 years ago

(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

	(not currently active) Ted Mielczarek
	Comment 19 • 12 years ago

(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.

	Ian Stakenvicius Assignee
	Comment 20 • 12 years ago

(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?

	darkxst
	Comment 21 • 12 years ago

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

	(not currently active) Ted Mielczarek
	Comment 22 • 12 years ago

(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).

	Ian Stakenvicius Assignee
	Comment 23 • 12 years ago

(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?

	Ian Stakenvicius Assignee
	Comment 24 • 12 years ago

(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!

	(not currently active) Ted Mielczarek
	Comment 25 • 12 years ago

(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.

	darkxst
	Comment 26 • 12 years ago

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.

	darkxst
	Comment 27 • 12 years ago

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.

	darkxst
	Comment 28 • 12 years ago

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'

	darkxst
	Comment 29 • 12 years ago

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.

	darkxst
	Comment 30 • 12 years ago

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?

	Mike Hommey [:glandium]
	Comment 31 • 12 years ago

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.

	darkxst
	Comment 32 • 12 years ago

is there something better to use? Do the MOZ_APP_* macros suffer the same problem?

	Ian Stakenvicius Assignee
	Comment 33 • 12 years ago

(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).

	darkxst
	Comment 34 • 12 years ago

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?

	(not currently active) Ted Mielczarek
	Comment 35 • 11 years ago

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.

	(not currently active) Ted Mielczarek
	Comment 36 • 11 years ago

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.

	Mike Hommey [:glandium]
	Comment 37 • 11 years ago

(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.

	(not currently active) Ted Mielczarek
	Comment 38 • 11 years ago

Okay, I will defer to your expertise.

	darkxst
	Comment 39 • 11 years ago

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?

	darkxst
	Comment 40 • 11 years ago

(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.

	darkxst
	Comment 41 • 11 years ago

I end up with a leftover ".mkdir.done" timestamp installed on the system, when using the generic install target to install the pkgconfig file.

	Landry Breuil (:gaston)
	Comment 42 • 11 years ago

(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.

	darkxst
	Comment 43 • 11 years ago

There where a couple of instances where the static library name was hard coded into sub-Makefiles. This update fixes that.

	darkxst
	Comment 44 • 11 years ago

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

	Mike Hommey [:glandium]
	Comment 45 • 11 years ago

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.

	Mike Hommey [:glandium]
	Comment 46 • 11 years ago

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

	Mike Hommey [:glandium]
	Comment 47 • 11 years ago

Ian, are you working on updating your patch with Ted's comments?

	darkxst
	Comment 48 • 11 years ago

(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?

	Mike Hommey [:glandium]
	Comment 49 • 11 years ago

(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.

	darkxst
	Comment 50 • 11 years ago

(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?

	darkxst
	Comment 51 • 11 years ago

(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.

	Mike Hommey [:glandium]
	Comment 52 • 11 years ago

(In reply to darkxst from comment #50)
> I thought you wanted to keep the soname versioning stuff?

Ian's patch adds soname versioning.

	darkxst
	Comment 54 • 11 years ago

This is required or else js-config will be broken due to hardcoded library names in MOZ_JS_LIBS.

	darkxst
	Comment 55 • 11 years ago

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

This should be included in Ian's patch.

	Jason Orendorff [:jorendorff]
	Comment 56 • 11 years ago

Are the four latest patches in this bug ready for review?

	darkxst
	Comment 57 • 11 years ago

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

	Jeff Walden [:Waldo]
	Comment 58 • 11 years ago

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.

	Mike Hommey [:glandium]
	Comment 59 • 11 years ago

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.

	Mike Hommey [:glandium]
	Comment 60 • 11 years ago

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 ?

	darkxst
	Comment 61 • 11 years ago

(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.

	Mike Hommey [:glandium]
	Comment 62 • 11 years ago

(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.

	darkxst
	Comment 63 • 11 years ago

renamed variable to JS_CONFIG_MOZ_JS_LIBS

	darkxst
	Comment 64 • 11 years ago

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.

	Mike Hommey [:glandium]
	Comment 66 • 11 years ago

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

	Mike Hommey [:glandium]
	Comment 67 • 11 years ago

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?

	darkxst
	Comment 68 • 11 years ago

(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.

	Landry Breuil (:gaston)
	Comment 69 • 11 years ago

(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..

	Mike Hommey [:glandium]
	Comment 70 • 11 years ago

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.

	darkxst
	Comment 71 • 11 years ago

ok, so I will patch it to install it as 'js17' etc?

	Jeff Walden [:Waldo]
	Comment 72 • 11 years ago

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.

	darkxst
	Comment 73 • 11 years ago

Addresses the two minor issues in glandium's review

	darkxst
	Comment 74 • 11 years ago

add versioning to the js shell so its installed to 'js17'

	Mike Hommey [:glandium]
	Comment 75 • 11 years ago

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.

	darkxst
	Comment 76 • 11 years ago

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.

	Mike Hommey [:glandium]
	Comment 77 • 11 years ago

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.

	darkxst
	Comment 78 • 11 years ago

oops, sorry about that. This is the right patch this time!

	Mike Hommey [:glandium]
	Comment 79 • 11 years ago

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.

	darkxst
	Comment 80 • 11 years ago

Address remaining minor issues from glandium's review

	Sean Stangl [:sstangl]
	Comment 81 • 11 years ago

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?

	Sean Stangl [:sstangl]
	Comment 82 • 11 years ago

This is the full blob of all rebased patches from this bug. Applies to m-i.

	Mike Hommey [:glandium]
	Comment 83 • 11 years ago

> 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)

	Sean Stangl [:sstangl]
	Comment 84 • 11 years ago

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.

	Sean Stangl [:sstangl]
	Comment 85 • 11 years ago

Rebased version of patch from Comment 73.

	Sean Stangl [:sstangl]
	Comment 86 • 11 years ago

Rebased version of patch from Comment 80.

	Sean Stangl [:sstangl]
	Comment 87 • 11 years ago

Rebased version of patch from Comment 63.

	Mike Hommey [:glandium]
	Comment 88 • 11 years ago

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

	Mike Hommey [:glandium]
	Comment 89 • 11 years ago

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

	Mike Hommey [:glandium]
	Comment 90 • 11 years ago

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.

	Sean Stangl [:sstangl]
	Comment 91 • 11 years ago

Thanks for the quick review! Passed a Tryserver run (https://tbpl.mozilla.org/?tree=Try&rev=3fa26db41242).

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6065e3b272
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba7b9ee7c0b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4548f5b02618
https://hg.mozilla.org/integration/mozilla-inbound/rev/50878806f6f1

	Sean Stangl [:sstangl]
	Comment 92 • 11 years ago

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.

	Sean Stangl [:sstangl]
	Comment 93 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/63f3e64b064a

	Mike Hommey [:glandium]
	Comment 94 • 11 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 95 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/8f6065e3b272
https://hg.mozilla.org/mozilla-central/rev/ba7b9ee7c0b8
https://hg.mozilla.org/mozilla-central/rev/4548f5b02618
https://hg.mozilla.org/mozilla-central/rev/50878806f6f1
https://hg.mozilla.org/mozilla-central/rev/63f3e64b064a