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

Fix install.sh output #842

Merged
merged 1 commit into from
Aug 25, 2023
Merged

Fix install.sh output #842

merged 1 commit into from
Aug 25, 2023

Conversation

xanderificnl
Copy link
Contributor

The behavior of the echo command and its handling of escape sequences, such as \n, can exhibit variability across different implementations of shells and utilities. This variability was evident during my experience while executing the install.sh script on a newly installed Fedora system:

[xander@fedora ~]$ curl https://rtx.pub/install.sh | sh
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  4302  100  4302    0     0  78350      0 --:--:-- --:--:-- --:--:-- 79666
rtx: installing rtx...
rtx: installed successfully to /home/xander/.local/share/rtx/bin/rtx
rtx: run the following to activate rtx in your shell:
echo "eval \"\$(/home/xander/.local/share/rtx/bin/rtx activate bash)\"" >> ~/.bashrc\n # <--newline
rtx: this must be run in order to use rtx in the terminal
rtx: run `rtx doctor` to verify this is setup correctly

Furthermore, a direct copy-paste of the command could potentially lead to unintended consequences. For instance, when executed in a bash shell, the command may inadvertently remove the trailing backslash:

[xander@fedora ~]$ echo "eval \"\$(/home/xander/.local/share/rtx/bin/rtx activate bash)\"" >> ~/.bashrc\n
[xander@fedora ~]$ cat .bashrcn # note the `n` at the end
eval "$(/home/xander/.local/share/rtx/bin/rtx activate bash)"
[xander@fedora ~]

Podman testing showed consistent echo behavior with bash across distributions, except for Debian and Ubuntu (using dash) which interpret escape sequences by default. Thus, replacing the newlines with empty info calls appears to be the most portable solution.

The behavior of the `echo` command and its handling of escape sequences, such as `\n`, can exhibit variability across different implementations of shells and utilities. This variability was evident during my experience while executing the `install.sh` script on a newly installed Fedora system:

```bash
[xander@fedora ~]$ curl https://rtx.pub/install.sh | sh
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  4302  100  4302    0     0  78350      0 --:--:-- --:--:-- --:--:-- 79666
rtx: installing rtx...
rtx: installed successfully to /home/xander/.local/share/rtx/bin/rtx
rtx: run the following to activate rtx in your shell:
echo "eval \"\$(/home/xander/.local/share/rtx/bin/rtx activate bash)\"" >> ~/.bashrc\n # <--newline
rtx: this must be run in order to use rtx in the terminal
rtx: run `rtx doctor` to verify this is setup correctly
```

Furthermore, a direct copy-paste of the command could potentially lead to unintended consequences. For instance, when executed in a bash shell, the command may inadvertently remove the trailing backslash:

```bash
[xander@fedora ~]$ echo "eval \"\$(/home/xander/.local/share/rtx/bin/rtx activate bash)\"" >> ~/.bashrc\n
[xander@fedora ~]$ cat .bashrcn # note the `n` at the end
eval "$(/home/xander/.local/share/rtx/bin/rtx activate bash)"
[xander@fedora ~]
```

Podman testing showed consistent `echo` behavior with bash across distributions, except for Debian and Ubuntu (using dash) which interpret escape sequences by default. Thus, replacing the newlines with empty `info` calls appears to be the most portable solution.
@jdx jdx enabled auto-merge (squash) August 25, 2023 00:07
@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Patch coverage has no change and project coverage change: +6.33% 🎉

Comparison is base (892c752) 81.27% compared to head (e450166) 87.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #842      +/-   ##
==========================================
+ Coverage   81.27%   87.60%   +6.33%     
==========================================
  Files         132      132              
  Lines       11533    11533              
==========================================
+ Hits         9373    10104     +731     
+ Misses       2160     1429     -731     

see 29 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jdx jdx merged commit e2baa73 into jdx:main Aug 25, 2023
15 checks passed
jdx pushed a commit that referenced this pull request Apr 9, 2024
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.

2 participants