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

bash_it.sh: Require interactive shell #1533

Conversation

rico-chet
Copy link
Contributor

Previously, a fix was added for bad behavior when bash-it printed text
to the console in conjunction with protocols like SCP, as commit
83c44fa "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

Previously, a fix was added for bad behavior when `bash-it` printed text
to the console in conjunction with protocols like `SCP`, as commit
83c44fa "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.
@rico-chet
Copy link
Contributor Author

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.

@nwinkler
Copy link
Member

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

@davidpfarrell
Copy link
Contributor

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

@rico-chet
Copy link
Contributor Author

Re #1533 (comment)

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.

@nwinkler
Copy link
Member

nwinkler commented Apr 1, 2020

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?

@rico-chet
Copy link
Contributor Author

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.

@davidpfarrell
Copy link
Contributor

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

@cornfeedhobo
Copy link
Member

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

@rico-chet ping

@rico-chet
Copy link
Contributor Author

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

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.

@cornfeedhobo
Copy link
Member

cornfeedhobo commented Jun 14, 2020

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!

@gaelicWizard
Copy link
Contributor

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.

@NoahGorny NoahGorny closed this Dec 5, 2022
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.

6 participants