rmyorston / busybox-w32

WIN32 native port of BusyBox.
https://frippery.org/busybox
Other
666 stars 124 forks source link

Issue with busybox ash inside bash #428

Open ale5000-git opened 1 week ago

ale5000-git commented 1 week ago

By running busybox ash inside bash (bash is from https://gitforwindows.org/) I get this: busybox-shell-inside-bash

Is there any way to fix it without manually altering PS1 or not?

From a different version of bash there isn't any error but it is still messed up: /[/e]0;/w/a/]/n/[/e[32m/]/u@/h /[/e[35m/]/[/e[0m/] /[/e[33m/]/w/[/e[0m/]/n/$

avih commented 1 week ago

No way to fix it other than setting a PS1 value which is suitable for ash.

In fact, it won't work in any other shell either, and sometimes not even with bash, and also on linux.

Because the string __git_ps1 comes from git-prompt which needs to be loaded (dot) into the shell, e.g. in ~/.bashrc or ~/.profile, so that this function becomes available and then it can be used in PS1.

Even more so, the better way to use the git-prompt thing in bash is to use PROMPT_COMMAND, which, in turn, sets PS1 every time a new prompt is printed, so that PS1 value is sometimes not even set by bash or the user, but rather by the git-prompt thing.

And that git-prompt doesn't support ash. It uses arrays and some other featured iirc which just don't work in ash.

So the only way to "fix" it to to setup a PS1 value of your own which ash supports.

avih commented 1 week ago

From a different version of bash there isn't any error but it is still messed up: /[/e]0;/w/a/]/n/[/e[32m/]/u@/h /[/e[35m/]/[/e[0m/] /[/e[33m/]/w/[/e[0m/]/n/$

This was fixed recently.

By default, ash incorrectly converted back slashes to forward slashes, but it was fixed about a month ago iirc. Update your busybox.exe .

avih commented 1 week ago

This was fixed recently.

Actually, I'm not sure.

There was an issue that when using sh -X (to keep backslashes at env vars), it also incorrectly converted forward slash to backslashes. This was fixed two months ago in bb128070e590234f8b63fb1d67f7621a1b4b3ff3 .

But this is a different issue. These are backslashes which should remain unmodified, but the default behavior is to convert them to slashes, so it's probably still an issue in master.

As a workaround, you can use a recent busybox.exe (month or two) and sh -X to preserve backslashes in env vars (including paths).

ale5000-git commented 1 week ago

I currently have the busybox of 2024-06-16. I will retry with the latest asap.

avih commented 1 week ago

I currently have the busybox of 2024-06-16. I will retry with the latest asap.

No need. it's recent enough.

Just use sh -X to preserve the backslashes in PS1, so this should fix the issue with incorrect forward slashes.

As for the __git_ps1 issue, there's no fix because it's incompatible with ash.

ale5000-git commented 1 week ago

I understand that the issue with __git_ps1 cannot by fixed by busybox but about the other issue can be automatically preserved in special variables like PS1 without -X?

The -X is definitely not something that is wanted unless there is a need to use on old version of Windows.

avih commented 1 week ago

can be automatically preserved in special variables like PS1 without -X?

Maybe @rmyorston wants to add it as another exception to the list (which currently includes at least COMSPEC).

The -X is definitely not something that is wanted

Speak for yourself.

For me, on win10, there's no reason for busybox to modify env vars values at all (changing the name of some vars to uppercase is OK). But it does modify variables by default, exactly like it did for you with PS1, which is rightly a bug.

So to workaround the bug which modifies env values by default, I use -X, and I encourage others to do that as well.

Just be careful that some paths include backslashes, so any script which deals with paths with NIH code should take that into account.

But if one uses the standard tools to handle paths (dirname, basename, etc), then these would handle backslashes withot any issues.

ale5000-git commented 1 week ago

Speak for yourself.

For me, on win10, there's no reason for busybox to modify env vars values at all (changing the name of some vars to uppercase is OK). But it does modify variables by default, exactly like it did for you with PS1, which is rightly a bug.

So to workaround the bug which modifies env values by default, I use -X, and I encourage others to do that as well.

It is obviously an user choice but I usually prefer to use slashes in pathes everywhere to be more coherent. They work almost perfectly everywhere in Windows (with just minor exceptions).

The only issues are like in these cases where backslashes aren't part of pathes.

ale5000-git commented 1 week ago

PS: An alternative way would be instead of a blacklist use a whitelist so that only variables that are know to contain pathes are changed.

avih commented 1 week ago

I usually prefer to use slashes in pathes everywhere to be more coherent.

-X only affects (prevents) converting env vars when the shell is entered. The relevant ones are typically the standard windows path vars ($PATH, $UERSPROFILE, $LOCALAPPDATA, etc).

But it doesn't prevent you from using forward shashes in everything you do in bb-w32, or adding dirs with forward shashes to $PATH, etc.

An alternative way would be instead of a blacklist use a whitelist so that only variables that are know to contain pathes are changed.

Out of curiosity, in the years you used bysybox-w32, did you ever have a use case which would break if those vars were not converted to forward slashes?

Because I use the bb-w32 shell quite a lot, and I don't think I've ever had a use case where I needed to use one of these windows path vars which would break if the paths contains backslashes...

And if you really have some vars which you want converted to forward shashes, then you can still use -X and then add something like this to your ~/.profile:

for name in PATH UERSPROFILE LOCALAPPDATA; do
    eval "v=\$$name"
    v=$(printf %s "$v" | sed 's/\\/\//g')
    eval "$name=\$v"
done

(and add as many variables names as you want)

ale5000-git commented 1 week ago

It isn't breaking, it is just annoying. I'm a bit perfectionist.

For example, without conversion, with this: export ANDROID_SDK_ROOT="${LOCALAPPDATA:?}/Android/Sdk" I end up with mixed slashes/backslashes that isn't nice.

I know I could also fix them with realpath but it is nice to have them correct as is.

eval is usually something that I avoid unless there isn't any other choice (potentially slow and risky). I also try to avoid sed when possible, regexp are slow and can be easily messed up (at least by me).

avih commented 1 week ago

It isn't breaking, it is just annoying. I'm a bit perfectionist.

For example, without conversion, with this: export ANDROID_SDK_ROOT="${LOCALAPPDATA:?}/Android/Sdk" I end up with mixed slashes/backslashes that isn't nice.

IMO, the fact that you're annoyed by something which is not broken and has zero impact on functionality, is not a good enough reason for bb-w32 to modify env vars by default.

IMO clearly the goal in converting to forward slashes was to avoid breaking some things, but I just can't think of anything which would break in such case under reasonably normal usage.

eval is usually something that I avoid unless there isn't any other choice (potentially slow and risky).

It's never slow. It's typically undistinguishable from non-eval code in terms of speed. Even if it was slow, doesn't matter when it only executes once you enter an interactive shell. This specific use case is also 100% safe, and 100% correct (up to $(...) stripping trailing newlines - which shouldn't exist anyway on path vars. it can be addressed trivially, but no need).

I also try to avoid sed when possible, regexp are slow and can be easily messed up (at least by me).

It's not the regexp which is slow, but rather the additional processes and command substitution, but again, it has no impact when it only executes once on shell init, and it would only add very few ms in practice.

There's not much to mess up here anyway. It's a trivial replacement, which you can also do with tr:

If you just refuse to use eval and sed, then don't:

PATH=$(printf %s "$PATH" | tr \\ /)
FOO=$(printf %s "$FOO" | tr \\ /)
...

The bottom line of all this is that I don't see a need to touch vars on shell init, and if you really want forward slashes only in specific vars, then currently the only option is to use -X and modify the vars you want as described above - which would fix your PS1 issue, and the issue with any other var which was incorrectly converted to forward slashes.

rmyorston commented 1 week ago

I've tweaked the shell's export built-in so unexporting a variable imported from the environment unsets its special flag and restores its original value from the environment. It can then be re-exported with its new value.

This allows PS1 in the example above to be reset with export -n PS1; export PS1.

unexport

There are new prereleases (PRE-5405 or above).

avih commented 1 week ago

This allows PS1 in the example above to be reset with export -n PS1; export PS1.

Hmm... export -n is not posix, and 100% undetectable as far as I can tell, because even export --help doesn't help.

100% undetectable applies to many bb shell features, like [[, because there's no manpage which documents the shell..., so I guess export -n being undetectable is not unique to export -n, but it doesn't make it better either...

How would one know that export -n is a thing?

But more on the topic of the discussion above, what is the default conversion to forward slashes supposed to help with?

As noted above, I haven't encountered an issue where anything would break due to keeping backslashes (and I do use -X, so I'd know if something got broken), and the only "issue" ale5000-git can report is that it's "annoying" that the result of e.g. FOO_DIR=$LOCALAPPDATA/foo ends up with mixed forward/back slashes - which is not an issue to any application.

rmyorston commented 6 days ago

Yup, export -n is a bashism which is also supported by upstream BusyBox. I suppose one would know about export -n by reading the bash documentation or the BusyBox source.

what is the default conversion to forward slashes supposed to help with?

As I said before:

It makes is easier to port Unix scripts and applications to Windows.

avih commented 6 days ago

It makes is easier to port Unix scripts and applications to Windows.

It can in some cases, and it's a personal preference, but for me the cons outeright the pros, because any shell code in busybox-w32 need to handle backslashes anyway, e.g. it it arrives unmodified via argv from cmd.exe or elsewhere, and *nix script also expect different list-separator (colon vs semicolon), so it's much better to be on the safe side and ensure that both backslashes and semicolon work in such scripts, and not try to use the code unmodified.

And obviously converting to forward shashes need to be curated, because some things like $COMSPEC must be excluded, as should $PS1, and other things we can't think of currently.

And trying to teach users to do export -n PS1 && export PS1 is quite meh, because I really don't think users would know where to look for this info, or what the hell is broken here at all.

Also, users on windows do expect paths to contain backslashes. it wouldn't surprise anyone IMO that $PATH has backslashes and semicolon separator - people expect that.

So overall I think for most people it would be better to not convert back to forward slashes, but indeed ultimately the decision about the default is yours.

ale5000-git commented 5 days ago

Hmm... export -n is not posix, and 100% undetectable as far as I can tell, because even export --help doesn't help.

100% undetectable applies to many bb shell features, like [[, because there's no manpage which documents the shell..., so I guess export -n being undetectable is not unique to export -n, but it doesn't make it better either...

How would one know that export -n is a thing?

But more on the topic of the discussion above, what is the default conversion to forward slashes supposed to help with?

As noted above, I haven't encountered an issue where anything would break due to keeping backslashes (and I do use -X, so I'd know if something got broken), and the only "issue" ale5000-git can report is that it's "annoying" that the result of e.g. FOO_DIR=$LOCALAPPDATA/foo ends up with mixed forward/back slashes - which is not an issue to any application.

I just express my opinion, obviously this thing is opinable but even though I don't always agree I respect the opinion of everyone and I really appreciate your contribution and the one of rmyorston.

ale5000-git commented 5 days ago

@rmyorston Thanks, your solution works fine but for this specific case isn't better an automated solution?

Note: PS0, PS1, PS2, PS4 are not executed (just displayed) so there isn't any advantage of replacing backslashes and it is also controproductive since they are supposed to expand Bash escape sequences.

The thing inside ` is executed but also with backslashes should work correctly.

PS3 does not expand Bash escape sequences, so it doesn't really matter.