1273917 - Create a build job that generates SpiderMonkey tarballs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Next up is trying to actually build from it.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Create a build job that generates SpiderMonkey tarballs

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ha I started working on a patch yesterday, and was stumbling through taskcluster. Glad I don't have to anymore :)

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8753868 [details] [diff] [review]
Create a build job that generates SpiderMonkey tarballs

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

Thanks! Though fitzgen was having such a good time working on this...

I'd like to see another pass at this with the duplication removed, or after you hitting me with a clue stick to explain why it's necessary.

::: js/src/Makefile.in
@@ -244,5 @@
>  # Generating source package tarballs
>  # (only possible when tar is found)
>  ifneq (,$(TAR))
>  
>  source-package:

Heh. I wonder if anyone ever uses this target. I didn't know it existed.

::: js/src/make-source-package.sh
@@ +9,5 @@
>  : ${TAR:=tar}
>  : ${SRCDIR:=$(cd $(dirname $0); pwd 2>/dev/null)}
>  : ${MOZJS_NAME:=mozjs}
> +: ${STAGING:=/tmp/mozjs-src-pkg}
> +: ${DIST:=/tmp}

Since you seem to know what $DIST and $STAGING mean, any chance you could document them? I've never been entirely sure what goes in $DIST.

::: testing/taskcluster/scripts/builder/build-sm-tarball.sh
@@ +7,5 @@
> +: ${TOOLTOOL_REV:=master}
> +: ${SPIDERMONKEY_VARIANT:=plain}
> +: ${UPLOAD_DIR:=$HOME/artifacts/}
> +: ${WORK:=$HOME/workspace}
> +: ${SRCDIR:=$WORK/build/src}

Need to strip out some useless stuff (eg SPIDERMONKEY_VARIANT), but also see below.

@@ +42,5 @@
> +# Install everything needed for the browser on this platform. Not all of it is
> +# necessary for the JS shell, but it's less duplication to share tooltool
> +# manifests.
> +BROWSER_PLATFORM=$PLATFORM_OS$BITS
> +TOOLTOOL_MANIFEST="$SRCDIR/browser/config/tooltool-manifests/$BROWSER_PLATFORM/releng.manifest"

Oh, bleh. You're copying my hairball. I'd say it's really too ugly to exist even once, but given its necessity, I would at least humbly request that you extract this out into sm-tooltool-config.sh or something, so that at least there's a single copy of this crap.

@@ +62,5 @@
> +# Ensure upload dir exists
> +mkdir -p $UPLOAD_DIR
> +
> +# Run the script
> +DIST=$UPLOAD_DIR $SRCDIR/js/src/make-source-package.sh

Ah, ok. So $DIST is where the tarball is placed, and $STAGING is... well, a staging area, from which the tarball is constructed. That makes sense.

@@ +64,5 @@
> +
> +# Run the script
> +DIST=$UPLOAD_DIR $SRCDIR/js/src/make-source-package.sh
> +BUILD_STATUS=$?
> +exit $BUILD_STATUS

All this BUILD_STATUS goop is a long-winded way of exec'ing make-source-package.sh, but I guess it's safer for refactoring this way.

::: testing/taskcluster/tasks/branches/base_jobs.yml
@@ +172,5 @@
> +        task: tasks/builds/sm_tarball.yml
> +    when:
> +      file_patterns:
> +       - js/public/**
> +       - js/src/**

This is ok for now. But what I'd really like this job to do, or a new downstream job to do, is now unpack the tarball somewhere and build it, then link a test program against it (statically, dynamically, or perhaps both in succession). And in that case, I'd like to extend these patterns to include mfbt, mozglue, zlib, nspr, etc.

::: testing/taskcluster/tasks/builds/sm_tarball.yml
@@ +3,5 @@
> +  variables:
> +    build_name: 'sm-tarball'
> +    build_type: 'opt'
> +task:
> +  workerType: spidermonkey

This should have been done with |hg cp| instead of just adding a new file. Except no it shouldn't, because you should be inheriting from sm_base.yml and just changing task.payload.command, the metadata, and the symbol.

@@ +44,5 @@
> +        # see https://github.com/mozilla/treeherder/blob/master/ui/js/values.js
> +        platform: linux64
> +    # Rather then enforcing particular conventions we require that all build
> +    # tasks provide the "build" extra field to specify where the build and tests
> +    # files are located.

(This is kind of a lie in the original you copied from, since the sm jobs are not true build jobs, and don't produce stuff consumed by downstream jobs. But this task might actually end up being a build job. Sadly, the filename is going to vary.)

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ms2ger: do you plan on making the follow up jobs for extracting + building SM as a library, and then also linking it with something as well? I had planned on doing these things, but I don't want to duplicate work going forward.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Steve Fink [:sfink] [:s:] from comment #3)
> Comment on attachment 8753868 [details] [diff] [review]
> Create a build job that generates SpiderMonkey tarballs
> 
> @@ +42,5 @@
> > +# Install everything needed for the browser on this platform. Not all of it is
> > +# necessary for the JS shell, but it's less duplication to share tooltool
> > +# manifests.
> > +BROWSER_PLATFORM=$PLATFORM_OS$BITS
> > +TOOLTOOL_MANIFEST="$SRCDIR/browser/config/tooltool-manifests/$BROWSER_PLATFORM/releng.manifest"
> 
> Oh, bleh. You're copying my hairball. I'd say it's really too ugly to exist
> even once, but given its necessity, I would at least humbly request that you
> extract this out into sm-tooltool-config.sh or something, so that at least
> there's a single copy of this crap.

It's not clear to me how to pass the variable definitions from this new sm-tooltool-config.sh to the two scripts.
 
> @@ +64,5 @@
> > +
> > +# Run the script
> > +DIST=$UPLOAD_DIR $SRCDIR/js/src/make-source-package.sh
> > +BUILD_STATUS=$?
> > +exit $BUILD_STATUS
> 
> All this BUILD_STATUS goop is a long-winded way of exec'ing
> make-source-package.sh, but I guess it's safer for refactoring this way.

Do tell me how to do that if it's better, I don't know shell at all :)

> ::: testing/taskcluster/tasks/branches/base_jobs.yml
> @@ +172,5 @@
> > +        task: tasks/builds/sm_tarball.yml
> > +    when:
> > +      file_patterns:
> > +       - js/public/**
> > +       - js/src/**
> 
> This is ok for now. But what I'd really like this job to do, or a new
> downstream job to do, is now unpack the tarball somewhere and build it, then
> link a test program against it (statically, dynamically, or perhaps both in
> succession). And in that case, I'd like to extend these patterns to include
> mfbt, mozglue, zlib, nspr, etc.

Agreed. Arguably this applies to all the other SM builds as well. 

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #4)
> Ms2ger: do you plan on making the follow up jobs for extracting + building
> SM as a library, and then also linking it with something as well? I had
> planned on doing these things, but I don't want to duplicate work going
> forward.

I'll probably do the followup jobs if nobody beats me to it, but this is pretty far removed from my actual job, so I don't know when I'll have time to do it.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Create a build job that generates SpiderMonkey tarballs

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to :Ms2ger from comment #5)
> (In reply to Steve Fink [:sfink] [:s:] from comment #3)
> > Comment on attachment 8753868 [details] [diff] [review]
> > Create a build job that generates SpiderMonkey tarballs
> > 
> > @@ +42,5 @@
> > > +# Install everything needed for the browser on this platform. Not all of it is
> > > +# necessary for the JS shell, but it's less duplication to share tooltool
> > > +# manifests.
> > > +BROWSER_PLATFORM=$PLATFORM_OS$BITS
> > > +TOOLTOOL_MANIFEST="$SRCDIR/browser/config/tooltool-manifests/$BROWSER_PLATFORM/releng.manifest"
> > 
> > Oh, bleh. You're copying my hairball. I'd say it's really too ugly to exist
> > even once, but given its necessity, I would at least humbly request that you
> > extract this out into sm-tooltool-config.sh or something, so that at least
> > there's a single copy of this crap.
> 
> It's not clear to me how to pass the variable definitions from this new
> sm-tooltool-config.sh to the two scripts.

We could do `source sm-tooltool-config.sh`, but maybe there is a better way.

> > @@ +64,5 @@
> > > +
> > > +# Run the script
> > > +DIST=$UPLOAD_DIR $SRCDIR/js/src/make-source-package.sh
> > > +BUILD_STATUS=$?
> > > +exit $BUILD_STATUS
> > 
> > All this BUILD_STATUS goop is a long-winded way of exec'ing
> > make-source-package.sh, but I guess it's safer for refactoring this way.
> 
> Do tell me how to do that if it's better, I don't know shell at all :)

I think it would be nice to `set -e` at the top, so that we automatically exit if any command fails. It would also let us skip this $BUILD_STATUS boilerplate.

> (In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #4)
> > Ms2ger: do you plan on making the follow up jobs for extracting + building
> > SM as a library, and then also linking it with something as well? I had
> > planned on doing these things, but I don't want to duplicate work going
> > forward.
> 
> I'll probably do the followup jobs if nobody beats me to it, but this is
> pretty far removed from my actual job, so I don't know when I'll have time
> to do it.

As discussed on IRC, I'll take this. Thanks!

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create the SM(standalone) build for taskcluster

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32e6b29ae967

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create the SM(standalone) build for taskcluster

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ok this new version seems to be working quite well!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b6adbe3dc1a&selectedJob=21216820

In fact, it seemed to build just fine and is running jit-tests as we speak :)

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push came back green \o/

Comment 12

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8755033 [details] [diff] [review]
Create the SM(standalone) build for taskcluster

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

::: testing/taskcluster/scripts/builder/build-sm-standalone.sh
@@ +20,5 @@
> +# Build the freshly extracted, standalone SpiderMonkey.
> +AUTOMATION=1 ./mozjs-*/js/src/devtools/automation/autospider.sh $SPIDERMONKEY_VARIANT
> +
> +# Copy artifacts for upload by TaskCluster
> +cp -rL ./mozjs-*/obj-spider/dist/bin/{js,jsapi-tests,js-gdb.py} $UPLOAD_DIR

Copy the mozjs-*.tar.gz2 file here in addition, since it's the main output of this build.

Hm... actually, you probably ought to upload the libraries. They're kind of more relevant for this one than the js binary.

Either here or in a followup bug, can you package up the binaries in a tarball (including the static and shared libs), and just upload that. Because that's something nice that we could point people to for downloading. That would be pretty awesome. (It'll be somewhat tied to the versions of things here, so I think the main release should still be the source package. Especially since you still need the headers from it.)

::: testing/taskcluster/tasks/branches/base_jobs.yml
@@ +179,5 @@
> +    platforms:
> +      - Linux64
> +    types:
> +      opt:
> +        task: tasks/builds/sm_standalone.yml

Come to think of it, can you make a debug version of this as well? Because if people are going to use the built bits, we definitely want both opt and debug versions available.

It's a little clunky, but you'll need to make an sm_standalone_debug.yml that is pretty much the same, but it can pass an argument or an environment variable to control how it gets compiled.

Actually, come to think of it, let's land as-is and do that in a followup bug.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Just looked into the logs, and I realized that this isn't running the command I specified in sm_standalone.yml, and is running a plain spidermonkey build instead:

> ++ dirname ./workspace/build/src/testing/taskcluster/scripts/builder/build-sm.sh

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ok apparently when you inherit a property that is a list, it ends up concatenating the lists rather than having the child overwrite the base. So the solution is to make sm_base.yml not supply any command, and add a new sm_variant_base.yml which inherits from sm_base.yml and supplies the autospider command for plain/plaindebug/compacting/etc.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create the SM(pkg) build for taskcluster

* Rename "standalone" to "package".

* Create sm_variant_base.yml for builds that just want to specify and autospider
  variant.

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cee74f95f144

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create the SM(pkg) build for taskcluster

Fix old references to "standalone".

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd0416226960

Comment 18

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8755571 [details] [diff] [review]
Create the SM(pkg) build for taskcluster

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

::: testing/taskcluster/tasks/branches/base_jobs.yml
@@ +150,5 @@
>      when:
>        file_patterns:
>         - js/public/**
>         - js/src/**
> +  sm-standalone:

sm-package

@@ +155,5 @@
> +    platforms:
> +      - Linux64
> +    types:
> +      opt:
> +        task: tasks/builds/sm_standalone.yml

sm_package.yml

::: testing/taskcluster/tasks/builds/sm_package.yml
@@ +7,5 @@
> +  payload:
> +    env:
> +      SPIDERMONKEY_VARIANT: 'plain'
> +
> +    command: [ "/bin/bash", "-c", "cd /home/worker/ && ./bin/checkout-sources.sh && ./workspace/build/src/testing/taskcluster/scripts/builder/build-sm-package.sh" ]

I wonder if this is better, or if this should pass the variant on the command line and duplicate the command in every variant file.

Oh well, you have it implemented this way, and I have no clear preference.

::: testing/taskcluster/tasks/builds/sm_variant_base.yml
@@ +1,4 @@
> +$inherits:
> +  from: 'tasks/builds/sm_base.yml'
> +task:
> +    command: ["/bin/bash", "-c", "cd /home/worker/ && ./bin/checkout-sources.sh && ./workspace/build/src/testing/taskcluster/scripts/builder/build-sm.sh" ]

Yet another alternative -- I bet you could inherit from multiple files, so you could pull in just the command from sm_command.yml or something. But then you'd still be stuck with the environment passing, so meh.

Comment 19

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8755571 [details] [diff] [review]
Create the SM(pkg) build for taskcluster

Oops, posting comments un-obsoleted.

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create the SM(pkg) build for taskcluster

Forgot to remove "command: ..." from sm_base.yml after I made
sm_variant_base.yml... Carrying r+.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9fbf1406f93

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create the SM(pkg) build for taskcluster

Add missing files to make-source-package.sh:

- configure.py
- moz.configure
- js/moz.configure

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d883aec50b0

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create the SM(pkg) build for taskcluster

Fix copying $SRC/js/moz.configure over to $PKG/moz.configure instead of
$PKG/js/moz.configure

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c03200218a0e

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create the SM(pkg) build for taskcluster

Add missing taskcluster/moz.build to the source package.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f6ebf133af9

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create the SM(pkg) build for taskcluster

Add missing test.mozbuild to source package.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98723fa1933a

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create the SM(pkg) build for taskcluster

Add missing modules/fdlibm/** to the source package.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76bf92dc70d0

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create the SM(pkg) build for taskcluster

Only run `make check-style`, not all of `make check` which tries to do random
python build stuff that doesn't work in standalone spidermonkey standalone
builds.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06b85c9795b4

Comment 27

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create the SM(pkg) build for taskcluster

Ok, as discussed on IRC, let's just skip `make check` for these packaged builds.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=819c1fed91ca&selectedJob=21378671

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8daa119382d

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91532055b0e9

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b8daa119382d
https://hg.mozilla.org/mozilla-central/rev/91532055b0e9