Closed Bug 1273917 Opened 3 years ago Closed 3 years ago

Create a build job that generates SpiderMonkey tarballs

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- fixed

People

(Reporter: Ms2ger, Assigned: fitzgen)

References

(Blocks 3 open bugs)

Details

Attachments

(1 file, 13 obsolete files)

22.07 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
Next up is trying to actually build from it.
Ha I started working on a patch yesterday, and was stumbling through taskcluster. Glad I don't have to anymore :)
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.)
Attachment #8753868 - Flags: review?(sphink)
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.
Flags: needinfo?(Ms2ger)
(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.
Flags: needinfo?(Ms2ger)
Attachment #8753868 - Attachment is obsolete: true
Assignee: Ms2ger → nfitzgerald
Blocks: 1263289
Status: NEW → ASSIGNED
Iteration: --- → 49.2 - May 23
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [devtools-html]
(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!
Attachment #8754399 - Attachment is obsolete: true
Attachment #8755025 - Attachment is obsolete: true
Attachment #8755025 - Flags: review?(sphink)
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 :)
Try push came back green \o/
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.
Attachment #8755033 - Flags: review?(sphink) → review+
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
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.
* Rename "standalone" to "package".

* Create sm_variant_base.yml for builds that just want to specify and autospider
  variant.
Attachment #8755571 - Flags: review?(sphink)
Attachment #8755033 - Attachment is obsolete: true
Fix old references to "standalone".

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd0416226960
Attachment #8755574 - Flags: review?(sphink)
Attachment #8755571 - Attachment is obsolete: true
Attachment #8755571 - Flags: review?(sphink)
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.
Attachment #8755571 - Attachment is obsolete: false
Comment on attachment 8755571 [details] [diff] [review]
Create the SM(pkg) build for taskcluster

Oops, posting comments un-obsoleted.
Attachment #8755571 - Attachment is obsolete: true
Attachment #8755574 - Flags: review?(sphink) → review+
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
Attachment #8755595 - Flags: review+
Attachment #8755574 - Attachment is obsolete: true
Add missing files to make-source-package.sh:

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d883aec50b0
Attachment #8755624 - Flags: review+
Attachment #8755595 - Attachment is obsolete: true
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
Attachment #8755631 - Flags: review+
Attachment #8755624 - Attachment is obsolete: true
Add missing taskcluster/moz.build to the source package.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f6ebf133af9
Attachment #8755635 - Flags: review+
Attachment #8755631 - Attachment is obsolete: true
Attachment #8755635 - Attachment is obsolete: true
Attachment #8755636 - Attachment is obsolete: true
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
Attachment #8755995 - Flags: review+
Attachment #8755643 - Attachment is obsolete: true
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
Attachment #8756023 - Flags: review+
Attachment #8755995 - Attachment is obsolete: true
Iteration: 49.2 - May 23 → 49.3 - Jun 6
https://hg.mozilla.org/mozilla-central/rev/b8daa119382d
https://hg.mozilla.org/mozilla-central/rev/91532055b0e9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1277338
No longer blocks: 1263289
Blocks: 1263289
Depends on: 1283434
Whiteboard: [devtools-html]
You need to log in before you can comment on or make changes to this bug.