bash-it icon indicating copy to clipboard operation
bash-it copied to clipboard

bash_it.sh: Require interactive shell

Open rico-chet opened this issue 4 years ago • 13 comments

Previously, a fix was added for bad behavior when bash-it printed text to the console in conjunction with protocols like SCP, as commit 83c44fac "template/profile: Require interactive shell".

It did work for those users who replace their original ~/.bash_profile file with the one provided by bash-it, but not for those who merely append a bash-it call to their existing ~/.bash_profile file.

Fix the behavior for the latter case, too.

This serves as a preparation for #1501

rico-chet avatar Mar 29 '20 23:03 rico-chet

Apparently, tests are run in non-interactive mode, so they will need to be adapted or the business logic for interactive mode moved to a separate script so it can be tested.

rico-chet avatar Mar 30 '20 09:03 rico-chet

Thanks for providing this fix - can you please take a look at the broken test cases?

nwinkler avatar Mar 31 '20 06:03 nwinkler

Greetings All,

Question: Is it possible that, even in non-interactive mode, the user may be trying to do something that requires (at least parts of) bash-it to initialize?

I'm thinking something along the lines of expecting an alias or plugin to be configured? say, pyenv or somesuch?

Just wondering if this is something to be concerned about what with the user delegating so much of their startup routine to bash-it (per design).

NOTE: This PR seems to keep par with the current pattern introduced Here, but with the user no-longer having direct control over it, I thought it might be worth discussing.

Wondering if we might find something like this useful:

~/.bash_profile

# Hide bash-it output if shell is non-interactive
case $- in
	*i*) source "$BASH_IT"/bash_it.sh;;
	  *) source "$BASH_IT"/bash_it.sh 1>/dev/null 2>&1;;
esac

I tested this locally with a few shell scripts and it worked as expected.

Thoughts?

-DF

davidpfarrell avatar Mar 31 '20 16:03 davidpfarrell

Re https://github.com/Bash-it/bash-it/pull/1533#issuecomment-606743441

Just wondering if this is something to be concerned about what with the user delegating so much of their startup routine to bash-it (per design).

IMHO, that's something people shouldn't do.

Wondering if we might find something like this useful:

# Hide bash-it output if shell is non-interactive

I tested this locally with a few shell scripts and it worked as expected.

I found that SCP transfers were slow to start in such a setting because of all the time it takes to run bash_it.sh. IIRC, tab completion for scp on the client side was slowed down, too.

I lean towards a different approach where there is an external bash_it.sh which skips in non-interactive mode and an internal bash_it.sh which runs regardless of whether the shell is interactive or not.

The matters seem a bit more complex than initially thought, we'll probably need additional automated tests for a swift discussion. Next time, I will look into testing with bats.

rico-chet avatar Mar 31 '20 20:03 rico-chet

Thanks for discussing this - the SCP use case is pretty important, true. We should make sure that whatever we does not result in longer init times when doing things like SSH, SCP, etc.

I'm not sure I understand the external/internal approach - can you please provide some more details on that, @rico-chet?

nwinkler avatar Apr 01 '20 08:04 nwinkler

With the current PR, we have the issues that:

  1. Tests fail in CI due to non-interactive shell being used.
  2. Users who call from a custom ~/.bash_profilebash_it.sh in non-interactive shells and rely on e.g. plugins having effect will find their things broken.

The approach is to have:

  • public script (for external use, let's stick to bash_it.sh) preventing non-interactive shells from calling the:
  • private script (for internal use, let's call it bash_it_private.sh) which runs the business logic. It can be called directly to run tests from non-interactive CI and by users who want to use bash-it plugins in non-interactive mode.

Then, when printing is added to bash-it, users in 2.) would need to switch from sourcing bash_it.sh to bash_it_private.sh &>/dev/null, that'd be a breaking change for them. Common users would not be affected.

Alternatively, we could add an environment variable, e.g. BASH_IT_ENABLE_NON_INTERACTIVE (default to false) or BASH_IT_INTERACTIVE_ONLY (default to true) that would switch between the two modes.

rico-chet avatar Apr 01 '20 10:04 rico-chet

I would lean toward keeping one access point for users, ie. bash-it.sh.

I think there's value in supporting a concept of "interactive-only components" and could see configuration going one of two ways:

  • Always-Initiated Components
  • Interactive-Only Components

or

  • Always-Initiated Components
  • Non-Interactive-Only Components
  • Interactive-Only Components

The 2nd option offers more flexibility, but the 1st option most-likely matches real-world usage.

Themes and Completions seem to clearly fit into the "Interactive-Only Components" category.

Plugins and Aliases may be a bit more nuanced. These components could potentially tag themselves (using meta-data) as interactive or non by default.

I think the user should have the ability to override the default on a per-component basis. Maybe something like

BASH_IT_INITIATE_PLUGINS_ALWAYS=plugin1,plugin2...

This could give the user the ability to orchestrate both their interactive and non-interactive sessions.

The user could get creative in their bash_profile and add ssh_detection code and then control exactly which components should be initiated for ssh/scp sessions.

NOTE: Components are still expected to be enabled via bash-it enable ... so the overrides would only apply to enabled components (silently ignored for others).

Additionally, I think my idea of routing all output to /dev/null in non-interactive mode still has merit. Although we may want to place such logic behind a configuration option.

-DF

davidpfarrell avatar Apr 01 '20 17:04 davidpfarrell

I really like this idea of plugins having the option of being always on vs interactive only.

However, I kinda see this PR as a bug fix, since it sounds like this hasn't been thought out previously. I know I've had to wrap my bash-it includes with this clause and has annoyed other people I've gotten to adopt bash-it. In that vein, I think it'd be great if this PR could be merged (once travis is happy) and further discussion moved to an issue maybe? I hope I don't come off like a jerk, I'm just trying to keep things moving :-)

cornfeedhobo avatar May 18 '20 16:05 cornfeedhobo

@rico-chet ping

cornfeedhobo avatar May 29 '20 21:05 cornfeedhobo

To make Travis happy, we would need to figure out how to run the tests in CI pretending the shell is interactive, because otherwise they just fail. Shouldn't be hard but might have side effects.

A new test would be handy which proves that SCP scenarios work fine. I made some progress running SSH server in a docker container but failed to get the test red.

And the question of how we deal with existing expectations isn't really solved. Due to lack of knowledge of bash-it internals I cannot come up with a simple solution yet.

rico-chet avatar May 29 '20 21:05 rico-chet

For now, I found that it's complicated on the bash side already: run_startup_files() is full of ifs, ifdefs and flags, so no wonder I cannot get the SCP scenario test red. FYI, SSH_SOURCE_BASHRC is set in Debian and Fedora.

A quick attempt to make existing tests green wasn't successful, so it will take a while for me to get it done, if at all.

If anybody's interested to pick it up, feel free to do so. Since personally, I'm not affected by the issue and am spending my spare time on other activities recently, it's not going to be done soon otherwise.

rico-chet avatar Jun 12 '20 23:06 rico-chet

I'm in the same boat. If I find spare time or need a break from work, I might copy your work to a branch and see what I can pull off.

Edit: thanks for all the hard work and for taking the time to follow up!

cornfeedhobo avatar Jun 14 '20 19:06 cornfeedhobo

If I may add my $0.02, I think that Bash It isn't the right place for deciding if it should load itself whether interactive or not. This feels like something that should be handled in .bashrc by the user. I would be surprised to find anyone relying on something loaded by Bash It in non-interactive scripts, except possibly manipulating $PATH in their custom.bash. IMO, I don't think Bash It should be loaded in .bash_profile at all, but rather in .bashrc.

gaelicWizard avatar Jan 18 '22 23:01 gaelicWizard