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)

52 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- fixed
firefox57 --- fixed

People

(Reporter: ptomato, Assigned: ptomato)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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.
Here's a patch that works for me.
Blocks: 1379541
Blocks: sm-embedding
Attachment #8884719 - Flags: review?(mh+mozilla)
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 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)
Attachment #8884719 - Attachment is obsolete: true
Attachment #8887529 - Flags: review?(mh+mozilla)
(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
(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 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)
(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
Attachment #8887529 - Attachment is obsolete: true
Attachment #8889025 - Flags: review?(mh+mozilla)
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)
Attachment #8889025 - Attachment is obsolete: true
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)
Attachment #8890022 - Flags: review?(mh+mozilla) → review+
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?
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
Attachment #8890022 - Attachment is obsolete: true
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+
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
Assignee: nobody → philip.chimento
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
https://hg.mozilla.org/mozilla-central/rev/1c634e6a3ab5
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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 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+
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)
everything *else* is working OK
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)
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
You need to log in before you can comment on or make changes to this bug.