Closed Bug 517765 Opened 11 years ago Closed 7 years ago

Source code management for standalone Spidermonkey release

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 --- fixed
firefox25 --- wontfix
firefox26 --- fixed

People

(Reporter: wes, Unassigned)

References

Details

Attachments

(1 file, 5 obsolete files)

Ash Berlin and I are currently preparing a second release candidate for JavaScript 1.8.0, and intend to continue to produce standalone-js releases targetted at the JS embedder community.  The 1.8.0 release candidate we're working on now is  intended as the "end of the line" for the CVS releases, old build system, etc.  Next release would track Firefox 3.5.x out of a mercurial repository.

The release procedure at http://www.mozilla.org/js/spidermonkey/release-notes/spidermonkey-releases.html talks about CVS tagging -- additionally there are patches in bugs we'd like to take and commit to the repo.

How should we do this? Is it possible for us to have commit access and create a standalone-js branch?  There is at least one patch we are considering right now which the browser would definitely not want.  Perhaps access to a repository cloned with "cp -r" ? Or should we be looking to work through a proxy (e.g. bc or jorendorff)?

Additionally, what source code review/landing procedure applies to this stuff, particularly with respect to patches that the browser people won't want/need/nptob? I would be happy with "r+ from js peer, push yourself" if we had access to an appropriate CVS repo. 

When back-porting, such as the case with Ash's back port of bug 419901, should we r? the original patch author (provided he is a js peer) or the original reviewer?

Finally, is there anybody who wants to "bless" what goes into a release? We're not planning anything crazy, just looking to see if somebody wants to volunteer to be our manager.

Thanks,
Wes
brendan: ^^^
It's best to catch up to TM and not backport selectively (very selectively if you don't backport much -- ton of fixes over the last couple of years). Otherwise I bet you end up sliding downslope toward more, bigger, and untenable backports.

Suggest you make an hg repo off of cvs.m.o and use it, and only for js/src/*, unless you intend to test your changes against Firefox and other builds from cvs.m.o or a clone of same.

I don't see using CVS as worth it if you don't already have access, unless you really are using it over the longer term. Which is a plan I don't endorse.

Whose reputation is on the line that might require an r+ from a JS peer? Peers have hands full with current-tree concerns. Again I'm worried about scope creep.

/be
Most of the important fixes over the last couple of years have already been back-ported and landed for us, thanks to jorendorff's and bc's efforts for RC1. 

I have no intention of testing 1.8.0 changes against Firefox; there doesn't seem to be much of a point and I really lack expertise there anyhow. Will take your advice and import CVS->hg somehow; hg can be cloned to its final home whenever that comes.

Scope creep is also something I worry about.  My "bugs to land" list is exactly the same as the current blockers list in js1.8src, bug 428420. Note that while those are all "closed" (but one), some haven't landed in CVS, which is where  the effort lies. Examples include that multiple context corruption bug, maxargs in JS_FN, etc.  Most of these appear to be reviewed and apply without change; they just didn't land.

The one unclosed bug is Bug 419537 - Array thread-safety restoration.  The "right" solution IMHO has already been suggested: slowification upon MT access. I think that really is out of scope given time & knowledge constraints, particularly as slowification itself apparently has MT issues.  

On the other hand, I want to be able to provide a js 1.8 source release where js 1.7 embedders can upgrade -- I know at least a couple who are stuck on 1.7 because they are running MT JS. I know this isn't the place for SpiderMonkey evangelism, but from the embedder market POV, jsapi is falling by the wayside as its most distinguishing feature -- thread safety -- is eroded.  MT JS and JS in an MT environment are frequently confused, exacerbating the issue.  I also see this time in history as an important locus: for the first time there is an organized effort to create cross-platform JS in a place other than the browser (CommonJS nee ServerJS). Momentum behind Spidermonkey would be a Good Thing.  This is a key reason why Ash & I (independent embedders working on different CommonJS/SpiderMonkey implementations) want to help re-invigorate the Spidermonkey source release effort.

Catching up to TM is also important -- we want to finish the 1.8.0 release ASAP, it has been in limbo for a long time -- and then swiftly move on to tracking TM with regular releases and ABI stability across library point revs. (Solving things like the Ubuntu bug from a couple of months back without impacting mainline).

Back on topic -- It occurs to me that 419537 could be worked around for the MT JS folks by disabling dense arrays with a makefile switch.  I think THAT is doable within our constraints, but input from somebody with knowledge of the array implementation would be useful from a QA point of view.

Which brings me to your reputation question: "Whose reputation is on the line that might require an r+ from a JS peer?" -- if Moz wants to take our 1.8.0 release, then maybe yours.  Or maybe it would be more advantageous to Mozilla to completely spin-off Standalone-JS tarball releases to a "community effort", I don't really know the answer to that. I DO know that the current state of affairs -- no release in 23 months, and nobody@moz with the time to finish one -- is not healthy.

Wes
Probably we should fix bug 419537 (one of you on the cc: list could do it) and then back-port. It probably will port easily.

Multi-threaded mostly-shared-mutable-heap architectures are kinda '90s and not in favor now, which may be why SpiderMonkey's advantage there (no scare quotes but I was tempted) is muted.

We do need to produce standalone SpiderMonkey, ideally when we produce Firefox releases. It shouldn't be so delayed. Let's catch up.

/be
Current source repository is mirrored publically on Google code; source browser URL: http://code.google.com/p/js18/source/list

hg clone https://js18.googlecode.com/hg/ js18
Right, so a few years pass and it seems things are still more or less as they were.  This bug seems like a good one to use for a tracker to get the necessary bits in place for regular spidermonkey releases.

A bit of notes i'm sure everyone knows -- mozilla-central is now releasing new major versions regularly; releasing spidermonkeys with the same versioning scheme as FF, etc for at least the ESR versions is the plan.  As of now, this means the next release would be spidermonkey-17.0 ESR.

What is needed, is that the codebase in mozilla-central contains all the necessary bits to support a standalone source release.  Step #1 is to make versioning be handled by the build system in accordance with milestone.txt rather than by external patches -- bug 812265 is for that.

Also needed is confirmation that all headers will be properly installed, ie that nothing is missing from the public headers, and no headers include any headers that aren't installed.  There were some issues oustanding on this with the attempt at a spidermonkey-1.8.7 release (bug 735599).  no bug has been filed for this yet.

Although not specific to the release itself, the library needs to have proper versioning itself, not just the project or filename.  Bug 809430 describes the issue and provides fixes for linux; similarly bug 520849 requests version info for the windows dll.

Finally , bug 479475 probably needs to be implemented to actually roll out the source releases.

Anything I missed, please add.
This patch adds a 'source-package' target to js/src/Makefile.in which allows building of release-ready tarballs directly out of the codebase.

- it works in-sourcetree as well as out-of-sourcetree
- it works from the srcdir unpacked from a previously-generated tarball
- it requires tar (check added to configure.in) to limit generation to environments where tar is available (ie linux/unix)
- it generates README and INSTALL if none are found in $(DIST)
- it copies LICENSE from m-c/b2g/ if exists, otherwise re-uses existing
- it copies virtualenv and all necessary bits from m-c so that ./configure runs
- it copies the embedded icu
Attachment #768497 - Flags: review?(sstangl)
oops; a minor bit of cruft from another patch i'm submitting. This one should be clean.
Attachment #768497 - Attachment is obsolete: true
Attachment #768497 - Flags: review?(sstangl)
Attachment #768501 - Flags: review?(sstangl)
Comment on attachment 768501 [details] [diff] [review]
add 'make source-package' support to js/src/Makefile

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

Thanks for working on automating packaging, and sorry for the long review wait.

The patch looks good, but I'm wary about doing all the work in Makefile.in. Do you think it would be possible to keep the 'source-package' target in the Makefile, but place the packaging logic in an accompanying shell script?
Attachment #768501 - Flags: review?(sstangl)
Sorry for the delay in response.

Yes, it'd definitely be doable to roll the actions into a shell script.  I'll try and provide a new version later today.

(I did a test roll yesterday of mozjs-24.0_beta and have at least one other bug to report, too, before the official release)
Split out the source-package functionality into a shell script, as requested.  Script has two targets -- 'build' and 'clean' so that Makefile targets 'source-package' and 'clean' are both handled.

I also added a bunch of stuff to DIST_GARBAGE to ensure an appropriately clean tarball can be rolled if running 'make source-package' in-sourcetree.

Notes:
- Tarball contents are identical whether 'make source-package' is run in-sourcetree or out-of-sourcetree
- Tarball contents are duplicated when 'make source-package' is run from the tree of an extracted tarball.
Attachment #790882 - Flags: review?
Attachment #790882 - Flags: review? → review?(sstangl)
Attachment #768501 - Attachment is obsolete: true
oops - the check for TAR in configure.in was skipped in the last patch.  This one is complete.
Attachment #790882 - Attachment is obsolete: true
Attachment #790882 - Flags: review?(sstangl)
Attachment #790924 - Flags: review?(sstangl)
Comment on attachment 790924 [details] [diff] [review]
add 'make source-package' support to js/src/

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

This looks great and appears to work just fine. Thanks for taking this on.

::: js/src/Makefile.in
@@ +353,5 @@
> +   unallmakefiles $(JS_CONFIG_NAME) js-config.h js-confdefs.h \
> +   backend.mk config/backend.mk devtools/backend.mk editline/backend.mk \
> +   gdb/backend.mk jsapi-tests/backend.mk shell/backend.mk tests/backend.mk \
> +   backend.RecursiveMakeBackend.built backend.RecursiveMakeBackend.built.pp \
> +   devtools/rootAnalysis/Makefile 

nit: trailing whitespace

::: js/src/make-source-package.sh
@@ +1,3 @@
> +#!/bin/sh
> +
> +# need environment vars: 

nit: there is a bunch of trailing whitespace in this file.
Attachment #790924 - Flags: review?(sstangl) → review+
nit-free version that also fixed a small edge-case error.  Going to push this to try just to make sure it clears.
Attachment #790924 - Attachment is obsolete: true
Attachment #792957 - Flags: review+
Comment on attachment 792957 [details] [diff] [review]
add 'make source-package' support into js/src , nit-free

https://tbpl.mozilla.org/?tree=Try&rev=e17956b11a25 is green.  Please checkin!  

Also this will help a lot to roll spidermonkey-24 out of ESR, so please also consider approval for beta:

[Approval Request Comment]
Bug caused by missing feature in build system 
User impact if declined: manual backport of patch to roll spidermonkey-24.
Testing completed (on m-c, etc.): see above
Risk to taking this patch (and alternatives if risky): nil, applies clean to beta
String or IDL/UUID changes made by this patch:
Attachment #792957 - Flags: checkin?
Attachment #792957 - Flags: approval-mozilla-beta?
Comment on attachment 792957 [details] [diff] [review]
add 'make source-package' support into js/src , nit-free

In the future, please just use checkin-needed if the bug only has one patch.
Attachment #792957 - Flags: checkin? → checkin+
Hmm...  the additions to DIST_GARBAGE and clean:: need to be added only when JS_STANDALONE, looks like...

Should I have done something different when I ran my try test, to catch this issue earlier?
Nope -- issue is the format of the patch, s.t. it didn't keep the execute perms when it was applied.  Here's a git-style patch of the same that should apply properly and not trigger the error hit earlier.

Will do a PGO run on try and if all clear I'll mark checkin-needed again.
Attachment #792957 - Attachment is obsolete: true
Attachment #792957 - Flags: approval-mozilla-beta?
Attachment #794101 - Flags: review+
Attachment #794101 - Flags: approval-mozilla-beta?
Comment on attachment 794101 [details] [diff] [review]
add 'make source-package' support into js/src , nit-free , git-style diff

PGO try build is green:  https://tbpl.mozilla.org/?tree=Try&rev=adf71ec09d2f

Please proceed with checkin again and beta approval
Attachment #794101 - Flags: checkin+
Comment on attachment 794101 [details] [diff] [review]
add 'make source-package' support into js/src , nit-free , git-style diff

"checkin+" means "is checked in". Use the "checkin-needed" keyword on the bug if you need a single r+'d patch landed.
Attachment #794101 - Flags: checkin+
You broke the tree rules by not getting build peer review on configure and make file changes. That being said, I'm inclined to let it slide, despite there being a few nits in the patch, such as listing backend.mk files in DIST_GARBAGE (the build system should do that for you).
For some reason, make-source-package.sh was being marked as not executable every time I pushed, no matter how many different ways I attempted to fix it. Re-landed with Ms2ger's help.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2084a87e4853
https://hg.mozilla.org/integration/mozilla-inbound/rev/d84b8ecb416c
https://hg.mozilla.org/mozilla-central/rev/2084a87e4853
https://hg.mozilla.org/mozilla-central/rev/d84b8ecb416c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(In reply to Ian Stakenvicius from comment #15)
> Comment on attachment 792957 [details] [diff] [review]
> add 'make source-package' support into js/src , nit-free
> 
> https://tbpl.mozilla.org/?tree=Try&rev=e17956b11a25 is green.  Please
> checkin!  
> 
> Also this will help a lot to roll spidermonkey-24 out of ESR, 

Can you please help expand on this ? Its unclear to me how this uplift will have any impact on existing ESR users or is a blocker for them.

Also we prefer such changes ride the trains or land very early in the cycle.

>so please also
> consider approval for beta:
> 
> [Approval Request Comment]
> Bug caused by missing feature in build system 
> User impact if declined: manual backport of patch to roll spidermonkey-24.
> Testing completed (on m-c, etc.): see above
> Risk to taking this patch (and alternatives if risky): nil, applies clean to
> beta
> String or IDL/UUID changes made by this patch:
Attachment #794101 - Flags: approval-mozilla-beta?
Cleared the beta nomination until we have more info as requested in comment#28.Please feel free to renominate if needed based on the additional info.
Comment on attachment 794101 [details] [diff] [review]
add 'make source-package' support into js/src , nit-free , git-style diff

(In reply to bhavana bajaj [:bajaj] from comment #28)
> (In reply to Ian Stakenvicius from comment #15)
> > Comment on attachment 792957 [details] [diff] [review]
> > add 'make source-package' support into js/src , nit-free
> > 
> > https://tbpl.mozilla.org/?tree=Try&rev=e17956b11a25 is green.  Please
> > checkin!  
> > 
> > Also this will help a lot to roll spidermonkey-24 out of ESR, 
> 
> Can you please help expand on this ? Its unclear to me how this uplift will
> have any impact on existing ESR users or is a blocker for them.
> 

It doesn't have an impact on ESR users at all, as nothing changes for them.  It affects the ability for spidermonkey to be released.

Spidermonkey's independent source releases are rolled out of the ESR codebase, and so instead of having to assemble that manually for spidermonkey-24 (to be rolled out of mozilla-24 ESR), this patch allows for it to be rolled directly in a semi-automated fashion via the build system.  It will help with the first release, and it will also help with any bugfix or seciruty releases that may need to be rolled out of later mozilla-ESR versions.

Technically, if there was an intention in maintaining spidermonkey-17, this patch should even be backported there; however we need to draw the line somewhere.


> Also we prefer such changes ride the trains or land very early in the cycle.

Makes sense, sorry for having to push it so late in the game.  It wasn't apparent to me that this was even necessary until I tried rolling a tarball out of the moz24 codebase the day it hit alpha -- I had thought the build system infrastructure for 'make source-package' would have been put in place for mozilla-17 but apparently it wasn't.


> > [Approval Request Comment]
> > Bug caused by missing feature in build system 
> > User impact if declined: manual backport of patch to roll spidermonkey-24.
> > Testing completed (on m-c, etc.): see above
> > Risk to taking this patch (and alternatives if risky): nil, applies clean to
> > beta
> > String or IDL/UUID changes made by this patch:
Attachment #794101 - Flags: approval-mozilla-beta?
Needinfo'ing :gps here to have some eyes and help understand on if this patch may be risky for a beta uplift at this stage ? I am slightly worried on if this may have an impact on build infra etc , any idea there ?

Also this landed on m-c and nominated for beta, is this not needed on Fx25 aurora ?
Flags: needinfo?(gps)
CC'ing a couple of releng folks as well for them to raise any red flags.
(In reply to bhavana bajaj [:bajaj] from comment #31)
> Also this landed on m-c and nominated for beta, is this not needed on Fx25
> aurora ?


Spidermonkey is only rolled out of ESR's, and Fx25 isn't going to be an ESR release is it?
I'm not sure what issues will arise with this late uplift.
There is potential for bustage in the next beta if this lands, but I can't say for sure one way or the other.
I think the risk is low. Any regression should be immediately noticeable via an obviously broken build.
Flags: needinfo?(gps)
Thanks ! I'll approve this for beta, we'll backout if any breakages happen.
Comment on attachment 794101 [details] [diff] [review]
add 'make source-package' support into js/src , nit-free , git-style diff

Please land asap on the beta branch so this can land into beta going to build tomorrow morning PT.

Also our QA will not have this on their radar for testing so please confirm the fix once we have the beta builds out that it fixes the problem and no new issues are found.

[RyanVm confirmed this is not needed on aurora as this one's mainly targeted for standalone js release that come off-of esr.]
Attachment #794101 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.