[NixOS stdenv] Sourcing nixpkgs stdenv breaks blesh trap handling

SuperSandro2000 commented 2 years ago

ble version: 0.4.0-devel3+8dbefe0 Bash version: 5.1.8(1)-release (x86_64-pc-linux-gnu)

bash inside of nixpkgs is: 5.1.12(1)-release (x86_64-pc-linux-gnu)

I am a heavy user of nix, nixpkgs and nix-shell. nix-shell is a convenient program which adds programs temporarily to your bash session. It also sources the nixpkgs stdenv which is used to build packages. Due to reasons which I couldn't understand ble.sh' trap handler breaks when the file is sources. You can find an online version of the file here https://github.com/NixOS/nixpkgs/blob/master/pkgs/stdenv/generic/setup.sh which already triggers the bug. I have commit rights in nixpkgs so if the file is doing something really wonky I can fix it upstream but it will take a while until it bubbles to a release.

(set -x; source pkgs/stdenv/generic/setup.sh)
++++(:3): set -x
++++(:3): source pkgs/stdenv/generic/setup.sh
+++++(pkgs/stdenv/generic/setup.sh:3): set -eu
+++++(pkgs/stdenv/generic/setup.sh:3): set -o pipefail
+++++(pkgs/stdenv/generic/setup.sh:3): shopt -s inherit_errexit
+++++(pkgs/stdenv/generic/setup.sh:3): [[ -n 5 ]]
+++++(pkgs/stdenv/generic/setup.sh:3): [[ 5 -lt 4 ]]
+++++(pkgs/stdenv/generic/setup.sh:3): ((  0 >= 6  ))
+++++(pkgs/stdenv/generic/setup.sh:3): : out
+++++(pkgs/stdenv/generic/setup.sh:3): trap exitHandler EXIT
+++++(~/.local/share/blesh/ble.sh:3): trap(): ble/builtin/trap exitHandler EXIT
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): local flags command sigspecs
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): ble/builtin/trap/.read-arguments exitHandler EXIT
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): flags=
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): command=
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): sigspecs=()
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): (( 2 ))
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): local arg=exitHandler
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): shift
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): [[ exitHandler == -?* ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): [[ '' != *[pc]* ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): command=exitHandler
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): flags=c
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): (( 1 ))
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): local arg=EXIT
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): shift
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): [[ EXIT == -?* ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): [[ c != *[pc]* ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): ble/array#push sigspecs EXIT
+++++(~/.local/share/blesh/ble.sh:3): ble/array#push(): builtin eval 'sigspecs+=("${@:2}")'
++++++(~/.local/share/blesh/ble.sh:3): ble/array#push(): sigspecs+=("${@:2}")
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): (( 0 ))
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): [[ c != *[hlpE]* ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): [[ c != *c* ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.read-arguments(): (( 1==0 ))
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): [[ c == *h* ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): [[ c == *E* ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): [[ c == *l* ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): [[ ! -n 1 ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): [[ -n 1 ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): ble-edit/exec/save-BASH_REMATCH
+++++(~/.local/share/blesh/ble.sh:3): ble-edit/exec/save-BASH_REMATCH(): _ble_edit_exec_BASH_REMATCH=("${BASH_REMATCH[@]}")
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): [[ c == *p* ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): local _ble_builtin_trap_inside=1
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): local spec ret
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): for spec in "${sigspecs[@]}"
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): ble/builtin/trap/.get-sig-index EXIT
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.get-sig-index(): [[ -n EXIT ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.get-sig-index(): [[ ! -n EXIT ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.get-sig-index(): ble/gdict#get _ble_builtin_trap_n2i EXIT
+++++(~/.local/share/blesh/ble.sh:3): ble/gdict#get(): ble/dict#get _ble_builtin_trap_n2i EXIT
+++++(~/.local/share/blesh/ble.sh:3): ble/dict#get(): builtin eval -- 'ret=${_ble_builtin_trap_n2i[x$2]-}; [[ ${_ble_builtin_trap_n2i[x$2]+set} ]]'
++++++(~/.local/share/blesh/ble.sh:3): ble/dict#get(): ret=0
++++++(~/.local/share/blesh/ble.sh:3): ble/dict#get(): [[ -n set ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.get-sig-index(): [[ -n 0 ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap/.get-sig-index(): return 0
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): [[ exitHandler == - ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): _ble_builtin_trap_handlers[ret]=exitHandler
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): [[ -n 1 ]]
+++++(~/.local/share/blesh/ble.sh:3): ble/builtin/trap(): ble/function#try ble/builtin/trap:EXIT exitHandler EXIT
+++++(~/.local/share/blesh/ble.sh:3): ble/function#try(): local lastexit=0
+++++(~/.local/share/blesh/ble.sh:3): ble/function#try(): ble/is-function ble/builtin/trap:EXIT
+++++(~/.local/share/blesh/ble.sh:3): ble/is-function(): declare -F ble/builtin/trap:EXIT
+++++(~/.local/share/blesh/ble.sh:3): ble/function#try(): return 127

akinomyoga commented 2 years ago

Thank you for the report. This is caused by set -e, which ble.sh doesn't assume. Maybe I can add a workaround for this, but I don't have an idea in what situation set -e can be turned on in an interactive session. I'd like to confirm what this script does before I make changes.

For example, if we simply turn on set -e in an interactive session, when a user makes even a single mistake in the session, the Bash session will unexpectedly be terminated. Even a correct command, such as grep regex file which returns the matched state via its exit status, causes the termination of the session. So, I assume there is no sane usage of set -e in interactive sessions.
If this is not an interactive session, then the question is why ble.sh is loaded into a non-interactive session. But I'm not sure if that really happens because ble.sh checks if it is an interactive Bash at the beginning of the script in loading time.
Maybe this is a script that is supposed to be loaded in a subshell of an interactive session?

SuperSandro2000 commented 2 years ago

Correct. The shell would exit if any command even a grep fails which is also the case in the nix-shell startup phase. A bit later a set +e is run so that in the interactive part the shell stays open. I am not fully sure what happens in detail but that is the closest I got to yesterday. The relevant C++ code which starts the shell can be found here https://github.com/NixOS/nix/blob/master/src/nix-build/nix-build.cc#L539. A few lines above that is the code that generates the rc file which also sources the stdenv setup file. If you have any other questions about nix in general or how the pieces belong together I can answer them but some deep details on how the stdenv works are also a bit complicated for me.

The session is interactive and loaded in a subshell. I already had problems with it in the past when none local variables leaked into it. I am sure I could also patch around it but I would like to avoid that due to the complexity of it.

akinomyoga commented 2 years ago

Thank you for the explanation. So, the user's bashrc is loaded here and then the aforementioned setup.sh is loaded here?

Anyway, I think I now understand the situation. Then I think it is sufficient just to patch trap and other builtins that ble.sh overrides. I have adjusted the codes to save/restore the shell options 6a946f0 and then added workarounds for the reported problem in commit 001c595.

The session is interactive and loaded in a subshell.

What do you mean by the subshell? The file specified to --rcfile is executed in the main shell but not in subshells like () or $().

[ Edit: In the previous my reply, I considered the case that ble.sh is loaded in the main shell and setup.sh is loaded in a subshell like xxx=$(source setup.sh; do_something) and intentionally keeps set -e for the later execution in the subshell. ]

Maybe this is unrelated to the present problem, but I have some suggestions about the initialization code of nixpkgs and NixOS you have provided.

Shouldn't `set +e` be executed at the end of `setup.sh`?

set +e appears to be executed after two or three assignments (here). %3%..%5% will be replaced by a safe command and quoted strings, so I don't think they can cause errors (in a sane setup)^{[Note 1]}. I believe the code sets set -e should be responsible for restoring the original setting, i.e., set +e should be handled here.

Note 1: Actually, we can cause errors by setting declare -n p='_dummy[$(false)]' or declare -n PATH='_dummy[$(false)]' or readonly p, but I don't think nixpkgs wants to exit the shell with these kinds of settings.

I think the initialization code shouldn't use `p` which may be used by users.

If a user sets up the variable p in .bashrc, its value will be lost by the initialization code. Maybe this is executed in a subshell so that the value p might not be used later, but considering that we source the user's bashrc, I guess we cannot completely exclude the possibility of the variable p being used later.

`set -u` is unset unconditionally. Is this intentional?

I haven't actually tried nixpkgs or NixOS, but set -u appears to be unconditionally cleared at the end of setup.sh. This means that even if the user turned on set -u in .bashrc, that will be cleared after loading setup.sh. Is this something intentional by nixpkgs?

In this sense, actually I believe set -e state is also recovered instead of unconditoinally clearing. Something like:

+__nixpkgs_setup_set_original=$-
 set -eu

[...snip...]

-# Disable nounset for nix-shell.
-set +u
+# Restore the original options
+[[ $__nixpkgs_setup_set_original == *e* ]] || set +e
+[[ $__nixpkgs_setup_set_original == *u* ]] || set +u
+unset -v __nixpkgs_setup_set_original

SuperSandro2000 commented 2 years ago

Thank you for the explanation. So, the user's bashrc is loaded here and then the aforementioned setup.sh is loaded here?

Yes but the bashrc is only loaded when the shell is not pure. That would mean blesh is not loaded, too so I don't think we need to worry about that.

What do you mean by the subshell?

I meant nested shell, not subshell.

Shouldn't set +e be executed at the end of setup.sh?

Maybe, probably. I am not sure.

I think the initialization code shouldn't use p which may be used by users.

That is a common problem with nix-shell. There are other variables like out which I set already in the past and aborted the shell early. I don't think the general problem can be solved that easy. I am not sure if the newer versions of nix-shell nix shell still source the stdenv. So far I mostly avoided this by using local in every function possible and unsetting temporary variables.

set -u is unset unconditionally. Is this intentional?

Let me try to upstream this. I don't think I can remove the set +x in nix-shell right now because that would break things on older versions. https://github.com/NixOS/nixpkgs/pull/156252

Edit:

sadly nix-shell does still not function like it should.

$ nix-shell -p hello
user@host:~/src/nixpkgs$ removed '/tmp/nix-shell-366865-0/rc'
removed '/tmp/nix-shell-366865-0/.attr-0'
removed directory '/tmp/nix-shell-366865-0'

Let me try to figure out what else goes wrong.

akinomyoga commented 2 years ago

$ nix-shell -p hello
user@host:~/src/nixpkgs$ removed '/tmp/nix-shell-366865-0/rc'
removed '/tmp/nix-shell-366865-0/.attr-0'
removed directory '/tmp/nix-shell-366865-0'

This looks like error messages produced by rm called in _nix_shell_clean_tmpdir. This kind of problems are caused when there is an alias alias rm='rm -v' or a function rm() { do_something; command rm -v "$@"; }. I'm not sure if this is the case for the present problem, but if it is, we may prefix command to the commands that are used non-interactively in order to avoid such a problem.

-                R"(_nix_shell_clean_tmpdir() { rm -rf %1%; }; )"s +
+                R"(_nix_shell_clean_tmpdir() { command rm -rf %1%; }; )"s +

SuperSandro2000 commented 2 years ago

Yeah, thats also something I should fix but I wanted to imply that the nix-shell is immediately closed without any input.

$ nix-shell -p hello
 * Safely within bottle.
user@host:~$ [ble: EOF]
$

akinomyoga commented 2 years ago

Unwanted outputs `prompt[ble: EOF]`

$ nix-shell -p hello
 * Safely within bottle.
user@host:~$ [ble: EOF]
$

Hmm, I see.

What the problem is:

Actually user@host:~$ is output when ble-attach is called from .bashrc. I guess you are now using the following setup.

source /path/to/ble.sh --attach=none

[...]

[[ ${BLE_VERSION-} ]] && ble-attach

This is the minimal reproducer of the problem:

$ cat bashrc.test
PS1=prompt_string
source /path/to/ble.sh --attach=none
ble-attach
exec true
$ bash --rcfile bashrc.test
prompt_string[ble: EOF]
$

Solution 1 (at bashrc side)

As a simple fix, you can modify your .bashrc as

-source /path/to/ble.sh --attach=none
+source /path/to/ble.sh

[...]

-[[ ${BLE_VERSION-} ]] && ble-attach

ble.sh provides two different attaching strategies. One is the manual attach, i.e., ble-attach from .bashrc. The other is the prompt attach through PROMPT_COMMAND. The latter doesn't work when the user just overwrites the existing value of PROMPT_COMMAND, so I have been considering that the former is more reliable. But this is the first case that I encountered where the latter works better.

Solution 2 (at nixpkgs side)

Hmm, I haven't thought there is a case that one starts Bash as an interactive shell (without commands) but performs exec before it actually shows the prompt and accepts user inputs.

Another solution at the nixpkgs side is to let ble.sh know that the interactive Bash is intended for an execution of a command by passing a dummy command string via -c exit.

$ bash --rcfile bashrc.test -c exit
$

I mean we change line 526 as

-          ? Strings{"bash", "--rcfile", rcfile}
+          ? Strings{"bash", "--rcfile", rcfile, "-c", "exit"}

SuperSandro2000 commented 2 years ago

Hmm, I see. Actually user@host:~$ is output when ble-attach is called from .bashrc. I think you are now using the following setup.

It is not only unwanted output but the nix-shell is closed immediately which renders it useless.

Solution 1 (at bashrc side)

I have chosen this solution. Since I am only appending PROMPT_COMMAND I think this works the best for me.

Solution 2 (at nixpkgs side)

That would only work when you use blesh and nix-shell. If you wouldn't use ble.sh the nix-shell would exit and execute that command. I could patch that locally but rebuilding nix forever is not that nice.

akinomyoga commented 2 years ago

If you wouldn't use ble.sh the nix-shell would exit and execute that command.

As I have called it a dummy command, it will not be executed because the shell process is replaced with another command by exec at the last line of the generated rcfile.

Ah, sorry, there was a mistake. More correctly, it needs to be

-          ? Strings{"bash", "--rcfile", rcfile}
+          ? Strings{"bash", "--rcfile", rcfile, "-i", "-c", "exit"}

in order to execute both rcfile and exit where the shell will be replaced by exec before the latter is actually executed.

Even if we do not load ble.sh, I feel it is better to include -c exit in the command line in the case that the exec on the last line fails. Or do we expect we fall into a new interactive Bash session when the exec fails? Ah, I now see shopt -s execfail is specified in the second last line, which implies that somehow it is expected that the shell falls into a new interactive session on the exec failure.

akinomyoga commented 2 years ago

If you can define some special shell variable, that tells the shell is running for the nix-shell command execution, at the beginning of the generated rcfile, I think I can add a workaround to ble.sh. I think this is also useful to disable in .bashrc other interactive bash settings that are unwanted for the nix-shell command execution.

Hmm, if the function name _nix_shell_clean_tmpdir is stable, I think I can use declare -f _nix_shell_clean_tmpdir &>/dev/null to check if it is in the nix-shell command execution. I think a dedicated shell variable or something is better, but if this function and its name is considered stable, I can check the function in ble.sh and cancel the loading of ble.sh in the case that the function exists. What do you think?

SuperSandro2000 commented 2 years ago

If you can define some special shell variable, that tells the shell is running for the nix-shell command execution, at the beginning of the generated rcfile, I think I can add a workaround to ble.sh. I think this is also useful to disable in .bashrc other interactive bash settings that are unwanted for the nix-shell command execution.

That is unlikely to be merged because nix-shell only support bash. For zsh you need something like https://github.com/chisui/zsh-nix-shell. Also nix-shell is deprecated and slowly replaced by nix shell and nix develop which work slightly different.

Hmm, if the function name _nix_shell_clean_tmpdir is stable,

I don't think that will change anytime soon.

I think a dedicated shell variable or something is better

I think we can use IN_NIX_SHELL https://github.com/NixOS/nix/blob/master/src/nix-build/nix-build.cc#L283 which can either pure or impure.

akinomyoga commented 2 years ago

Thank you! I haven't noticed IN_NIX_SHELL. I have updated the initial check of ble.sh in commit b4bd955.

SuperSandro2000 commented 2 years ago

Thank you! I haven't noticed IN_NIX_SHELL. I have updated the initial check of ble.sh in commit b4bd955.

Nice, that also fixed a bug I found earlier today that commands like nix-shell -p yarn --command "yarn global upgrade" did not work.

akinomyoga commented 2 years ago

Does everything work fine now for the reported problem of this issue and related stuff? If you are still checking the behavior, that's fine.

SuperSandro2000 commented 2 years ago

I am not sure what changed since I tested this but nix-shell -p yarn --command "yarn global upgrade" works now like a charm.

akinomyoga commented 2 years ago

Hmm, yeah I actually don't know the detailed reason why it has failed with ble.sh, but ble.sh is not designed for non-interactive uses after all. If the problem happens only for non-interactive uses, I feel it's just fine that we stop loading ble.sh by IN_NIX_SHELL. Anyway, thank you for opening the issue and your prompt and patient responses!

SuperSandro2000 commented 2 years ago

I feel it's just fine that we stop loading ble.sh by IN_NIX_SHELL.

Well, that also means that ble.sh is disabled in nix-shells. Also not the perfect solution.

akinomyoga commented 2 years ago

Oh, so nix-shells are not always non-interactive?

SuperSandro2000 commented 2 years ago

Oh, so nix-shells are not always non-interactive?

They can be both. If you specify a command it it will be closed after the command is done. You can also supply shell code to execute and add a return at the end to drop into an interactive when everything is run.

interactive: nix-shell -p hello command nix-shell --command "export HISTCONTROL=$HISTCONTROL HISTFILESIZE=$HISTFILESIZE HISTSIZE=$HISTSIZE; return"

not interactive: nix-shell -p hello --command "hello"

akinomyoga commented 2 years ago

Thank you for the information! Then, we actually shouldn't just stop loading ble.sh with IN_NIX_SHELL. Later I need to check what nix-shell actually does.

Edit 2022-02-02: I guess I need to install nix-shell and check the behavior in my environment.

akinomyoga commented 2 years ago

I've set nix up and checked the behavior. I've locally fixed ble.sh to accept loading ble.sh with IN_NIX_SHELL and added a workaround for the initialization in nix-shell but found another issue.

In nix-shell environment, the shell variable BASH is somehow rewritten to the path of a different binary as the current process (specifically, some dumb Bash binary without readline support), which causes problems. In my case, the actual binary is /nix/store/xz3kkiscxh01yazxa1dk08q047mfl1fh-bash-interactive-5.1-p12/bin/bash but the value of BASH is /nix/store/2kh3c4v2vf6d6xg6c9n8zvfpwf3zzwca-bash-5.1-p12/bin/bash.

To begin with, the shell variable BASH shouldn't be changed to something different from the current Bash binary. BASH is explained in Bash Reference Manual 5.2 as "The full pathname used to execute the current instance of Bash.". The nix-shell environment breaks this assumption. This is not a problem specific to ble.sh. For example, when a user wants to start a child shell by running

$ "$BASH"

it enters a dumb shell without a line editor at all.

I have tracked down the issue.

First of all, on the shell startup in nix-shell, Bash seems to fail to identify its real path and sets /bin/bash to BASH as a fallback.
This seems to be adjusted by $stdenv/setup. I have the following lines in $stdenv/setup:
```
/nix/store/xcy53qbjynin70g46w1nx011gr7jw8l1-stdenv-linux/setup:1: export SHELL=/nix/store/2kh3c4v2vf6d6xg6c9n8zvfpwf3zzwca-bash-5.1-p12/bin/bash
/nix/store/xcy53qbjynin70g46w1nx011gr7jw8l1-stdenv-linux/setup:304: BASH="$SHELL"
```
This sets SHELL and BASH to the non-interactive version of Bash. I guess $stdenv/setup is actually designed for some background processing of nixpkgs and not designed for the interactive uses.
Then in the generated rcfile for nix-shell, SHELL is again overwritten to the interactive version.
```
/tmp/nix-shell-49112-0/rc:3: SHELL='/nix/store/xz3kkiscxh01yazxa1dk08q047mfl1fh-bash-interactive-5.1-p12/bin/bash'
```
I believe nix-shell should also update the value of BASH here. @SuperSandro2000 Can you add a line BASH=$SHELL here?

(But maybe we should actually solve the problem that Bash fails to find its real path as this is the root cause in step 1.)

SuperSandro2000 commented 2 years ago

In nix-shell environment, the shell variable BASH is somehow rewritten to the path of a different binary as the current process (specifically, some dumb Bash binary without readline support), which causes problems. In my case, the actual binary is /nix/store/xz3kkiscxh01yazxa1dk08q047mfl1fh-bash-interactive-5.1-p12/bin/bash but the value of BASH is /nix/store/2kh3c4v2vf6d6xg6c9n8zvfpwf3zzwca-bash-5.1-p12/bin/bash.

That sounds to me like a bug no one before encountered which should be fixed.

This sets SHELL and BASH to the non-interactive version of Bash. I guess $stdenv/setup is actually designed for some background processing of nixpkgs and not designed for the interactive uses.

Yes :) thats why the newer equivalents of the command to that part different but they also break other parts and are not fully ready yet at least for most of my usecases.

(But maybe we should actually solve the problem that Bash fails to find its real path as this is the root cause in step 1.)

I think the shell where the code is run initially is not interactive but later nix-shell starts an interactive one and there something might be not quite right.

BASH gets overwritten here since commit. So I think we cannot just drop that.

I believe nix-shell should also update the value of BASH here. @SuperSandro2000 Can you add a line BASH=$SHELL here?

Done in https://github.com/NixOS/nix/pull/6047

akinomyoga commented 2 years ago

Thank you! It seems to be already merged, but could you tell when it will be applied to the downstream? Would it be in the next release 2.6.1 or 2.7.0? Or, is already available on the user side after updating?

SuperSandro2000 commented 2 years ago

Would it be in the next release 2.6.1 or 2.7.0

I am not sure. I am not that much involved into the release process of nix but I would guess that 2.6.1 would only contain important bug fixes, so 2.7.0 should definitely have it.

Or, is already available on the user side after updating?

Not in the default config. Maybe when they are using nix-unstable in a few days or weeks but 2.7.0 should also be released in about 4 weeks or so.

akinomyoga commented 2 years ago

I see. Thank you for the explanation! Hmm, maybe I need to wait for 2.7.0 to enable ble.sh in nix-shell, or maybe I should add some workarounds for $BASH in nix-shell and soon enable ble.sh. I will later reconsider it.

SuperSandro2000 commented 2 years ago

You can get it by using an overlay now already:

$ cat ~/.config/nixpkgs/overlays/nix.nix
final: prev: {
  nixVersions = prev.nixVersions // {
    unstable = prev.nixVersions.unstable.overrideAttrs (oldAttrs: with final;rec {
      patches = oldAttrs.patches ++ [
        (fetchpatch {
          url = "https://github.com/NixOS/nix/pull/6047.patch";
          sha256 = "sha256-tu2V8hNeuq2nrwb7Tz0OaNRBtgkCbs7Fr05T6Xjw3YQ=";
        })
      ];
    });
  };
}

If you are still on nix stable or an older nixUnstable version you would need to use

$ cat ~/.config/nixpkgs/overlays/nix.nix
final: prev: {
  nixUnstable = prev.nixUnstable.overrideAttrs (oldAttrs: with final;rec {
    patches = oldAttrs.patches ++ [
      (fetchpatch {
        url = "https://github.com/NixOS/nix/pull/6047.patch";
        sha256 = "sha256-tu2V8hNeuq2nrwb7Tz0OaNRBtgkCbs7Fr05T6Xjw3YQ=";
      })
    ];
  });
}

Then when you do nix-shell -p nixVersions.unstable or nix-shell -p nixUnstable it will build the new version and open a shell with it.

akinomyoga commented 2 years ago

Yeah, thank you for the information! I'm fine with it, but the problem is the users. Users do not necessarily use the unstable version of nix, and I also don't want to require users to use the unstable versions just for ble.sh. If I just release ble.sh for the unstable versions of nix, there will be troubles in the nix stable users. While, I don't want to introduce in ble.sh unnecessary workarounds that will soon be expired. But maybe I finally need to add the workaround.

SuperSandro2000 commented 2 years ago

Maybe display a warning when the nix version is to old? We could get that with nix --version which is really fast and we could only execute when entering a nix-shell?

akinomyoga commented 2 years ago

Yeah, that is one option. Anyway, we can't say the current release version 2.6.0 is old in any sense, so I think I wouldn't show that warning soon.

Hmm, OK. I guess there will be a non-negligible number of users that do not shift to the latest version of nix soon. Also, I guess we need to test it anyway, so I'd apply the workaround and enable ble.sh in nix-shell later. Then, after that, I'd ask you to test it.

akinomyoga commented 2 years ago

@SuperSandro2000 Finally, I have added a workaround for nix-shell and re-enabled loading ble.sh in nix-shell (commit ceb2e7c). It seems working in my environment but could you test whether it works without problem in interactive sessions of nix-shell? Thank you!

SuperSandro2000 commented 2 years ago

I gave it a quick try and didn't find any obvious bugs immediately. I think this can be closed.

akinomyoga commented 2 years ago

OK. I close it. If you find another problem, you can reopen this or create a new issue. Thank you again!

akinomyoga / ble.sh