Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve misc implementation in Bash #430

Merged
merged 7 commits into from
Aug 12, 2024
Merged

Conversation

akinomyoga
Copy link
Contributor

This conflicts with #429, but after either one is merged, I'll rebase the other to resolve the conflicts.

@akinomyoga akinomyoga changed the title Improve misc in Bash Improve misc implementation in Bash Jul 17, 2024
@cantino
Copy link
Owner

cantino commented Jul 17, 2024

Are there any issues with bash 3 vs 4 vs 5 with these changes? I know we've had issues in the past on older systems (mostly Macs I think).

@akinomyoga
Copy link
Contributor Author

akinomyoga commented Jul 17, 2024

Thanks for your review. These should work in Bash 3.1+. To use the array PROMPT_COMMAND (which is only supported in Bash 5.1), this PR checks the Bash version on this line. The other features are all available in Bash 3/4/5.

However, Bash 3.0 would fail to parse PROMPT_COMMAND+=(...). If you would like to make it work also in Bash 3.0, I can adjust it. For reference, Bash bundled with macOS is Bash 3.2 and doesn't have this issue. The unmaintained MSYS1 under the MinGW project had shipped the old version of Bash 3.1 until recently, but I'm not aware of any systems that still ship Bash 3.0.

@akinomyoga
Copy link
Contributor Author

akinomyoga commented Jul 17, 2024

I'd suggest including a check for the Bash version (>= 3.1 or >= 3.0) at the beginning of mcfly.bash. What do you think? If you agree. I can add a commit in this PR.

@cantino
Copy link
Owner

cantino commented Jul 21, 2024

Yes, thank you. Maybe we just warn if it's older than 3.1? If that's on Mac and Linux by default than I don't think we need to support less than 3.1.

@akinomyoga
Copy link
Contributor Author

akinomyoga commented Jul 21, 2024

rebased 8ad6576..70efe35 on top of the latest master.

@akinomyoga
Copy link
Contributor Author

akinomyoga commented Jul 21, 2024

I added 9bf6891 and 03ef43f for the Bash version check. I finally decided to include the support for Bash 3.0. To perform the Bash version check properly, we anyway need to allow lower versions of Bash to parse the script. Then, Bash 3.0 actually works under such a modification.

Details--Because of the nature of the script that is supposed to be evaluated by eval "$(mcfly init bash)", we cannot perform the early return from the script based on the Bash version. So the entire script needs to be correctly parsed even when the script is evaluated in Bash lower than 3.1 and would be disabled by the Bash version check. Meanwhile, the only thing we need to modify for Bash 3.0 is to allow Bash 3.0 to parse the part only enabled in Bash 5.1, which is the same as the modification for the Bash version check.

mcfly.bash Show resolved Hide resolved
akinomyoga added a commit to akinomyoga/mcfly that referenced this pull request Jul 21, 2024
The command "which" is a non-POSIX external command, and technically,
its existence in the system is not ensured.  We here use Bash's
built-in command "type", which is the most reliable.  We prefix
"builtin" to "type" because some users overwrite "type" with a shell
function or an alias to mimic Windows' "type" command.

The use of the which command was first removed in Ref. [1].  However,
this was reverted after the issue in Ref. [2].  As far as a relative
path is not contained in PATH, the problem reported in Ref. [2] does
not seem to be reproducible in all the versions from Bash 3.0 to 5.2
and in the devel branch of Bash.  However, when the path "./bin" is
included in PATH, the "command -v" produces the relative path
"./bin/mcfly" as reported.  This seems to have been solved by using
the "which command in the reporter's environment, but the behavior of
the "which" command may depend on the implementation.  We should
explicitly resolve the relative path if the obtained MCFLY_PATH is
relative.

References:

[1] cantino#216
[2] cantino#292
[3] cantino#430 (comment)
@akinomyoga akinomyoga force-pushed the bash-refactor branch 2 times, most recently from 855c101 to 53f3314 Compare July 21, 2024 22:24
The command "which" is a non-POSIX external command, and technically,
its existence in the system is not ensured.  We here use Bash's
built-in command "type", which is the most reliable.  We prefix
"builtin" to "type" because some users overwrite "type" with a shell
function or an alias to mimic Windows' "type" command.

The use of the which command was first removed in Ref. [1].  However,
this was reverted after the issue in Ref. [2].  As far as a relative
path is not contained in PATH, the problem reported in Ref. [2] does
not seem to be reproducible in all the versions from Bash 3.0 to 5.2
and in the devel branch of Bash.  However, when the path "./bin" is
included in PATH, the "command -v" produces the relative path
"./bin/mcfly" as reported.  This seems to have been solved by using
the "which command in the reporter's environment, but the behavior of
the "which" command may depend on the implementation.  We should
explicitly resolve the relative path if the obtained MCFLY_PATH is
relative.

References:

[1] cantino#216
[2] cantino#292
[3] cantino#430 (comment)
This is to avoid later error messages in the case MCFLY_PATH contained
an invalid/outdated path before the initialization by mcfly.bash.
We perform this in a shell function to temporarily set IFS.
@cantino cantino merged commit cdbb5fe into cantino:master Aug 12, 2024
19 checks passed
cantino pushed a commit that referenced this pull request Aug 12, 2024
The command "which" is a non-POSIX external command, and technically,
its existence in the system is not ensured.  We here use Bash's
built-in command "type", which is the most reliable.  We prefix
"builtin" to "type" because some users overwrite "type" with a shell
function or an alias to mimic Windows' "type" command.

The use of the which command was first removed in Ref. [1].  However,
this was reverted after the issue in Ref. [2].  As far as a relative
path is not contained in PATH, the problem reported in Ref. [2] does
not seem to be reproducible in all the versions from Bash 3.0 to 5.2
and in the devel branch of Bash.  However, when the path "./bin" is
included in PATH, the "command -v" produces the relative path
"./bin/mcfly" as reported.  This seems to have been solved by using
the "which command in the reporter's environment, but the behavior of
the "which" command may depend on the implementation.  We should
explicitly resolve the relative path if the obtained MCFLY_PATH is
relative.

References:

[1] #216
[2] #292
[3] #430 (comment)
@cantino
Copy link
Owner

cantino commented Aug 12, 2024

Thanks @akinomyoga!

@akinomyoga akinomyoga deleted the bash-refactor branch August 12, 2024 01:44
@akinomyoga
Copy link
Contributor Author

Thanks!

@aminya
Copy link

aminya commented Aug 12, 2024

This has introduced a bug. See #435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants