[MERGE REVIEW] Remove ConfigObj validation

[John Arbash Meinel]

Martin Pool wrote:
> On  8 Jun 2006, Aaron Bentley <aaron.bentley at utoronto.ca> wrote:
>> John Meinel has observed that importing the ConfigObj validator was
>> taking a significant portion of startup time.  Since the validator is an
>> optional component, and we're not using it, this patch deletes it.
>>
>> It also adds a benchmark for startup time.  According to the benchmark,
>> this cuts our startup time from 107 ms to 47 ms.
> 
> Jolly good.  I think this merged already but I have one comment:
> 
>> +    def run_bzr_external(self, *args, **kwargs):
>> +        bzr_path = os.path.dirname(os.path.dirname(bzrlib.__file__))+'/bzr'
>> +        if len(args) == 1:
>> +            args = shlex.split(args[0])
>> +        args = list(args)
>> +        process = Popen([bzr_path]+args, stdout=PIPE, stderr=PIPE)
>> +        out = process.stdout.read()
>> +        err = process.stderr.read()
>> +        retcode = process.wait()
>> +        supplied_retcode = kwargs.get('retcode')
>> +        if supplied_retcode is not None:
>> +            assert supplied_retcode == retcode
>> +        else:
>> +            assert retcode == 0
>> +        return [out, err]
>> +
> 
> +1 to having something like this added back in; it is useful in rare
> cases.  Perhaps 'run_bzr_subprocess' would be better?  I do think it
> should have a docstring:
> 
>   Run bzr in a subprocess for testing.
> 
>   This starts a new Python interpreter and runs bzr in there. 
>   This should only be used for tests that have a justifiable need for
>   this isolation: e.g. they are testing startup time, or signal
>   handling, or early startup code, etc.  Subprocess code can't be 
>   profiled or debugged so easily.
> 
> Robert (iirc) expressed the other day a dislike of run_bzr things that
> split up strings; they're prone to split the wrong way.  (Though using
> shlex might help.)  Also we have a general principle against magic-dwim
> on arguments.  Since this should be called rarely I would rather just
> delete the splitting of args and make the caller be explicit.
> 
> It may be better to run sys.executable, which will get the same Python
> interpreter and might help on Windows.
> 
> Since retcode is the only kwarg why not just take it explicitly?

I would like to second Martin's comments.

1) Use sys.executable because most scripts are not executable in the
same sense on windows. So we should do:
process = Popen([sys.executable, bzr_path] + args], ...
2) I'm not big on the shlex stuff (as I mentioned to Aaron at the time).
He had a small point that "You can write them as you would write them on
the command line", and that it makes it easier to write tests, which
should be encouraged.
I've switched to using the separate args model myself. And we do have a
small problem if someone wanted to supply a single string with a space
in it. I think this would work:
  run_bzr_external('"foo bar"')
But I'm not positive. I realize this is an edge case, however.
3) I would also agree with the retcode stuff. run_bzr_external
(run_bzr_subprocess) is an independent function, not wrapped around
anything else, so it would make sense to have explicit arguments.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060610/34509317/attachment.pgp