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

Update shell integration versions #355

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elyscape
Copy link

This is dependent on gnachman/iterm2-website#51 being pushed out to the website.

@gnachman
Copy link
Owner

There are two problems:

  1. I'm not sure there's been a big enough change yet to force everyone to upgrade. It's a real hassle for people who ssh into a lot of different machines.
  2. For about 6 months bash did not advertise a version. I'd like to deal with that at the same time as bumping the numbers.

@elyscape
Copy link
Author

Makes sense, though I would argue that, at least in the case of Bash, it's worth upgrading, as (from what I can tell) the bash-preexec script was added in verison 6. Any suggestions on how to deal with item 2?

@gnachman
Copy link
Owner

Yeah, there was a big refactor for version 6, but it didn't fix serious problems. It was mostly for code cleanliness and fixed a few things around the edges (like dealing with unusual history settings better and not stepping on aliases).

Issue 2 is really hard to fix. The only way to detect it is to see that we get the usual escape sequences in the expected order without a version number report and then offer to fix it. But it will have false positives because some people customize programs like Julia's REPL to do the same thing. I'm open to suggestions since this idea kinda stinks.

@elyscape
Copy link
Author

elyscape commented Apr 3, 2018

Hmm. When the bash integration didn't advertise a version, did it still advertise that it was running bash? Do you have a particular commit I could look to see what the behavior might have been like at the time? I did a cursory search and couldn't find anything.

@gnachman
Copy link
Owner

gnachman commented Apr 3, 2018

Unfortunately the shell and version are in the same code, so they were lost at the same time. This is the responsible commit:
gnachman/iterm2-website@b041393#diff-ae57e6e0e89998b78701feb3b3124693

Note how the new version does not call iterm2_print_version_number.

@elyscape
Copy link
Author

I think I figured out a way to deal with this.

The bash shell integration script exports the variables ITERM_PREV_PS1 and ITERM_ORIG_PS1. As a result, these variables will show up in the environment of child processes. The data structure returned by the KERN_PROCARGS2 sysctl, after pointers to the arguments, contains a null value, pointers to the environment variables, and then another null value. So, if:

  • we have received the usual shell integration escape sequences
  • have not received an integration version advertisement
  • and, when the active job changes to a child process, it has ITERM_PREV_PS1 or ITERM_ORIG_PS1 in its environment variables

Then bash integration is enabled but didn't advertise the version. It's kinda ugly, but it should work.

@gnachman
Copy link
Owner

That's a good idea! The only open question is what do for users who are SSH'ed somewhere they've installed shell integration. It might be acceptable to just guess that if you get shell integration escape sequences without the version that it's bash. It'll annoy some Julia users and such, but I think it's a small proportion.

@elyscape
Copy link
Author

It'll annoy some Julia users and such, but I think it's a small proportion.

We could reach out to the TerminalExtensions.jl package and have them add some sort of shell version announcement, even if it's just a dummy value.

@gnachman
Copy link
Owner

Looks like they already do "\033]1337;ShellIntegrationVersion=1\007", so we may be in the clear.

@gnachman
Copy link
Owner

Ah shoot, they never call it. I filed an issue here: Keno/TerminalExtensions.jl#34

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