Create a build job that generates SpiderMonkey tarballs

RESOLVED FIXED in Firefox 49

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ms2ger, Assigned: fitzgen)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla49
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 13 obsolete attachments)

22.07 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Next up is trying to actually build from it.
(Reporter)

Comment 1

2 years ago
Created attachment 8753868 [details] [diff] [review]
Create a build job that generates SpiderMonkey tarballs
Attachment #8753868 - Flags: review?(sphink)
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)
(Reporter)

Comment 5

2 years ago
(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)
(Reporter)

Comment 6

2 years ago
Created attachment 8754399 [details] [diff] [review]
Create a build job that generates SpiderMonkey tarballs
(Reporter)

Updated

2 years ago
Attachment #8753868 - Attachment is obsolete: true

Updated

2 years ago
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!
Created attachment 8755025 [details] [diff] [review]
Create the SM(standalone) build for taskcluster

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32e6b29ae967
Attachment #8755025 - Flags: review?(sphink)
Attachment #8754399 - Attachment is obsolete: true
Created attachment 8755033 [details] [diff] [review]
Create the SM(standalone) build for taskcluster
Attachment #8755033 - Flags: review?(sphink)
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.
Created attachment 8755571 [details] [diff] [review]
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.
Attachment #8755571 - Flags: review?(sphink)
Attachment #8755033 - Attachment is obsolete: true
Created attachment 8755574 [details] [diff] [review]
Create the SM(pkg) build for taskcluster

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+
Created attachment 8755595 [details] [diff] [review]
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
Attachment #8755595 - Flags: review+
Attachment #8755574 - Attachment is obsolete: true
Created attachment 8755624 [details] [diff] [review]
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
Attachment #8755624 - Flags: review+
Attachment #8755595 - Attachment is obsolete: true
Created attachment 8755631 [details] [diff] [review]
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
Attachment #8755631 - Flags: review+
Attachment #8755624 - Attachment is obsolete: true
Created attachment 8755635 [details] [diff] [review]
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
Attachment #8755635 - Flags: review+
Attachment #8755631 - Attachment is obsolete: true
Created attachment 8755636 [details] [diff] [review]
Create the SM(pkg) build for taskcluster

Add missing test.mozbuild to source package.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98723fa1933a
Attachment #8755636 - Flags: review+
Attachment #8755635 - Attachment is obsolete: true
Created attachment 8755643 [details] [diff] [review]
Create the SM(pkg) build for taskcluster

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76bf92dc70d0
Attachment #8755643 - Flags: review+
Attachment #8755636 - Attachment is obsolete: true
Created attachment 8755995 [details] [diff] [review]
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
Attachment #8755995 - Flags: review+
Attachment #8755643 - Attachment is obsolete: true
Created attachment 8756023 [details] [diff] [review]
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
Attachment #8756023 - Flags: review+
Attachment #8755995 - Attachment is obsolete: true

Updated

2 years ago
Iteration: 49.2 - May 23 → 49.3 - Jun 6

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b8daa119382d
https://hg.mozilla.org/mozilla-central/rev/91532055b0e9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

2 years ago
Blocks: 1277338
No longer blocks: 1263289

Updated

2 years ago
Blocks: 1263289
(Reporter)

Updated

2 years ago
Depends on: 1283434

Updated

2 years ago
Whiteboard: [devtools-html]
You need to log in before you can comment on or make changes to this bug.