Take a stab at PROMPT_COMMAND #414

jyn514 · 2019-07-07T17:47:05Z

Location information is wrong - PROMPT_COMMAND=';' gives a traceback
pointing to ~/.local/oil/oshrc instead of [ interactive ] or
[ PROMPT_COMMAND ]

Other than that, this seems to be working pretty well with my patched version of bash-git-prompt.

Addresses #400

Location information is wrong - PROMPT_COMMAND=';' gives a traceback pointing to ~/.local/oil/oshrc instead of [ interactive ] or [ PROMPT_COMMAND ] Addresses oils-for-unix#400

andychu · 2019-07-07T18:28:42Z

I think this needs to be similar in spirit to WordParser.EvalForPlugin() and Tracer in core/dev.py.

PS4 is evaluating something for every line for set -x. And PROMPT_COMMAND is for every line of the interactive shell.

For example I believe it should avoid changing $? -- I think you reported that bug with $PS1 or something.

That said, if you can add a test case in spec/interactive.test.sh, and make it pass for bash and osh, then we can merge that and figure out all the details in a subsequent change. (e.g. followups for location info.)

- Don't let command change status - Handle arrays. Previously, osh would crash on `PROMPT_COMMAND=(1 2 3)`

jyn514 · 2019-07-07T19:45:12Z

My local .profile is messing up the tests for bash. Would you be willing to consider an ignored --noprofile argument for osh? I didn't open an issue since it would basically only be useful for tests.

andychu · 2019-07-07T19:49:29Z

I usually do something like:

case $SH in *bash) flag='--noprofile' ;; esac
$SH $flag -i ...

Does that work?

jyn514 · 2019-07-07T20:02:31Z

Hmm, that worked which makes me confused why the tests aren't passing. I'm running test/spec.sh interactive and bash fails all the tests. The weird thing is if I run the command manually, without going through test/spec.sh, it works fine. Here's the test case I'm looking at:

#### fatal errors continue

# NOTE: tried here doc, but sys.stdin.isatty() fails.  Could we fake it?
case $SH in *bash) flag='--noprofile';; esac
$SH $flag -i -c '
echo $(( 1 / 0 ))
echo one
exit 42
'
## status: 42
## STDOUT:
one
## END

Running the command through spec.sh:

$ test/spec.sh interactive -v
interactive.test.sh
case    line    dash    bash    mksh    osh
  0       4     N-I     FAIL    N-I     pass    'exit' in oshrc (regression)
[bash stdout] Expected 'one\n', got 'To run a command as administrator (user "root"), use "sudo <command>".\nSee "man sudo_root" for details.\n\none\n'
bash stdout:
To run a command as administrator (user "root"), use "sudo <command>".
See "man sudo_root" for details.

one

Running manually:

$ /bin/bash --noprofile --rcfile /home/joshua/Documents/Programming/oil/_tmp/spec-tmp/interactive.test.sh/oshrc -i -c 'echo hello'
one

andychu · 2019-07-08T00:07:44Z

OK weird, I reproduced this on another machine. Let me see what's going on!

andychu · 2019-07-08T00:09:27Z

Weirdly it works on Travis:

https://travis-ci.org/oilshell/oil/builds/555543685

andychu · 2019-07-08T00:13:33Z

I don't understand why /etc/bash.bashrc is being run... but somehow it is... still looking.

https://askubuntu.com/questions/22607/remove-note-about-sudo-that-appears-when-opening-the-terminal

andychu · 2019-07-08T00:18:32Z

OK I think this is system bash vs. hermetic bash from test/spec-bin.sh. Although Travis maybe has a newer version of Ubuntu.

I think Ubuntu must have patched bash to run /etc/bash.bashrc even if --rcfile is passed?? That is annoying. And maybe they got rid of it in a later version of Ubuntu.

andy@lisa ~/git/oilshell/oil (master)$ bash --version
GNU bash, version 4.3.48(1)-release (x86_64-pc-linux-gnu)

andy@lisa ~/git/oilshell/oil (master)$ _tmp/spec-bin/bash --version
GNU bash, version 4.4.0(1)-release (x86_64-unknown-linux-gnu)
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

Here's my Ubuntu:

andy@lisa ~/git/oilshell/oil (master)$ cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=16.04
DISTRIB_CODENAME=xenial
DISTRIB_DESCRIPTION="Ubuntu 16.04.2 LTS"

andychu · 2019-07-08T00:19:59Z

I mention the spec-bin stuff here, along with issue #159

https://github.com/oilshell/oil/wiki/Spec-Tests

I guess you just have to compile bash once, and it should go away. And it's better to do that anyway so all the rest of the tests pass. There are definitely differences between bash 4.3 and bash 4.4, so if your system has 4.3 it's worth the time.

- Set $HOME variable so bash doesn't print message about sudo. See oils-for-unix#414 (comment) - Don't source startup files, they could print a message (and in my case, did)

jyn514 · 2019-07-16T01:06:52Z

I set HOME instead and that seems to have fixed it. I do have 4.4 installed, I don't think it's a version error.

jyn514 · 2019-07-16T02:24:01Z

I added a test case, could you take another look?

core/main_loop.py

andychu · 2019-07-16T02:57:31Z

spec/interactive.test.sh

+case $SH in
+	*bash) echo "$TEST_CASE" | $SH --noprofile --norc -i;;
+	*osh) $SH --rcfile /dev/null -i -c "$TEST_CASE";;
+	*) $SH -i -c "$TEST_CASE";;


Hm also this seems to be doing a ton of stuff that's different for each shell, which makes the validity of the test hard to gauge at a glance.

Are you trying to avoid the spurious stdout from bash?

If doing the test/spec-bin.sh stuff in your environment fixes it, then please do that instead.

I added test/spec.sh interactive on Travis, and it doesn't have that problem. For now that is sort of the "golden environment".

If for some reason you can't get a _tmp/spec-bin/bash, or the instructions don't make sense, let me know.

Yes that should have been commented. This isn't an issue with spurious output, this is because osh and bash have different ideas of what it means to be interactive.

bash -i -c 'PROMPT_COMMAND="echo hi"' does not give any output, but echo 'PROMPT_COMMAND="echo hi"'' | bash -i does.

osh goes the other way and prints the prompt:

$ echo 'PROMPT_COMMAND="echo hi"' | osh -i osh$ hi osh$ ^D $

I agree not having the same test for both isn't ideal but I didn't see another way to check this.

jyn514 · 2019-07-17T01:05:56Z

Speaking of exceptions, this patch crashes osh if PROMPT_COMMAND throws an exception:

(osh) ~/.../Programming/oil ▶️ echo 'PROMPT_COMMAND=""; :' | bin/^C
(osh) ~/.../Programming/oil ▶️ PROMPT_COMMAND='which which'
/usr/bin/which
(osh) ~/.../Programming/oil ▶️ prlimit --nproc=1 --pid=$$
osh I/O error: Resource temporarily unavailable
(bash) ~/src/oil ▶️

I'm looking into how EvalForPlugin works.

jyn514 · 2019-07-17T01:23:29Z

I'm not sure how to treat the word returned by EvalForPlugin as a command substitution, I have the following code but it just prints the word instead of running a command.

  w_parser = parser.parse_ctx.MakeWordParserForPlugin(command)
  try:
    ps1_word = w_parser.ReadForPlugin()
  except util.ParseError as e:
    ps1_word = word.ErrorWord("<ERROR: Can't parse PS1: %s>", e)
    _parse_cache[ps1_str] = ps1_word

  # Evaluate, e.g. "${debian_chroot}\u" -> '\u'
  print(ex.word_ev.EvalForPlugin(ps1_word).s)

andychu · 2019-07-17T01:42:35Z

I think that behavior is OK for now.

I will probably do some refactoring, i.e. instead of _Eval, add something like RunFuncForCompletion. But if some new tests pass, then I won't worry too much about the details.

OK thanks for letting me know about the bash/osh difference. Please add a comment about that in the test.

My main issue now is setting $HOME because your version of bash is reading a file in ~. That is sort of regressing against issue $42.

We could set $HOME to something _tmp/spec-tmp/dummy-home, but it shouldn't be the actual HOME dir, which can have arbitrary files.

I think it will pass on Travis without that, so please remove it for now, and then we can figure out what to do if it's still failing on your machine.

Avoids dependence on files in HOME. Requested in oils-for-unix#414 (comment)

jyn514 · 2019-07-17T02:02:08Z

Ok I took HOME out, let me know if there's any more changes I should make. ~~The tests are failing locally, I'm going to try building bash from source finally.~~

Yup, building from source fixed it. Maybe we should add that to the wiki?

andychu · 2019-07-17T04:09:05Z

Great, yes feel free to update the wiki!

andychu · 2019-07-17T04:32:40Z

BTW I got the tests working roughly the same under bash/OSH:

82d22cc

Thanks for adding this!

Take a stab at PROMPT_COMMAND

825988d

Location information is wrong - PROMPT_COMMAND=';' gives a traceback pointing to ~/.local/oil/oshrc instead of [ interactive ] or [ PROMPT_COMMAND ] Addresses oils-for-unix#400

Clean up PROMPT_COMMAND

e7dac79

- Don't let command change status - Handle arrays. Previously, osh would crash on `PROMPT_COMMAND=(1 2 3)`

Get interactive tests to pass

cb8b5c8

- Set $HOME variable so bash doesn't print message about sudo. See oils-for-unix#414 (comment) - Don't source startup files, they could print a message (and in my case, did)

Add type comment for travis

6ff5823

Add test case for PROMPT_COMMAND

1824708

andychu reviewed Jul 16, 2019

View reviewed changes

core/main_loop.py Outdated Show resolved Hide resolved

core/main_loop.py Outdated Show resolved Hide resolved

andychu reviewed Jul 16, 2019

View reviewed changes

jyn514 added 2 commits July 16, 2019 21:03

Don't execute PROMPT_COMMAND unless it's a string

0480130

Use try/finally to restore status

3fd0ca6

Remove HOME variable from spec tests

2e48f67

Avoids dependence on files in HOME. Requested in oils-for-unix#414 (comment)

andychu merged commit 54272e8 into oils-for-unix:master Jul 17, 2019

jyn514 deleted the feature/prompt-command branch July 17, 2019 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take a stab at PROMPT_COMMAND #414

Take a stab at PROMPT_COMMAND #414

jyn514 commented Jul 7, 2019

andychu commented Jul 7, 2019

jyn514 commented Jul 7, 2019

andychu commented Jul 7, 2019 •

edited

Loading

jyn514 commented Jul 7, 2019 •

edited

Loading

andychu commented Jul 8, 2019

andychu commented Jul 8, 2019

andychu commented Jul 8, 2019

andychu commented Jul 8, 2019

andychu commented Jul 8, 2019

jyn514 commented Jul 16, 2019 •

edited

Loading

jyn514 commented Jul 16, 2019

andychu Jul 16, 2019 •

edited

Loading

jyn514 Jul 17, 2019 •

edited

Loading

jyn514 commented Jul 17, 2019

jyn514 commented Jul 17, 2019

andychu commented Jul 17, 2019

jyn514 commented Jul 17, 2019 •

edited

Loading

andychu commented Jul 17, 2019

andychu commented Jul 17, 2019

Take a stab at PROMPT_COMMAND #414

Take a stab at PROMPT_COMMAND #414

Conversation

jyn514 commented Jul 7, 2019

andychu commented Jul 7, 2019

jyn514 commented Jul 7, 2019

andychu commented Jul 7, 2019 • edited Loading

jyn514 commented Jul 7, 2019 • edited Loading

andychu commented Jul 8, 2019

andychu commented Jul 8, 2019

andychu commented Jul 8, 2019

andychu commented Jul 8, 2019

andychu commented Jul 8, 2019

jyn514 commented Jul 16, 2019 • edited Loading

jyn514 commented Jul 16, 2019

andychu Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

jyn514 Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

jyn514 commented Jul 17, 2019

jyn514 commented Jul 17, 2019

andychu commented Jul 17, 2019

jyn514 commented Jul 17, 2019 • edited Loading

andychu commented Jul 17, 2019

andychu commented Jul 17, 2019

andychu commented Jul 7, 2019 •

edited

Loading

jyn514 commented Jul 7, 2019 •

edited

Loading

jyn514 commented Jul 16, 2019 •

edited

Loading

andychu Jul 16, 2019 •

edited

Loading

jyn514 Jul 17, 2019 •

edited

Loading

jyn514 commented Jul 17, 2019 •

edited

Loading