Merge lp:~gagern/bzr/bug560030-include-bash-completion-plugin into lp:bzr

Source is not Python 2.4 compatible:
+ "debug": debug_output if debug else "",

Should the plugin really be unconditionally installed when a number of platforms don't use bash?

Thanks for spotting the Python 2.4 thing. Testing it with Python 2.4 I also found another problem: the plugin used to depend on testtools for the registry of the --parallel option to the bzr selftest command.

Isn't there a large number of users that don't use launchpad, or that never merge NEWS files, or that don't store their credentials in ~/.netrc? Come to think of it, isn't there a large number of users that don't sign any commits, or never shelve stuff, or don't send merge directives via mail? Or, even more to the point, how many users are using the shell-completion builtin?

I bet you get my point: yes, there are certainly users that won't require the bash_completion plugin. But it's small enough that the extra space requirement shouldn't hurt, designed with the hope that the __init__.py executes pretty fast, and on the whole shouldn't hurt.

Seeing as most major GNU/Linux distros install the outdated completion script along with bzr despite its shortcomings seems to me an indication that there will be a large enough user base to warrant inclusion into core. There are some bzr builtins and core plugins that I assume have a smaller user base.

And if some platform is certain to never have any need for bash completion, and is concerned enough about memory requirements or whatever, they can simply delete the plugin in their installation.

I'm ok with merging this into core, but I'd like a few cleanups first.

Mostly, the plugin carries some excessive baggage from its birth as a standalone plugin that doesn't match the policies we have for the core plugins.

Regarding distribution, windows installers need to be told explicitly about new
plugins, so no worries there. For the other OSes, well, bash is at least available
if not already installed by default.

Regarding load time impact, you can go a step further and use CommandRegistry.register_lazy().

We need tests. I don't clearly understand how the completion works and you may need to redesign a bit to be able to test at the python level. Don't hesitate
to ask for help.

I don't ask for a complete coverage here, but making sure we know how to
complete at least some basic commands and their args will be a minimum.
Having a design (with associated tests) ready for further enhancements
(prefix completion for known transport protocols or useful shortcuts like :parent, :push and the like) will be a nice bonus though.

The above are rather generic, here are some more concrete points:
- delete .bzrignore,
- use bzrlib version like the other plugins,
- watch for PEP8 compliance (head, fun, tail, etc in bashcomp.py miss double vertical spaces)

Thanks for the review, Vincent! Probably won't have time to deal with it before 27 April or so, so I'm putting this on hold until then.

Testing is a real problem here: the things I'd be most concerned about or interested in is bash syntax and behaviour, and obviously testing these involves executing bash, which can't be done from within python only. Maybe I can write tests that check whether bash is available, and skip if it isn't.

I think this approach is the only thing that could really catch regressions, in particular the one I recently introduced and fixed on trunk. This is the only thing that can actually check the behaviour of the hardcoded template around the simple dynamically generated code.

Another thing I might do is refactor stuff so that data aquisition and code generation are separated. That way, we could ensure that the data aquisition does the correct thing, and hopefully keep the code generation simple enough so it will be easy to check. This might also open the door for other shells, zsh in particular, so it might be a good thing even without tests. But it's a major step, so it will take a bit of time.

>>>>> Martin von Gagern <email address hidden> writes:

    > Thanks for the review, Vincent! Probably won't have time to deal
    > with it before 27 April or so, so I'm putting this on hold until
    > then.

    > Testing is a real problem here: the things I'd be most concerned
    > about or interested in is bash syntax and behaviour, and obviously
    > testing these involves executing bash, which can't be done from
    > within python only. Maybe I can write tests that check whether
    > bash is available, and skip if it isn't.

Yup, that's the idea, define a feature in bzrlib.tests.features for
that.

    > I think this approach is the only thing that could really catch
    > regressions, in particular the one I recently introduced and fixed
    > on trunk. This is the only thing that can actually check the
    > behaviour of the hardcoded template around the simple dynamically
    > generated code.

Exactly. You may even want to add more functions in the template to have
smaller tests.

    > Another thing I might do is refactor stuff so that data aquisition
    > and code generation are separated. That way, we could ensure that
    > the data aquisition does the correct thing, and hopefully keep the
    > code generation simple enough so it will be easy to check.

... no comment :)

    > This might also open the door for other shells, zsh in particular,
    > so it might be a good thing even without tests. But it's a major
    > step, so it will take a bit of time.

Yes, I didn't mention that in my first review and our experience is that
it's easier to review and land smaller proposals, so don't try to
address all of that in a single proposal.

But once you get there, have a look at
bzrlib.builtins.cmd_shell_complete which is used by zsh (I think).

Some more nits found while re-reading your mp:

- Don't use relative imports, they tend to break in obscure ways if you
  happend to have a PYTHON_PATH that includes the current dir and you're
  working in the directory of your plugin (or another plugin that use
  the same package name or another python program, etc). Even if *you*
  don't do that, we had bug reports in the past about that.

- try to use:

    from bzrlib import commands
    class cmd_foo(commands.Command)

  instead of

    from bzrlib.command import Command
    class cmd_foo(Command)

  We don't always respect this rule, but we're fixing such usages as we
  encounter them.

> delete .bzrignore,
Dropped.

> use bzrlib version like the other plugins,
Done, and also dropped meta.py along the way.

> watch for PEP8 compliance
Should be better now.

> check whether bash is available, and skip if it isn't.
> define a feature in bzrlib.tests.features for that.
Have a test for bash, but it's in my own testing code, as I can't imagine other tests requiring bash as well, and as I wanted to include the test suite in the standalone distribution of the plugin as well.

Maybe some general feature class testing for the existence of arbitrary binaries, possibly taking the PATH environment variable and platform conventions into account might be a good idea as well. But that would be a different merge request, I think.

> You may even want to add more functions in the template
> to have smaller tests.
I don't feel like clobbering the bash function namespace with too many functions. I consider the completion tests to be pretty much black box tests: input a list of words, output a list of completions. Otherwise I'd have to write not only a number of smaller functions, but a suitable testing framework for all of these. Or express test input and assertions in bash syntax.

> bzrlib.builtins.cmd_shell_complete which is used by zsh (I think).
At least not by the zsh completion script shipped by either zsh or bzr. There are scripts out there making use of it, but none seem particularly "official". In any case, I'll investigate zsh in more detail once this got landed.

> Don't use relative imports, they tend to break in obscure ways
How so? I thought Python 2 does relative imports by default, and only falls back to absolute imports if the relative import fails. So I would have assumed that a relative import would be on the safe side, whereas an absolute one could break in cases where there was a relative import of the same name available.

Anyway, I simply take your word for it, and changed everything to absolute paths. Don't want to do the same for the standalone distribution, though, as there people might choose a different plugin name.

> try to use [module imports instead of class imports]
Done.

>>>>> Martin von Gagern <email address hidden> writes:

    >> check whether bash is available, and skip if it isn't.
    >> define a feature in bzrlib.tests.features for that.

    > Have a test for bash, but it's in my own testing code, as I can't
    > imagine other tests requiring bash as well, and as I wanted to
    > include the test suite in the standalone distribution of the
    > plugin as well.

Perfect.

    > Maybe some general feature class testing for the existence of
    > arbitrary binaries, possibly taking the PATH environment variable
    > and platform conventions into account might be a good idea as
    > well. But that would be a different merge request, I think.

Very good idea, many plugins could certainly benefit from it.

    >> You may even want to add more functions in the template
    >> to have smaller tests.

    > I don't feel like clobbering the bash function namespace with too
    > many functions. I consider the completion tests to be pretty much
    > black box tests: input a list of words, output a list of
    > completions.

Hehe, yes, you got it right, I'm not a big fan of blackbox tests :)

For the sake of the discussion, imagine that we encounter a bug about an
option with a weird character like ' or `, whatever.

I'd like the ability to write a test for just the option related part
without having to rely on a command that will use this option.

But except for the remarks bwlow, I'm fine with the tests you've added.

    > Otherwise I'd have to write not only a number of smaller
    > functions, but a suitable testing framework for all of these. Or
    > express test input and assertions in bash syntax.

Yeah, well, that's the point, I don't have strong opinions there as I
don't know the code well enough to decide whether or not you can write
the test in pythin or if bash is really required.

    >> bzrlib.builtins.cmd_shell_complete which is used by zsh (I think).

    > At least not by the zsh completion script shipped by either zsh or
    > bzr.

Yes it is, look for shell-complete (not shell_complete) in contrib/zsh/_bzr.

    > There are scripts out there making use of it, but none seem
    > particularly "official". In any case, I'll investigate zsh in more
    > detail once this got landed.

Yeah, no problem.

    >> Don't use relative imports, they tend to break in obscure ways

    > How so?

I don't remember the bug number off-hand, but having '.' in your
PYTHONPATH and '.' containing an unrelated python module with the same
name may wrongly trigger an import while using
'bzrlib.plugins.<plugin_name>' ensures we get the right one.

<snip/>

   > Don't want to do the same for the standalone distribution, though,
   > as there people might choose a different plugin name.

Wow, interesting... There are so many cases where you *need* the plugin
name to be the same as its containing directory (BZR_PLUGINS_AT works
hard to remove this limitation) that I didn't think about this case... I
won't explore it myself :)

Now for some specifics:

19 --- bzrlib/plugins/bash_completion/README.txt 1970-01-01 00:00:00 +0000

This still includes material related to the use of the plugin in its
non-core version...

On 04.05.2010 12:32, Vincent Ladeuil wrote:
> For the sake of the discussion, imagine that we encounter a bug about an
> option with a weird character like ' or `, whatever.

The black-box tests are mostly about testing the fixed functionality
part of the template, not individual commands. But I've just adjusted
the get_script method to make it easier for future tests to provide
cooked completion data without actually having to provide commands for
these.

> Yeah, well, that's the point, I don't have strong opinions there as I
> don't know the code well enough to decide whether or not you can write
> the test in pythin or if bash is really required.

The bash completion stuff is advanced enough that a simplistic python
interpreter wouldn't suffice. And an afvanced one would be a project in
its own right, and require a full test suite to boot...

> Yes it is, look for shell-complete (not shell_complete) in contrib/zsh/_bzr.

OK, it does command name completion using that callback. But not option
completion, although that would be provided by the shell-completion
builtin as well, at least to some degree.

> 19 --- bzrlib/plugins/bash_completion/README.txt 1970-01-01 00:00:00 +0000
>
> This still includes material related to the use of the plugin in its
> non-core version.

Once the plugin got merged, I'd adjust the README to state the fact, and
point out that there are distinct lines of development. That much I'd
write for the standalone plugin and merge into core in a separate merge
request. So I'd like the README to stay a while, but get updated.

> Depending on how long this plugin will continue to live outside of the
> bzr tree, we may want to clean this stuff. I wonder how it will will
> behave (maintaining it in both trees and merging from the plugin may
> trigger some spurious conflicts whatever choise we make)...

I guess I'll keep the plugin around for about a year or so, so people
can use it even without updating their bzr setup. Once most distros ship
bzr with the plugin in place, I'll probably phase out the standalone
distribution. And in any case I'll probably not keep the standalone
branch up to date in case third parties provide branches against the bzr
tree affecting the plugin. Dunno how likely this is.

> Splitting the above into three assertCompletionXXXX will make the tests
> more explicit about what they are checking. Keeping the checks about the
> format returned may be left here though IMHO.

Done, and it does look better. Thanks!

> but 'Approved' with the tweaks mentioned above.

Why the plural? I'd consider the assertion stuff a single tweak, and the
rest of your comments I felt were suggestions, perhaps for a future
merge proposal, but in any case not requirements for this merge here.

> On 04.05.2010 12:32, Vincent Ladeuil wrote:
> > For the sake of the discussion, imagine that we encounter a bug about an
> > option with a weird character like ' or `, whatever.
>
> The black-box tests are mostly about testing the fixed functionality
> part of the template, not individual commands. But I've just adjusted
> the get_script method to make it easier for future tests to provide
> cooked completion data without actually having to provide commands for
> these.

Ok, good enough for now.
<snip/>

> Once the plugin got merged, I'd adjust the README to state the fact, and
> point out that there are distinct lines of development. That much I'd
> write for the standalone plugin and merge into core in a separate merge
> request. So I'd like the README to stay a while, but get updated.

Fine for me, thanks for sharing your thoughts on the subject.

> I guess I'll keep the plugin around for about a year or so,

Sounds reasonable.

> Done, and it does look better. Thanks!

Excellent.

>
> > but 'Approved' with the tweaks mentioned above.
>
> Why the plural?

Because my English is not precise enough I suspect :)

> I'd consider the assertion stuff a single tweak,

Yup, that's what I was referring to.

 and the
> rest of your comments I felt were suggestions, perhaps for a future
> merge proposal, but in any case not requirements for this merge here.

Correct.

I'm going to assume that someone else has done a more thorough review of the bash side of things, as I don't really know how things integrate together.

The test infrastructure looks decent.

I do wonder about using "os.access('/bin/bash', X_OK)" rather than just:

subprocess.Popen(['bash', '-c', 'echo hello'], shell=True)

Partially because I just tested it, and under Windows if you have a bash.exe in your path, this succeeds. And in theory, that means that you could run the tests on all platforms, rather than requiring a specific path to be available.

That can be updated/added later, though.

Note that if I hack in that 'bash' is the path that can be accessed, most of the tests pass on Windows except for a couple. However, they're really quite slow:

...lugins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_commit_dashm OK 2163ms
...plugins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_global_opts OK 3039ms
...ugins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_init_format_2 OK 1719ms
...ins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_init_format_any OK 1786ms
...b.plugins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_init_opts OK 1808ms
...lugins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_simple_scipt OK 65ms
...gins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_status_negated OK 1822ms
..._completion.tests.test_bashcomp.TestBashCompletionInvoking.test_revspec_tag_all FAIL 3407ms
    Text attachment: log
------------
15.616 creating repository in file:///C:/users/jameinel/appdata/local/temp/testbzr-06_rhc.tmp/.bzr/.
15.632 creating branch <bzrlib.branch.BzrBranchFormat7 object at 0x01FA9110> in file:///C:/users/jameinel/appdata/local/temp/testbzr-06_rhc.tmp/
15.725 opening working tree 'C:/users/jameinel/appdata/local/temp/testbzr-06_rhc.tmp'
15.999 creating repository in file:///C:/users/jameinel/appdata/local/temp/testbzr-06_rhc.tmp/nInvoking.test_revspec_tag_all/work/.bzr/.
16.180 creating branch <bzrlib.branch.BzrBranchFormat6 object at 0x033F5E50> in file:///C:/users/jameinel/appdata/local/temp/testbzr-06_rhc.tmp/nInvoking.test_revspec_tag_all/work/
16.292 opening working tree 'C:/users/jameinel/appdata/local/temp/testbzr-06_rhc.tmp/nInvoking.test_revspec_tag_all/work'
------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "c:\Python26\lib\site-packages\testtools-0.9.2-py2.6.egg\testtools\runtest.py", line 128, in _run_user
    return fn(*args)
  File "c:\Python26\lib\site-packages\testtools-0.9.2-py2.6.egg\testtools\testcase.py", line 368, in _run_test_method
    testMethod()
  File "C:\Users\jameinel\dev\bzr\jam-integration\bzrlib\plugins\bash_completion\tests\test_bashcomp.py", line 197, in test_revspec_tag_all
    self.assertCompletionEquals('tag1', 'tag2', '3tag')
  File "C:\Users\jameinel\dev\bzr\jam-integration\bzrlib\plugins\bash_completion\tests\test_bashcomp.py", line 97, in assertCompletionEquals
    self.assertEqual(set(words), self.completion_result)
AssertionError: not equal:
a = set(['3tag', 'tag1', 'tag2'])
b = set()

I'm cer...

In a VirtualBox guest on the same hardware, I can confirm that they run better, even if they are still a bit slow:
...letion.tests.test_bashcomp.TestBashCodeGen.test_bzr_version OK 32ms
...etion.tests.test_bashcomp.TestBashCodeGen.test_command_case OK 8ms
...tion.tests.test_bashcomp.TestBashCodeGen.test_command_cases OK 1ms
...tion.tests.test_bashcomp.TestBashCodeGen.test_command_names OK 2ms
...etion.tests.test_bashcomp.TestBashCodeGen.test_debug_output OK 1ms
...ion.tests.test_bashcomp.TestBashCodeGen.test_global_options OK 3ms
...pletion.tests.test_bashcomp.TestBashCompletion.test_cmd_ini OK 685ms
...on.tests.test_bashcomp.TestBashCompletion.test_commit_dashm OK 680ms
...ion.tests.test_bashcomp.TestBashCompletion.test_global_opts OK 682ms
...n.tests.test_bashcomp.TestBashCompletion.test_init_format_2 OK 649ms
...tests.test_bashcomp.TestBashCompletion.test_init_format_any OK 676ms
...etion.tests.test_bashcomp.TestBashCompletion.test_init_opts OK 664ms
...on.tests.test_bashcomp.TestBashCompletion.test_simple_scipt OK 38ms
....tests.test_bashcomp.TestBashCompletion.test_status_negated OK 649ms
...st_bashcomp.TestBashCompletionInvoking.test_revspec_tag_all OK 985ms
...bashcomp.TestBashCompletionInvoking.test_revspec_tag_prefix OK 894ms
...pletion.tests.test_bashcomp.TestDataCollector.test_commands OK 623ms
...ion.tests.test_bashcomp.TestDataCollector.test_commit_dashm OK 8ms
...n.tests.test_bashcomp.TestDataCollector.test_global_options OK 1ms
...tion.tests.test_bashcomp.TestDataCollector.test_init_format OK 9ms
...n.tests.test_bashcomp.TestDataCollector.test_status_negated OK 6ms

----------------------------------------------------------------------
Ran 21 tests in 7.347s

However they:
 a) pass
 b) run in a reasonable speed

So I'm probably okay with this.

I'll mark this as ready, but give it a day or two in case people want to comment. (Especially on whether we want to try to run the tests on more platforms. Somebody should also probably try to test it on Mac...)

Just for the record, I ran this on my MacBook. All the test passed, and it took about 11 seconds for all the tests to run.

On 18.05.2010 22:48, John A Meinel wrote:
> I do wonder about using "os.access('/bin/bash', X_OK)" rather than just:
> subprocess.Popen(['bash', '-c', 'echo hello'], shell=True)
> And in theory, that means that you could run the tests on all platforms, rather than requiring a specific path to be available.
> That can be updated/added later, though.

lp:~gagern/bzr/bash_completion-ExecutableFeature has the change from
hardcoded paths to generic PATH environment variable.
I'm avoiding Popen(shell=True) to avoid executing yet another shell
process just to do the path resolution.

> most of the tests pass on Windows except for a couple. However, they're really quite slow:

Ouch! Wouldn't have expected this.

One bad solution would be skipping these tests on windows, or depending
on some environment setting, or some such hack. Feels bad, as it reduces
test coverage for performance reasons only.

In the short run, keeping the number of test cases actually executing
bash is probably the best solution. In the long run, it might be
feasible to execute all of these tests from a single bash instance, but
ensuring proper isolation under these circumstances could be tricky.

> ..._completion.tests.test_bashcomp.TestBashCompletionInvoking.test_revspec_tag_all FAIL 3407ms

That one deserves a closer look. Filed bug #582538 for this.

> I'm certainly hesitant to be adding tests that take multiple seconds to run. Though it may just be a win32 bash thing. Can someone else run "bzr selftest -s bp.bash" and let me know?

What alternatives do you have in mind? Not testing the handwritten bash
code seems like a poor alternative.

> The README clearly doesn't apply as-is anymore.

Will adjust that for both the in-tree and the standalone version at a
later point in time. Will submit a new merge proposal for that.

Thanks for the review, looking forward for the merge.

submitted to PQM by hand.