search

oils-for-unix / oils

Oils is our upgrade path from bash to a better language and runtime. It's also for Python and JavaScript users who avoid shell!
http://www.oilshell.org/
Other
2.85k stars 159 forks source link

Python `bind` for osh #2092

Closed KingMob closed 2 days ago

andychu commented 2 weeks ago

I ran the spec tests locally and got this -

case    osh     bash
Ignoring osh-cpp failure: 0 test bind (w/out flags) for adding bindings to readline fns
0       FAIL    ok      test bind (w/out flags) for adding bindings to readline fns
        0/5 ok
Ignoring osh-cpp failure: 1 test bind -r for removing bindings
1       FAIL    ok      test bind -r for removing bindings
        0/5 ok
Ignoring osh-cpp failure: 2 test bind -x for setting bindings to custom shell functions
2       FAIL    ok      test bind -x for setting bindings to custom shell functions
        0/5 ok
3       ok      ok      test bind -u for unsetting all bindings to a fn
4       ok      ok      test bind -q for querying bindings to a fn
5       ok      ok      test bind -m for setting bindings in specific keymaps
6       ok      ok      test bind -f for setting bindings from an inputrc init file

Is that what is expected to work?

The code looks reasonable, and this seems like good progress!

If so, I can help fix the C++ issues

KingMob commented 2 weeks ago

Is that what is expected to work?

Yes, this is what I expect. The failing tests don't have code implemented for them yet.

andychu commented 2 weeks ago

I pushed a couple minor commits to pass type checking and translation

So make sure to PULL before editing any code

It looks like we can get past the rest of the C++ build errors without much work too -- I will look at that

This looks VERY promising, thank you!

andychu commented 2 weeks ago

OK I pushed some stubs for C++ -- wasn't that bad, although there was one mycpp workaround necessary

Let's see what CI results looks like!

andychu commented 2 weeks ago

Oh weird there are some line_input.c compile errors that don't happen on my machine either

http://op.oilshell.org/uuu/github-jobs/8226/interactive.wwz/_tmp/soil/logs/py-all-and-ninja.txt

Maybe this is due to older GNU readline versions?

Maybe bash added a new flag recently that depends on a new version or something?

KingMob commented 2 weeks ago

Hmm, I haven't seen that error either. The rl_function_of_keyseq_len fn exists in my bash. I see it in lib/readline/readline.h and lib/readline/bind.c.

Looks like that particular fn was added six years ago for bash 5.0. I'm guessing python's readline wrapper uses an older version as its basis? Bash 4.4 used rl_bind_keyseq to do removal by passing a null ptr for the fn to bind to.

I'll look into what precise version python's readline was based off of. Using that as a basis is probably easier; bash/readline is old, but it's still changing, and I don't think I want to bring in the latest readline just for this. (Honestly, I'm not sure you want to use readline over a more modern lib, even without the license issue.)

andychu commented 4 days ago

Cool, I'm taking a look at this! I just merged master again

andychu commented 3 days ago

OK I made a whole bunch of minor changes, and I think this is ready to merge! Here are what the tests look like now

But I think this is great progress! Let me know what you think

andychu commented 3 days ago

Here's a summary of what I did

Let me know if you have any questions ... in general we have a boatload of tools that make this very mechanical

And I think concentrating on just Python is absolutely right for contributors, especially with something as hairy as bind. This code is unusual because it interfaces with an external library

Thanks for working on this! It DEFINITELY would not get done without your efforts. From working with this PR, I now know a little more about bind :-)

KingMob commented 3 days ago

Hmmm, looks like the new bind -q spec doesn't pass on non-cpp osh. Apparently, if it's not bound, it should write that to stdout, instead of stderr.

andychu commented 3 days ago

Yeah I noticed that, though it's arguable how much we have to match bash EXACTLY

i.e. we try to avoid "implementing bash bugs", which may be changed in later versions

For stdout/stderr, the heuristic I use is --

i.e. I think stderr can "break" / change wording, but stdout should be frozen

If it's consistent with the rest of OSH, then we could keep it on stderr

Not sure though

KingMob commented 3 days ago

Oh, well...I already changed it. In particular, it failed when bind -q is called on a valid function name, but one that's not currently bound.

Whether bash is mistaken in writing that to stdout instead, or in returning a non-zero status code...I dunno. It does seem like something that could be parsed, though.

KingMob commented 3 days ago

There's a lot of CI checks failing their publish-* tasks...

Anyway, I think my part on the current PR is done. I have nothing else I want to change.

andychu commented 3 days ago

To check the CI failures, I go here:

https://op.oilshell.org/uuu/github-jobs/

and then click on your job:

https://op.oilshell.org/uuu/github-jobs/8291/

and then if you click on the first failure, you'll see:

https://op.oilshell.org/uuu/github-jobs/8291/cpp-coverage.wwz/_tmp/soil/logs/compare-gcc-clang.txt

    mycpp pass: PROTOTYPES
builtin/readline_osh.py:172 Use explicit len(obj) or 'obj is not None' for mystr, mylist, mydict

This is a mycpp error, which says that you should not use implicit booleans, basically because those 2 things are different in C++ !

We do a literal translation, so you have to be specific about what you want

andychu commented 2 days ago

Merged, thank you! Please keep us updated on Zulip