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

[akinomyoga]

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]

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]

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]

/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.

[akinomyoga]

/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.

I understand that part, but the question is whether Lmod has requirements on the behavior of other modules. This part seems like the loading check for the Lmod module itself. You've suggested it as a good reference, but I'm wondering about the intent for the good reference.

• Did you mean just an example to test the root user?
• Or did you mean a kind of template that the other modules need to follow in order to work under Lmod?

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.

As far as I see your explanation, it just seems like a loader of the Lmod framework on /etc/profiles.d/modules.sh. Then, I think this should rather be maintained by the packages because /etc/profile.d/*.sh are controlled by packages but not managed in the upstream projects. This project bash-preexec.sh doesn't have control of /etc/profile.d/*.sh.

[LecrisUT]

• Did you mean just an example to test the root user?

Yes, just that. Just having an interface like *_ALLOW_ROOT_USE and such to have fine-grained control of these. E.g. in Fedora builds, you would need to override these so that you can still run Lmod during your builds. It might be appropriate to give such overrides here as well for interactive runs.

As far as I see your explanation, it just seems like a loader of the Lmod framework on /etc/profiles.d/modules.sh. Then, I think this should rather be maintained by the packages because /etc/profile.d/*.sh are controlled by packages but not managed in the upstream projects. This project bash-preexec.sh doesn't have control of /etc/profile.d/modules.sh.

Well, yes, we can carry the $- != *i* check as well. The only case to discuss is if we allow override mechanism like BASH_PREEXEC_ALLOW_NON_INTERACTIVE, in which case it should be standardized and carried upstream instead. And similarly for root user checks.

[akinomyoga]

• Did you mean just an example to test the root user?

Yes, just that.

Thank you for your clarification. I've been misunderstanding the intent.

As far as I see your explanation, it just seems like a loader of the Lmod framework on /etc/profiles.d/modules.sh. Then, I think this should rather be maintained by the packages because /etc/profile.d/*.sh are controlled by packages but not managed in the upstream projects. This project bash-preexec.sh doesn't have control of /etc/profile.d/modules.sh.

Well, yes, we can carry the $- != *i* check as well. The only case to discuss is if we allow override mechanism like BASH_PREEXEC_ALLOW_NON_INTERACTIVE,

The interactive check can be included in bash-preexec.sh because there is no use case of bash-preexec.sh in non-interactive shells. At least, nothing of bash-preexec.sh works in non-interactive shells even if it is loaded. In that sense, BASH_PREEXEC_ALLOW_NON_INTERACTIVE is also useless because nothing happens even if the user enables it.

And similarly for root user checks.

Compared to the variable for the non-interactive shells, the one for the root-user check may have use cases.

However, I'm not convinced by that specific option, so I'm currently not thinking of including it in this PR (Of course, we can continue the discussion in another PR). The root user that I suggested in #159 (comment) is just one example. The problem of forcibly loading bash-preexec.sh for all users is not specific to the root user. In shared systems where multiple different users log in, some users might want to load bash-preexec, but some other users wouldn't like it. The solution for the root user used by Lmod doesn't work for such general users.

@LecrisUT Do you request this PR to include the root user check?

I'm sorry to bother you, but I have to explicitly ask this because this project seems very strict about PRs; a PR won't be merged if PR has unresolved conversations. In this sense, every comment in PRs in bash-preexec is by default considered requests that should be fixed before merging. However, the check for the root user seems an unrelated topic to me, which shouldn't block the check for the interactive check.

[LecrisUT]

@LecrisUT Do you request this PR to include the root user check?

Deferred, I trust your judgement on whether it is relevant to have an override for the interactive is needed or not. The root check can be addressed separately.

[akinomyoga]

Deferred,

Thanks.

[akinomyoga]

@jparise I'd appreciate it if you could review this PR. In #152 (comment), the maintainers seem to require several external reviewers to leave reviews before merging (except for the trivial ones like #165), though there is no sufficient audience watching this repository. Recently, I'm the only reviewer regularly watching this repository.

[jparise]

This code looks good to me. I think it might be nice to mention __bp_inside_test in test/README.md because users who want to test bash-preexec.sh as part of their own tests would also need to set it.

Overall, though, I'm not clear if this is solving a known problem or if enforcing this restriction is more of a "correctness / precondition"?

[akinomyoga]

I think it might be nice to mention __bp_inside_test in test/README.md because users who want to test bash-preexec.sh as part of their own tests would also need to set it.

@jparise Thanks for the suggestion! That makes sense. I updated it (6b7a9be..acc0aa5).

Overall, though, I'm not clear if this is solving a known problem or if enforcing this restriction is more of a "correctness / precondition"?

I think even without this condition, there wouldn't be a real problem.

This is just for the cleanliness of the shell environment not to introduce random functions and variables that wouldn't be used. Some users put source bash-preexec.sh in their .bashrc or .bash_profile without the checking for the interactive shell, or even distributions' /etc/profile or /etc/bashrc sometimes source bash-preexec.sh in both interactive and non-interactive shells through /etc/profile.d/bash-preexec.sh.