Closed
Bug 1379540
Opened 7 years ago
Closed 7 years ago
Standalone SpiderMonkey source tarball should include convention-conforming configure script
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: ptomato, Assigned: ptomato)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
1.80 KB,
patch
|
ptomato
:
review+
jcristau
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
962 bytes,
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36 Steps to reproduce: Downloaded the latest mozjs-52.2.1 tarball from Treeherder. It doesn't have a configure script; by convention tarballs should have one. Additionally, it's a convention that the configure script does not error out when passed unknown options. For example, this is needed for autobuilders that pass a whole bunch of options like --disable-Werror to all the packages they build.
Assignee | ||
Comment 1•7 years ago
|
||
Here's a patch that works for me.
Assignee | ||
Updated•7 years ago
|
Blocks: sm-embedding
Updated•7 years ago
|
Attachment #8884719 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 2•7 years ago
|
||
I also found out when trying to get downstream GNOME and Linux distributions to build mozjs52 is that they will balk at having a build-time dependency on autoconf-2.13. I worked around this by hand-generating the js/src/old-configure script with autoconf-2.13 locally, and including that in my git repo. It requires some extra magic since a fresh git checkout will mess up the mtimes and cause the build to halt due to files being newer than other files, but I assume that magic won't be necessary in a tarball where the mtimes are preserved.
Comment 3•7 years ago
|
||
Comment on attachment 8884719 [details] [diff] [review] 0008-build-Include-configure-script-be-nicer-about-option.patch Review of attachment 8884719 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/configure @@ +5,5 @@ > +export OLD_CONFIGURE="$SRCDIR"/old-configure > + > +set -- "$@" --enable-project=js > + > +which python2.7 > /dev/null && exec python2.7 "$TOPSRCDIR/configure.py" "$@" || exec python "$TOPSRCDIR/configure.py" "$@" As much as I'd like a configure script to be in-tree (here and at the top-level), that will cause problems with VCS. What you want is to alter the process that creates the source tarballs (js/src/make-source-package.sh). ::: python/mozbuild/mozbuild/configure/__init__.py @@ -356,4 @@ > # All options should have been removed (handled) by now. > for arg in self._helper: > without_value = arg.split('=', 1)[0] > - raise InvalidOptionError('Unknown option: %s' % without_value) Rejecting unknown options was a deliberate choice.
Attachment #8884719 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8884719 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8887529 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #3) > What you want is to alter the process that creates the source tarballs > (js/src/make-source-package.sh). Thanks for the tip! Is this patch all I need, or can I not depend on the files having been generated at the time the process has run? > Rejecting unknown options was a deliberate choice. Is there any new information I can give that might make you re-evaluate that choice? If it helps, the reason I'm running into this is that all of GNOME's infrastructure expects source packages to adhere to this standard: https://github.com/cgwalters/build-api
Comment 6•7 years ago
|
||
(In reply to Philip Chimento [:ptomato] from comment #5) > Is there any new information I can give that might make you re-evaluate that > choice? If it helps, the reason I'm running into this is that all of GNOME's > infrastructure expects source packages to adhere to this standard: > https://github.com/cgwalters/build-api It SHOULD take the following arguments: --build=HOST --host=HOST The semantics of these arguments are as defined by autoconf. Our configure takes --host and --target, where --host doesn't mean the same as autoconf's. You're going to have problems. The configure script MUST either ignore unknown options that start with --enable- and --disable-, or accept an argument --help which prints valid options We have the latter.
Comment 7•7 years ago
|
||
Comment on attachment 8887529 [details] [diff] [review] Add generated configure files to JS source tarball Review of attachment 8887529 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/make-source-package.sh @@ +98,5 @@ > cp -pPR ${TOPSRCDIR}/.cargo/config.in ${tgtpath}/.cargo > > + # copy generated configure files to avoid build dependency on autoconf 2.13 > + cp -pPR ${TOPSRCDIR}/js/src/configure ${tgtpath}/js/src > + cp -pPR ${TOPSRCDIR}/js/src/old-configure ${tgtpath}/js/src I'm pretty sure this breaks the spidermonkey-package builds, where those files don't exist by the time this script runs. (see taskcluster/scripts/builder/build-sm-package.sh)
Attachment #8887529 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #7) > I'm pretty sure this breaks the spidermonkey-package builds, where those > files don't exist by the time this script runs. (see > taskcluster/scripts/builder/build-sm-package.sh) Would autoconf be available at that time, so that I could generate the scripts on the spot? Although I wouldn't know whether it's called autoconf213, autoconf2.13 or autoconf-2.13 on whatever machine runs taskcluster/scripts/builder/build-sm-package.sh. I'll attach a patch, but it assumes that autoconf is available and it is called autoconf213. (In reply to Mike Hommey [:glandium] from comment #6) > The configure script MUST either ignore unknown options that start with > --enable- and --disable-, or accept an argument --help which prints valid > options > > We have the latter. I missed the latter part, you're right. I'll try to get GNOME to shape up there. https://bugzilla.gnome.org/show_bug.cgi?id=785257
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8887529 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889025 -
Flags: review?(mh+mozilla)
Comment 10•7 years ago
|
||
Comment on attachment 8889025 [details] [diff] [review] Add generated configure files to JS source tarball Review of attachment 8889025 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/make-source-package.sh @@ +7,4 @@ > > : ${MKDIR:=mkdir} > : ${TAR:=tar} > +: ${AUTOCONF:=autoconf213} You should default to autoconf2.13, here, which is what the job creating the tarballs have available. @@ +99,5 @@ > ${MKDIR} -p ${tgtpath}/.cargo > cp -pPR ${TOPSRCDIR}/.cargo/config.in ${tgtpath}/.cargo > > + # generate and copy configure files to avoid build dependency on autoconf 2.13 > + ${AUTOCONF} ${TOPSRCDIR}/js/src/configure.in >${TOPSRCDIR}/js/src/configure configure.in can just be copied to configure (with cp). You also need to set its executable bit. @@ +101,5 @@ > > + # generate and copy configure files to avoid build dependency on autoconf 2.13 > + ${AUTOCONF} ${TOPSRCDIR}/js/src/configure.in >${TOPSRCDIR}/js/src/configure > + ${AUTOCONF} --localdir=${TOPSRCDIR}/js/src \ > + ${TOPSRCDIR}/js/src/old-configure.in >${TOPSRCDIR}/js/src/old-configure You could just create the file in the final location directly, instead of adding a file to the source directory to then copy it. (same above, btw)
Attachment #8889025 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 11•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8889025 -
Attachment is obsolete: true
Assignee | ||
Comment 12•7 years ago
|
||
Comment on attachment 8890022 [details] [diff] [review] Add generated configure files to JS source tarball Still not entirely sure how to test this in anything resembling the environment that it runs in, though...
Attachment #8890022 -
Flags: review?(mh+mozilla)
Updated•7 years ago
|
Attachment #8890022 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 13•7 years ago
|
||
Hmm, unfortunately it looks like the job that creates the tarball doesn't have autoconf2.13 available: https://treeherder.mozilla.org/logviewer.html#?job_id=125785206&repo=try&lineNumber=322#L322 How could I find out what to do here? I suppose I could push another try commit with "ls /usr/bin/autoconf*" added to the script, but is there anywhere I could look at the worker config?
Comment 14•7 years ago
|
||
The logs from a successful job say: [task 2017-08-25T00:30:46.064530Z] checking for autoconf... /usr/bin/autoconf-2.13 https://public-artifacts.taskcluster.net/eDHhls1RSJGFFYFp86A6PA/0/public/logs/live_backing.log
Assignee | ||
Comment 15•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8890022 -
Attachment is obsolete: true
Assignee | ||
Comment 16•7 years ago
|
||
Comment on attachment 8901417 [details] [diff] [review] Add generated configure files to JS source tarball Ah, it is autoconf-2.13 with a dash, after all. I assume this can still be r+ with that change...
Attachment #8901417 -
Flags: review+
Assignee | ||
Comment 17•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8331f017fa51113774cf11b29ae1ecedafc39fa&selectedJob=126056484 I checked the generated tarball; configure and old-configure are included, configure is executable, and each is newer than its .in counterpart. Windows builds are broken in the try push due to the intermittent failure from bug 1318172, and also what looks like bug 1391424 (which was fixed but then backed out.) I'm assuming it's OK to request checkin on this since the SpiderMonkey pkg build succeeded and that's the only thing that was modified?
Keywords: checkin-needed
Updated•7 years ago
|
Assignee: nobody → philip.chimento
Comment 18•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c634e6a3ab5 Add generated configure files to JS source tarball. r=glandium
Keywords: checkin-needed
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1c634e6a3ab5
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 20•7 years ago
|
||
Comment on attachment 8901417 [details] [diff] [review] Add generated configure files to JS source tarball [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: Makes building and packaging standalone SpiderMonkey easier for downstream Linux distributions. User impact if declined: No effect on Firefox users, but embedded SpiderMonkey users will have to either have a dependency on Autoconf 2.13, which is outdated enough that it's a problem for most Linux distributions, or carry a patch to generate these files themselves at packaging time. Fix Landed on Version: 57 Risk to taking this patch (and alternatives if risky): Not risky, only affects the SpiderMonkey pkg build String or UUID changes made by this patch: None See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8901417 -
Flags: approval-mozilla-esr52?
Comment 21•7 years ago
|
||
Comment on attachment 8901417 [details] [diff] [review] Add generated configure files to JS source tarball packaging tweak for spidermonkey source tarball, esr52.4+
Attachment #8901417 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment 22•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/3421fe5c8e72
status-firefox-esr52:
--- → fixed
Comment 23•7 years ago
|
||
This appears to have broken the SM(pkg) job on ESR52. https://treeherder.mozilla.org/logviewer.html#?job_id=128579364&repo=mozilla-esr52 Can you take a look, Philip? I'll hold off on backing out for now since everything is working OK.
Flags: needinfo?(philip.chimento)
Comment 24•7 years ago
|
||
everything *else* is working OK
Assignee | ||
Comment 25•7 years ago
|
||
Assignee | ||
Comment 26•7 years ago
|
||
Removing the -p flag from cp seems to have worked on a try push. I'm a bit paranoid that it might be intermittent depending on which order the files are extracted from the tarball, but this change makes sense to me in any case.
Flags: needinfo?(philip.chimento) → needinfo?(ryanvm)
Comment 27•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/83f59534440d Don't preserve timestamp when copying configure.in to configure. r=RyanVM
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/83f59534440d
Comment 29•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr52/rev/cbeedaa8ba69180a48d1944e2171d0dc5dd81c66
Flags: needinfo?(ryanvm)
You need to log in
before you can comment on or make changes to this bug.
Description
•