Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python bind for osh #2092

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Python bind for osh #2092

wants to merge 15 commits into from

Conversation

KingMob
Copy link
Collaborator

@KingMob KingMob commented Oct 7, 2024

No description provided.

@KingMob KingMob changed the title Kingmob/push mzoxrymlqqwp Python bind for osh Oct 13, 2024
@KingMob KingMob force-pushed the kingmob/push-mzoxrymlqqwp branch 2 times, most recently from f2cf1c4 to f8ff91a Compare October 27, 2024 12:17
@andychu
Copy link
Contributor

andychu commented Nov 5, 2024

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
Copy link
Collaborator Author

KingMob commented Nov 5, 2024

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
Copy link
Contributor

andychu commented Nov 5, 2024

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
Copy link
Contributor

andychu commented Nov 6, 2024

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
Copy link
Contributor

andychu commented Nov 6, 2024

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
Copy link
Collaborator Author

KingMob commented Nov 8, 2024

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants