386874 - add a python emulation for nsinstall for --disable-compile-environment

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Axel Hecht [:Pike]]

Attachment: move nsinstall check below the python check, add a python port of nsinstall

On Mac and Linux, the build system doesn't have a nsinstall implementation without having a compiler. For --disable-compile-environment that's unfortunate, in particular for localizers. As l10n repackaging uses nsinstall, but only rarely, the perf impact of having that implemented in python is negligible.

As perf is neglected, I'm not caring about links and just do copies hardcoded. The options are implemented so that they don't cause errors, but ignored.

Patch coming up.

Comment 1

[Benjamin Smedberg]

Comment on attachment 270930 [details] [diff] [review]
move nsinstall check below the python check, add a python port of nsinstall

>Index: configure.in

>@@ -641,6 +633,16 @@
> fi
> echo PYTHON="$PYTHON"
> 
>+AC_PATH_PROGS(NSINSTALL_BIN, nsinstall )
>+if test -z "$COMPILE_ENVIRONMENT"; then
>+if test -z "$NSINSTALL_BIN" || test "$NSINSTALL_BIN" = ":"; then
>+    _fullsrcdir=`${PYTHON} -c "import os.path
>+print os.path.abspath('${srcdir}')"`
>+    NSINSTALL_BIN="${PYTHON} ${_fullsrcdir}/config/nsinstall.py"

I don't understand why you have to do all this topsrcdir munging here. Why can't this be:

NSINSTALL_BIN='$(PYTHON) $(topsrcdir)/config/nsinstall.py'

and let make do the substitution?

>Index: config/nsinstall.py

This file makes me cringe a little bit... I'd kinda prefer a function which does the installation driven by

if __name__ == '__main__':
  from optparse import OptionParser
  ...etc

But I'm not going to hold up development for this... please be mindful of that style in the future, though.

>+# The remaining arguments are not used in our tree, thus they're not
>+# implented.
>+p.add_option('-C', action="store", metavar="CWD",
>+             help="NOT SUPPORTED")
>+p.add_option('-o',
>+             help="Set owner (NOT SUPPORTED)", metavar="owner")
>+p.add_option('-g',
>+             help="Set group (NOT SUPPORTED)", metavar="group")
>+
>+(options, args) = p.parse_args()
>+
>+assert not (options.C or options.o or options.g)\
>+       , "Specified commandline options not supported by this port"

asserts are only evaluated when the script is run non-optimized. The better way to do this is:

def BadArg(option, opt, value, parser):
  raise Exception("Option %s not supported." % opt)

p.add_option('-g', action='callback', callback=BadArg)

Comment 2

[Axel Hecht [:Pike]]

Attachment: error for real on bad args, use \${topsrcdir} in configure.in

I kept nsinstall.py as one script without module support. If we move more work into python, we should have a separate utility module, which nsinstall.py can call into then.

I couldn't use ${topsrcdir} in configure.in because that's substituted by configure in, which is supposed to work with relative paths on invocation (which is cool, I use lots these days). I could escape the variable expansion though, so that looks much nicer now. I could escape the variable expansion of PYTHON, too, if you prefer.

I used your suggestion to do the callback on the unsupported args, but switched to OptionParser.error, which formats the error message like all the other ones. I made the error message align with "no such option: %s", which OptionParser gives if you specify an option it doesn't know about.

Comment 3

[Benjamin Smedberg]

I didn't say "${topsrcdir}" but rather '$(topsrcdir)'. That is, the autoconf.mk setting would be:

NSINSTALL = $(PYTHON) $(topsrcdir)/config/nsinstall.py

Comment 4

[Axel Hecht [:Pike]]

Attachment: use '$()'

New patch, just using $() in single quotes. Without the quotes, I end up in python's shell on running configure. I removed the comment about escaping, too.

Comment 5

[Benjamin Smedberg]

Comment on attachment 272642 [details] [diff] [review]
use '$()'

Sorry for not noticing before, but you need to re-add the AC_MSG_ERROR for the case where we're not disabling compiles, so something like:

if test -z "$NSINSTALL_BIN" || test "$NSINSTALL_BIN" = ":"; then
  if test -z "$COMPILE_ENVIRONMENT"; then
    NSINSTALL_BIN=...
  else
    AC_MSG_ERROR(...)
  fi
fi

r=me with that fixed

Comment 6

[Axel Hecht [:Pike]]

As per discussion on irc, I was supposed to ignore comment 5.

Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1847; previous revision: 1.1846
done
RCS file: /cvsroot/mozilla/config/nsinstall.py,v
done
Checking in config/nsinstall.py;
/cvsroot/mozilla/config/nsinstall.py,v  <--  nsinstall.py
initial revision: 1.1

I'd like to do a separate patch for the branch. That needs to be a tad different as that shouldn't need a python check in general. Leaving open for that.

Comment 7

[Axel Hecht [:Pike]]

Attachment: configure.in patch for the branch

This is the patch that I'd like to use on the MOZILLA_1_8_BRANCH to go together with branching nsinstall.py.

It's a different patch, as we're not requiring python on the branch, and we most likely don't want to, so I only test for python if nsinstall isn't found and --disable-configure-environment is specified. It does expand the PYTHON variable inside NSINSTALL_BIN, too, as that's not defined in autoconf.mk.in on the branch.

The other way to go about this would be to check and define python in general and just not error on the lack of python, but that would only be valuable if we needed python for more things on the branch, like minotaur testing. I don't see us adding that for the branch right now, though, and landing big repackaging changes on the branch seems to be out of the question, too, so just looking for python locally should suffice.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 272958 [details] [diff] [review]
configure.in patch for the branch

Requesting approval for branch landing.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 272958 [details] [diff] [review]
configure.in patch for the branch

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 10

[Axel Hecht [:Pike]]

Landed on branch and tested.