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.

bug 479475 might be easier with this, too, possibly

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

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

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.

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

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.

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

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

oops, wrong patch; this one contains all changes

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.

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

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

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

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

(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

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

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

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

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

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

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

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

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.

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

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.

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

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

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

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

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.

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

Okay, I will defer to your expertise.

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?

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

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

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

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

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

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

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

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

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

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

(In reply to darkxst from comment #50) > I thought you wanted to keep the soname versioning stuff? Ian's patch adds soname versioning.

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

Use versioned static library name to avoid any name conflicts with multiple spidermonkey versions installed. This should be included in Ian's patch.

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

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

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

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

renamed variable to JS_CONFIG_MOZ_JS_LIBS

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

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

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

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.

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

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.

Addresses the two minor issues in glandium's review

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

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.

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

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

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.

Address remaining minor issues from glandium's review

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?

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

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

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.

Rebased version of patch from Comment 73.

Rebased version of patch from Comment 80.

Rebased version of patch from Comment 63.

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

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

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.

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.

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