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

RESOLVED FIXED

Status

()

Toolkit
Build Config
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Pike, Assigned: Pike)

Tracking

({verified1.8.1.8})

unspecified
verified1.8.1.8
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 270930 [details] [diff] [review]
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.
Attachment #270930 - Flags: review?(benjamin)

Comment 1

11 years ago
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)
Attachment #270930 - Flags: review?(benjamin) → review-
(Assignee)

Comment 2

11 years ago
Created attachment 272631 [details] [diff] [review]
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.
Attachment #270930 - Attachment is obsolete: true
Attachment #272631 - Flags: review?(benjamin)

Comment 3

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

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

Comment 4

11 years ago
Created attachment 272642 [details] [diff] [review]
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.
Attachment #272631 - Attachment is obsolete: true
Attachment #272642 - Flags: review?(benjamin)
Attachment #272631 - Flags: review?(benjamin)

Comment 5

11 years ago
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
Attachment #272642 - Flags: review?(benjamin) → review+
(Assignee)

Comment 6

11 years ago
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.
Whiteboard: fixed on trunk
(Assignee)

Comment 7

11 years ago
Created attachment 272958 [details] [diff] [review]
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.
Attachment #272958 - Flags: review?(benjamin)

Updated

11 years ago
Attachment #272958 - Flags: review?(benjamin) → review+
(Assignee)

Comment 8

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

Requesting approval for branch landing.
Attachment #272958 - Flags: approval1.8.1.7?
Comment on attachment 272958 [details] [diff] [review]
configure.in patch for the branch

approved for 1.8.1.7, a=dveditz for release-drivers
Attachment #272958 - Flags: approval1.8.1.7? → approval1.8.1.7+
(Assignee)

Comment 10

11 years ago
Landed on branch and tested.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: verified1.8.1.7
Resolution: --- → FIXED
Whiteboard: fixed on trunk

Updated

11 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.