fix(doctor): detect preexec plugin using env ATUIN_PREEXEC_BACKEND #1856

akinomyoga · 2024-03-11T12:33:46Z

Partial fix for #1849

This is one possible approach to solve #1849. Depending on what we want to specifically detect, we might need adjustments. For the implementation details, please see the commit message and changes. There is an extra commit 68a5d5a that fixes the function names and code comments for the current approach using shell -ic command and shell variables.

Discussion

I added a new environment variable ATUIN_PREEXEC_BACKEND in atuin/src/shell/atuin.bash to detect if any preexec backend is correctly set up for Atuin in the Bash session that executed atuin doctor. However, one might want to check the default shell setup but not the setup in the caller session. Currently, the shell name is determined by the shell that executed atuin doctor. In that sense, it would be natural to extract the setup of the session that executed atuin doctor. However, the existing code seems to try to extract the setup of the user's default setup of the shell.

Another possibility is to prepare another section for the preexec backend than the plugin section. Or, we may include the preexec-backend information in the plugin string of atuin, such as atuin (preexec: blesh-0.4.0) or atuin (preexec: bash-preexec).

Checks

I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
I have checked that there are no existing pull requests for the same thing

akinomyoga · 2024-03-11T12:37:15Z

We may need checks from the Nushell and Xonsh people about whether the added line doesn't break the configuration.

akinomyoga · 2024-03-11T12:44:31Z

Plugin Detection in ble.sh

ble.sh also has some Bash plugin detection for diagnostics, which typically checks the existence of a shell function for each plugin. Maybe I can port the list of function names that ble.sh uses for the plugin detection.

ellie · 2024-03-11T12:51:10Z

atuin/src/command/client/doctor.rs

+        let mut vec = Vec::new();
+        if ShellInfo::shellvar_exists(shell, "ATUIN_SESSION") {
+            vec.push("atuin".to_string());
+            match env::var("ATUIN_PREEXEC_BACKEND") {


Can we keep this in the previous map? I'm not sure why we need to start special casing things here.

I'd rather we have a single path that works for all (var/env) rather than special casing of each.

Can I defer the updating until we settle the approach?

As far as the current approach of this PR is used, we need branching somewhere. If we would try to put everything in HashMap, we need to perform the branching inside the for-loop for HashMap and also need to introduce the mechanism to resolve the dependencies between different plugins, which seem overengineering.

Can I defer the updating until we settle the approach?

Sure

introduce the mechanism to resolve the dependencies between different plugins

The intent was never to represent the dependency between Atuin and some preexec method. Simply to list all installed plugins, regardless of dependency.

Hence, later on, we run logic over the list of dependencies and use that to suggest problems.

I'd like to be able to know if someone has preexec installed independently of whether they have the Atuin shell plugin.

ellie · 2024-03-11T12:51:58Z

atuin/src/shell/atuin.zsh

@@ -27,6 +27,8 @@ fi
 export ATUIN_SESSION=$(atuin uuid)
 ATUIN_HISTORY_ID=""

+export ATUIN_PREEXEC_BACKEND=


I'm not super happy about adding this to anything other than bash

This is needed as described in the commit message.

Or do you have an idea to avoid setting the environment variable in other shells? For example, another approach could be to include the process ID of Bash in even another environment variable ATUIN_BASH_PID and check if the PPID of Atuin matches the recorded Bash process ID.

I think this makes more sense as a change to ble. There may well be other plugins that require some way of detecting that they're running with ble installed.

I'd rather not have to work around the lack of any method to reliably detect ble being installed in this way, especially if this workaround leaks to other shell implementations.

Otherwise we could load up the users shell file and grep it for ble install

ellie · 2024-03-11T12:54:48Z

Is it possible for ble to start exporting a var we can detect? I'm happy to update the doctor guidance to suggest that ble cannot be auto-detected for now.

I'm not sure about such a large change to workaround ble not doing this. I'd also prefer such changes happen purely to the bash implementation, if we must.

akinomyoga · 2024-03-11T13:33:58Z

Is it possible for ble to start exporting a var we can detect? I'm happy to update the doctor guidance to suggest that ble cannot be auto-detected for now.

It would be technically possible to make it work with the current specific implementation of Atuin, but my current feeling is that I wouldn't like to add such a thing just to make Atuin's hack work. The current design of ble.sh is based on a policy that it doesn't leave any change to the shell environment when ble.sh is sourced in a session that is not supposed to provide line editing. This is needed because .bashrc can be evaluated in sessions that don't require line editing (including the sessions for Window Manager and startup scripts for other software). The request is essentially to pollute the environment for a session that is not supposed to enable ble.sh, which contradicts the current policy.

I'm not sure about such a large change to workaround ble not doing this.

In Atuin's perspective, what does exceptional handling might be blesh, but it's the opposite in blesh's perspective. Rather, I feel Atuin's implementation is still a hack that could possibly be broken by other factors while I believe ble.sh is doing the right thing.

Also, the approach that we should take depends on what we would like to test with atuin doctor. I was not sure about it so asked in the "Discussion" section in the cover description of this PR. Did you read that? Could you read the "Discussion" section and tell me what you think atuin doctor should detect specifically?

I'd also prefer such changes happen purely to the bash implementation, if we must.

I actually thought about it initially, but the current shell detection is incomplete. The Bash process name is not necessarily bash, but could be sh or also bash-5.1, bash-dev, etc. Also, in the current main branch, BLE_ATTACHED and bash_preexec_imported are tested regardless of the shell. I decided to perform the check regardless of the shell for this reason.

However, if we only support atuin doctor for the program name bash (and discard the support for sh or the other), I can add that check. For bash-<version>, I think we can test it by shell.starts_with("bash") instead of shell == "bash".

ellie · 2024-03-11T13:51:36Z

just to make Atuin's hack work

We actually wouldn't need a hack if we could just

env::var("BLESH_IS_INSTALLED").is_some()

Or similar. Such as this section from the bash-preexec readme: https://github.com/rcaloras/bash-preexec?tab=readme-ov-file#library-authors

(yes I'm aware that's a shell var, but the point still stands that detecting blesh is really difficult)

However because this isn't possible, we resort to elaborate workarounds. Like spawning an interactive subshell. I may see about spawning a pty - does blesh try and dodge that too?

This is needed because .bashrc can be evaluated in sessions that don't require line editing

This makes sense.

The request is essentially to pollute the environment for a session that is not supposed to enable ble.sh, which contradicts the current policy.

I'm starting an interactive subshell purely because some shell plugins do not provide any other alternatives. If there was any way at all for me to detect that blesh was installed, I'd take that.

Did you read that? Could you read the "Discussion" section and tell me what you think atuin doctor should detect specifically?

Sorry - I read it, but didn't see any questions relevant to the current discussion.

atuin doctor should detect the plugins used in the current shell. The goal here is purely to prompt new installers if they've misread the docs and guide them on what to do - literally because in the past 24hrs alone the number of issues opened is becoming unmanageable, and I'd like to reduce the easy cases.

I'm not particularly bothered if it's the default shell or the calling shell, hence my lack of discussion here. I'm trying to solve the current issue in focus - which is that detecting if blesh is in use seems to be very difficult.

ellie · 2024-03-11T13:59:00Z

Anyway I feel a lot of resistance to changing anything there with blesh, so don't worry about it.

I'll update the help message with the doctor for this release, while we continue to find a suitable workaround

ellie · 2024-03-11T14:28:19Z

see: #1858

If this ends up being the only possible way to solve the issue then it's acceptable, however one of the following would make things much much easier

An env var from ble, available within an interactive bash session that can be detected by a user running the atuin doctor command
A shell var that is available within an interactive subprocess (started with bash -ic). Sounds like this is against your goals, so no bother there.

Otherwise even a file we could look for, or... something, might be ok.

akinomyoga · 2024-03-11T16:12:20Z

Such as this section from the bash-preexec readme: https://github.com/rcaloras/bash-preexec?tab=readme-ov-file#library-authors

bash-preexec is not as intrusive as ble.sh. ble.sh takes over its entire input stream and may cause serious problems when it's enabled in an unexpected way. That's one of the reason that ble.sh needs to be more careful.

(yes I'm aware that's a shell var, but the point still stands that detecting blesh is really difficult)

However because this isn't possible, we resort to elaborate workarounds. Like spawning an interactive subshell. I may see about spawning a pty - does blesh try and dodge that too?

In that case, ble.sh wouldn't try to cancel its loading.

I'm starting an interactive subshell purely because some shell plugins do not provide any other alternatives. If there was any way at all for me to detect that blesh was installed, I'd take that.

Yeah, and that actually depends on what we would like to specifically detect (see below).

Sorry - I read it, but didn't see any questions relevant to the current discussion.

Sorry for not being clear about the question. What I am unsure of is whether we want to check the default shell setup by the user described in ~/.bashrc or the actual shell setup of the session that executed atuin doctor.

Bash can be started with a custom setup with bash --rcfile <custom init file>. This is actually the way that I test atuin with different settings. I wouldn't directly modify my ~/.bashrc to test different sets of the setups. If atuin doctor only sees the settings in ~/.bashrc by starting an independent session by bash -ic ..., that may cause confusion. I feel the user is likely to test the setup of the session where atuin doctor is loaded, so I feel it's better to reference the environment variable, if present, exported in the session that executed atuin doctor,

In addition, there are different levels of detections: whether blesh is installed or not, whether blesh is loaded in the session, whether blesh is in the attached state, and whether Atuin's hooks are properly set into blesh. The proper approach depends on all those things, and the implementation for some could be even more complicated. For that reason, I wanted to confirm it to avoid implementing every possible detection.

atuin doctor should detect the plugins used in the current shell.

I think that in addition, we would like to check whether it is correctly set up to work with Atuin. Even if ble.sh is loaded and enabled, it doesn't work if Atuin's shell configuration doesn't set blehook. The current version of PR handles this. Only when ble.sh and Atuin integration is properly set up, it shows blesh as an enabled plugin.

I'm not particularly bothered if it's the default shell or the calling shell, hence my lack of discussion here. I'm trying to solve the current issue in focus - which is that detecting if blesh is in use seems to be very difficult.

If so, I'd suggest detecting the settings of the shell session that executed atuin doctor, if possible, and detect shell variables in bash -ic command only when it's not possible. That is the necessary complication.

see: #1858

If this ends up being the only possible way to solve the issue then it's acceptable, however one of the following would make things much much easier

An env var from ble, available within an interactive bash session that can be detected by a user running the atuin doctor command

That is one possibility that I had in my mind, but that was related to the question of what to specifically detect. ble.sh defines a shell variable BLE_SESSION_ID='<start_time>/<PID>'. If it is changed to be exported, Atuin can detect ble.sh by checking whether atuin doctor's PPID or PGID matches the <PID> in BLE_SESSION_ID.

However, it just detects whether ble.sh is loaded in the session and doesn't ensure that it's enabled. Also, it doesn't ensure that Atuin's integration is properly set up for blesh.

A shell var that is available within an interactive subprocess (started with bash -ic). Sounds like this is against your goals, so no bother there.

If there would be a convincing argument, I might think about leaving a variable to show that loading ble.sh is attempted, but so far I don't think it is necessary. I think there are still other better approaches depending on the goal of what to specifically detect.

akinomyoga · 2024-03-11T16:13:45Z

Based on those pieces of feedback, I'll create and push a next version tomorrow.

ellie · 2024-03-12T17:44:39Z

I appreciate that in order to cover every possible blesh setup issue, we'd need to handle everything you say.

But that's not really the goal here. The goal is to handle the case where a new user installs atuin, but installs neither bash-preexec nor blesh. In all cases so far, they are aware of neither, and they are not particularly aware of why they are needed.

In that case, we want to prompt them to do so + link to that specific section of the docs. Maybe in the future we can also detect a variety of other blesh states that could cause issues, but right now I get a lot of people asking why bash isn't recording their history, when the reason is that they haven't installed what they need to.

What I am unsure of is whether we want to check the default shell setup by the user described in ~/.bashrc or the actual shell setup of the session that executed atuin doctor.

I don't feel strongly about either approach. The latter is probably ideal, but if the former is significantly simpler then it's the approach I'd prefer.

There's certainly edge cases not handled by the former, but they're not something I'd worry that much about given the goal I described above.

If so, I'd suggest detecting the settings of the shell session that executed atuin doctor, if possible, and detect shell variables in bash -ic command only when it's not possible. That is the necessary complication.

Totally happy with this. The current implementation is only as-is due to lack of any available env vars.

whether blesh is installed or not, whether blesh is loaded in the session, whether blesh is in the attached state, and whether Atuin's hooks are properly set into blesh.

My main concern here is whether or not blesh is installed. Detecting the other issues might be nice in the future, but the issue I'm trying to solve sooner is "user hasn't installed anything enabling us to hook bash".

In this patch, if the plugin provides an environment variable, we use the environment variable to test the existence of the plugin. When an environment variable is not available, we continue to use the mock interactive session by "shell -ic command". We also test shell-specific plugins only in the corresponding shells. An additional test can be performed by a custom function for each plugin.

ellie

Looks good to me, thank you!

akinomyoga · 2024-04-02T08:08:38Z

Thanks!

akinomyoga · 2024-04-02T08:10:28Z

I actually haven't yet pushed the version of ble.sh that exports BLE_SESSION_ID, but I'll push it in the next push to the master. Anyway, this PR contains an adjustment for an older ble.sh that doesn't export BLE_SESSION_ID, so it should anyway work properly.

…tuinsh#1856) * refactor(doctor): update func names and desc to match current impl * fix(doctor): use environment variable to detect plugin if possible In this patch, if the plugin provides an environment variable, we use the environment variable to test the existence of the plugin. When an environment variable is not available, we continue to use the mock interactive session by "shell -ic command". We also test shell-specific plugins only in the corresponding shells. An additional test can be performed by a custom function for each plugin.

* fix: install script echo * fix(nu): Update atuin.nu to resolve 0.92 deprecation (#1913) * feat(install): Update install.sh to support KDE Neon (#1908) KDE Neon is based on Ubuntu 22.04, but the OS List for Ubuntu-based distros does not have the string "neon". This commit adds it. * chore(deps): bump lukemathwalker/cargo-chef (#1901) Bumps lukemathwalker/cargo-chef from latest-rust-1.76.0-buster to latest-rust-1.77.0-buster. --- updated-dependencies: - dependency-name: lukemathwalker/cargo-chef dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): flake.lock: Update (#1910) Flake lock file updates: • Updated input 'flake-utils': 'github:numtide/flake-utils/d465f4819400de7c8d874d50b982301f28a84605' (2024-02-28) → 'github:numtide/flake-utils/b1d9ab70662946ef0850d488da1c9019f3a9752a' (2024-03-11) • Updated input 'nixpkgs': 'github:NixOS/nixpkgs/ebe6e807793e7c9cc59cf81225fdee1a03413811' (2024-02-29) → 'github:NixOS/nixpkgs/807c549feabce7eddbf259dbdcec9e0600a0660d' (2024-03-29) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix(doctor): detect preexec plugin using env ATUIN_PREEXEC_BACKEND (#1856) * refactor(doctor): update func names and desc to match current impl * fix(doctor): use environment variable to detect plugin if possible In this patch, if the plugin provides an environment variable, we use the environment variable to test the existence of the plugin. When an environment variable is not available, we continue to use the mock interactive session by "shell -ic command". We also test shell-specific plugins only in the corresponding shells. An additional test can be performed by a custom function for each plugin. * chore(deps): bump sysinfo from 0.30.6 to 0.30.7 (#1888) Bumps [sysinfo](https://github.com/GuillaumeGomez/sysinfo) from 0.30.6 to 0.30.7. - [Changelog](https://github.com/GuillaumeGomez/sysinfo/blob/master/CHANGELOG.md) - [Commits](GuillaumeGomez/sysinfo@v0.30.6...v0.30.7) --- updated-dependencies: - dependency-name: sysinfo dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Ivan Toriya <toriya@precisdigital.com> Co-authored-by: Wind <WindSoilder@outlook.com> Co-authored-by: Diego Carrasco Gubernatis <557703+dacog@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Koichi Murase <myoga.murase@gmail.com>

ellie reviewed Mar 11, 2024

View reviewed changes

akinomyoga force-pushed the bash-backend branch 3 times, most recently from 3deab6b to 68780bd Compare March 14, 2024 14:25

akinomyoga force-pushed the bash-backend branch from 68780bd to 0015443 Compare March 23, 2024 08:16

akinomyoga added 2 commits March 23, 2024 17:16

refactor(doctor): update func names and desc to match current impl

f3214a1

akinomyoga force-pushed the bash-backend branch from 0015443 to 22dca50 Compare March 23, 2024 08:16

ellie approved these changes Apr 2, 2024

View reviewed changes

ellie merged commit f881ce5 into atuinsh:main Apr 2, 2024
16 checks passed

akinomyoga deleted the bash-backend branch April 2, 2024 08:08

BrewTestBot mentioned this pull request Apr 15, 2024

atuin 18.2.0 Homebrew/homebrew-core#169062

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(doctor): detect preexec plugin using env ATUIN_PREEXEC_BACKEND #1856

fix(doctor): detect preexec plugin using env ATUIN_PREEXEC_BACKEND #1856

akinomyoga commented Mar 11, 2024 •

edited

akinomyoga commented Mar 11, 2024

akinomyoga commented Mar 11, 2024

ellie Mar 11, 2024

akinomyoga Mar 11, 2024

ellie Mar 11, 2024

ellie Mar 11, 2024

akinomyoga Mar 11, 2024

ellie Mar 11, 2024

ellie Mar 11, 2024

ellie commented Mar 11, 2024

akinomyoga commented Mar 11, 2024

ellie commented Mar 11, 2024 •

edited

ellie commented Mar 11, 2024

ellie commented Mar 11, 2024

akinomyoga commented Mar 11, 2024 •

edited

akinomyoga commented Mar 11, 2024

ellie commented Mar 12, 2024

ellie left a comment

akinomyoga commented Apr 2, 2024

akinomyoga commented Apr 2, 2024

fix(doctor): detect preexec plugin using env ATUIN_PREEXEC_BACKEND #1856

fix(doctor): detect preexec plugin using env ATUIN_PREEXEC_BACKEND #1856

Conversation

akinomyoga commented Mar 11, 2024 • edited

Discussion

Checks

akinomyoga commented Mar 11, 2024

akinomyoga commented Mar 11, 2024

Plugin Detection in ble.sh

ellie Mar 11, 2024

Choose a reason for hiding this comment

akinomyoga Mar 11, 2024

Choose a reason for hiding this comment

ellie Mar 11, 2024

Choose a reason for hiding this comment

ellie Mar 11, 2024

Choose a reason for hiding this comment

akinomyoga Mar 11, 2024

Choose a reason for hiding this comment

ellie Mar 11, 2024

Choose a reason for hiding this comment

ellie Mar 11, 2024

Choose a reason for hiding this comment

ellie commented Mar 11, 2024

akinomyoga commented Mar 11, 2024

ellie commented Mar 11, 2024 • edited

ellie commented Mar 11, 2024

ellie commented Mar 11, 2024

akinomyoga commented Mar 11, 2024 • edited

akinomyoga commented Mar 11, 2024

ellie commented Mar 12, 2024

ellie left a comment

Choose a reason for hiding this comment

akinomyoga commented Apr 2, 2024

akinomyoga commented Apr 2, 2024

akinomyoga commented Mar 11, 2024 •

edited

ellie commented Mar 11, 2024 •

edited

akinomyoga commented Mar 11, 2024 •

edited