rcaloras / bash-preexec

⚡ preexec and precmd functions for Bash just like Zsh.
MIT License
862 stars 94 forks source link

Do not enable `bash-preexec.sh` in non-interactive shells #160

Open akinomyoga opened 2 months ago

akinomyoga commented 2 months ago

From the discussion in #159:

Is it ok to say that bash-preexec.sh is designed to be able to run outside of the interactive mode as well?

As far as I can tell, bash-preexec.sh doesn't target the non-interactive mode. The concept of the hooks preexec and precmd is specific to the interactive use of the shell. Also, even if bash-preexec.sh is sourced, I think nothing would happen because PROMPT_COMMAND (which will invoke the precmd hook) is not active in the non-interactive shell, and the preexec will also never be executed because it is only enabled after a call of the precmd hook. In addition, the DEBUG trap will be called for every command, which would become a large overhead, although the trap doesn't do anything actually. Therefore, I think bash-preexec.sh should not be loaded in the non-interactive shell.

Maybe we can have an explicit check for the interactive shell at the beginning of bash-preexec.sh.

LecrisUT commented 2 months ago

Maybe it's worth making environment variable overrides. Here's a snippet from Lmod, and although not related to interactive shell, would be good reference:

if [ -z "${LMOD_ALLOW_ROOT_USE+x}" ]; then
  LMOD_ALLOW_ROOT_USE=yes
fi

( [ -n "${USER_IS_ROOT:-}" ] || ( [ "${LMOD_ALLOW_ROOT_USE:-}" != yes ] && [ $(id -u) = 0 ] ) ) && return
akinomyoga commented 2 months ago

Maybe it's worth making environment variable overrides. Here's a snippet from Lmod, and although not related to interactive shell, would be good reference:

if [ -z "${LMOD_ALLOW_ROOT_USE+x}" ]; then
  LMOD_ALLOW_ROOT_USE=yes
fi

( [ -n "${USER_IS_ROOT:-}" ] || ( [ "${LMOD_ALLOW_ROOT_USE:-}" != yes ] && [ $(id -u) = 0 ] ) ) && return

What's that? These lines seem to be set up by /etc/profiles.d/modules.sh (i.e., the entry point of Lmod) and do not seem to be something each module should perform. Does Lmod instruct every module to include those lines? Also, I'm not sure if we should include such a setting specific to the specific framework in the upstream codebase. Shouldn't they be added by the Fedora packages?

LecrisUT commented 2 months ago

/etc/profiles.d/modules.sh is loaded only once in order to define the module command. That section is a fallthrough to check if the file is sourced by the root and if there are overrides. This file is defined in /usr/share/lmod/lmod/init/profile and afaict, Lmod controls and installs that file.

Shouldn't they be added by the Fedora packages?

Of course, that would also be possible, but maybe some standardization of override environment variables should be considered if that would be added.