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

dietpi-unsupported_terminal.sh | Support for non-bash terminal sessions #2347

Closed
Elijahg opened this issue Dec 12, 2018 · 9 comments
Closed
Labels
Milestone

Comments

@Elijahg
Copy link

Elijahg commented Dec 12, 2018

Creating a bug report/issue:

Updating from v6.17.12 to v6.19.7 fails. There seems to be a problem with the /etc/profile.d/unsupported_terminal.sh script, I've had a peek in it but it seems fine so not really sure what's going on!

Required Information:

  • DietPi version |
    G_DIETPI_VERSION_CORE=6
    G_DIETPI_VERSION_SUB=17
    G_DIETPI_VERSION_RC=12
  • Distro version | stretch
  • Kernel version | Linux jaffa 4.14.71-v7+ #1145 SMP Fri Sep 21 15:38:35 BST 2018 armv7l GNU/Linux
  • SBC device | RPi 3 Model B (armv7l)
  • Power supply used | 3a IKEA PSU
  • SDcard used | SanDisk ultra

Additional Information (if applicable):

  • Software title | possibly dietpi-update, though the error is when apt-get runs a script during the install of spamassassin
  • Was the software title installed freshly or updated/migrated? updated
  • Can this issue be replicated on a fresh installation of DietPi? unknown
  • dietpi-bugreport ID | 04ebad78-518e-403e-8c90-4cbeecb52127

Steps to reproduce:

Run dietpi-update on a 6.17.12 install. The installation errors with -su: 19: /etc/profile.d/dietpi-unsupported_terminal.sh: Syntax error: "(" unexpected (expecting "fi")

Subsequent runs of dietpi-update and apt-get result in the same error.

Expected behaviour:

Installation should complete.

Actual behaviour:

Installation fails

Extra details:

echo $TERM returns xterm-256color

Attempt to apt-get upgrade:
eli@jaffa:~ $ sudo apt-get upgrade
Reading package lists... Done
Building dependency tree
Reading state information... Done
Calculating upgrade... Done
The following packages have been kept back:
docker-ce mariadb-client-10.1 mariadb-server mariadb-server-10.1 mariadb-server-core-10.1
0 upgraded, 0 newly installed, 0 to remove and 5 not upgraded.
1 not fully installed or removed.
After this operation, 0 B of additional disk space will be used.
Do you want to continue? [Y/n] y
Setting up spamassassin (3.4.2-1~deb9u1) ...
-su: 19: /etc/profile.d/dietpi-unsupported_terminal.sh: Syntax error: "(" unexpected (expecting "fi")
dpkg: error processing package spamassassin (--configure):
subprocess installed post-installation script returned error exit status 2
Errors were encountered while processing:
spamassassin
E: Sub-process /usr/bin/dpkg returned an error code (1)

@MichaIng
Copy link
Owner

@Elijahg
Thanks for your report.

Very strange, just tested the current script and it works as expected here, no syntax errors, whiptail menu pops up, if $TERM variable is unsupported, also https://www.shellcheck.net/ does not show any real syntax error.

Strange that spamassassin calls these scripts with su, obviously as login-shell. Question is with which user 🤔.

First check, if your script is as it should be:
cat /etc/profile.d/dietpi-unsupported_terminal.sh
Should match: https://raw.githubusercontent.com/Fourdee/DietPi/master/rootfs/etc/profile.d/dietpi-unsupported_terminal.sh

If it is due to somehow with bad user/permissions/incomplete environment, workaround is to temporary rename the script to run the APT update:
mv /etc/profile.d/dietpi-unsupported_terminal.sh /etc/profile.d/dietpi-unsupported_terminal.sh.bak
and move it back afterwards.

@Elijahg
Copy link
Author

Elijahg commented Dec 12, 2018

Thanks for the quick reply @MichaIng!

My version doesn't match the one in v6.19.7, but is identical to the 6.17.12. Perhaps the updater script should have replaced this file by the time it had tried to install spamassasin?

If there's any further investigation you'd like me to do I'll hold off updating for a bit! Thanks again!

@MichaIng
Copy link
Owner

@Elijahg
Ah jep APT upgrade is done before downloading new DietPi code, which is also reasonable.
But still, the code actually is fine. Needs investigation in which cases, e.g. sudo/su with different user, it might fail. Can't find a reason currently, since full bash environment is (or should be) loaded on every login-shell 🤔. Will try to replicate with spamassassin as well.

Actually, since it appears during update, easiest is to simply remove the script. The updater will anyway put the new one in place. It is a non-essential script that just works around an issue, if your SSH client ships a terminal variable $TERM that is not supported by the system. In this cases the SSH terminal usually still opens, but several binaries will throw error messages (e.g. estimating terminal size, capabilities, tput and such) and/or fail. But we just had a single case where this was the case 😉.

@Elijahg
Copy link
Author

Elijahg commented Dec 12, 2018

Something actually that I forgot to mention @MichaIng is I use zsh instead of Bash as my shell. That shouldn't in theory make any difference though since the scripts run under Bash.

I've removed it and updated successfully :) Many thanks for your help!

@MichaIng
Copy link
Owner

@Elijahg
Ahh interesting. Jep now it makes sense:

  • /etc/bash.bashrc => /etc/bashrc.d/*.sh is only loaded on interactive bash sessions and also called from /etc/profile only if in bash session. So in your case dietpi-globals are not loaded and no DietPi login banner is shown on login, right?
  • Only dietpi-unsupported_terminal.sh is placed in /etc/profile.d/, since it is required just once on login. But there are now two issues with non-bash login session:
  1. The script is not zsh compatible obviously, not even dash compatible:
2018-12-12 23:33:49 root@micha:/tmp# dash /etc/profile.d/dietpi-unsupported_terminal.sh
/etc/profile.d/dietpi-unsupported_terminal.sh: 19: /etc/profile.d/dietpi-unsupported_terminal.sh: Syntax error: "(" unexpected (expecting "fi")

I think the arrays are the issue here. And the shebang #!/bin/bash seems to be ignored when scripts are sourced (instead of executed).
2. Even if it would be compatible, the G_WHIP_MENU global function is not loaded to show the whiptail menu.

So there are two solutions:

  1. Make the script bourne shell compatible: Single braces only, check arrays and use bare whiptail menu, since dietpi-globals are not and will most properly never be compatible to non-bash.
  2. Move script to /etc/bashrc.d, where it is only called from bash sessions, but this means it is called on every interactive subshell again.

Sadly an additional [ "$BASH" ] check does not work, at least in case of dash. The whole script seams to be already parsed.

Perhaps there is another way to completely skip parsing when not sourcing with bash.

@MichaIng MichaIng changed the title Bug in script dietpi-unsupported_terminal.sh: Syntax error: "(" unexpected (expecting "fi") dietpi-unsupported_terminal.sh | Support for non-bash terminal sessions Dec 19, 2018
@MichaIng
Copy link
Owner

MichaIng commented Dec 19, 2018

@Fourdee
What do you think?

On the one hand, keeping everything bash-only code is the consistent way here. So all terminal session related scripts/globals require bash and we don't need to take care different compatibilities/syntax. No big deal to move the above script to bashrc.d. It is only loaded on interactive shells and theoretically not only the SSH client but as well some broken/wrong terminal emulator could ship a wrong $TERM variable, that could be included into the check (leave out SSH session check, similar to LANG=POSIX check we do).

On the other hand, I like to allow users some flexibility. So where applicable and not too much effort, we can make these login scripts bourne shell compatible, which is intended for scripts in profile.d/.


Okay had another though about it and simply moved the script to /etc/bashrc.d:

@MichaIng MichaIng added this to the v6.20 milestone Dec 27, 2018
@MichaIng
Copy link
Owner

Close, since this has been resolved with above PRs.

@Elijahg
Copy link
Author

Elijahg commented Dec 31, 2018

@MichaIng, apologies for not replying before - I for some reason only got an email notification when you closed the issue 🤔 I realise it's no longer a problem but for completeness here's that I would have said!

  • /etc/bash.bashrc => /etc/bashrc.d/*.sh is only loaded on interactive bash sessions and also called from /etc/profile only if in bash session. So in your case dietpi-globals are not loaded and no DietPi login banner is shown on login, right?

I was actually sourcing /etc/bashrc.d/dietpi-login.sh when logging in - it's in my .zshrc.

  • Only dietpi-unsupported_terminal.sh is placed in /etc/profile.d/, since it is required just once on login. But there are now two issues with non-bash login session:

I think the arrays are the issue here. And the shebang #!/bin/bash seems to be ignored when scripts are sourced (instead of executed).

That would explain why when sourcing it via my .zshrc it was being executed with zsh, not bash.

  1. Even if it would be compatible, the G_WHIP_MENU global function is not loaded to show the whiptail menu.

I've never had a problem with the whiptail menus, I presumably everything was running purely under bash once zsh had actually launched any of the Dietpi scripts.

Close, since this has been resolved with above PRs.

I'll look forward to giving it a go, if you'd like me to test anything please let me know :) Thanks again for the awesome distro and for your help!

@MichaIng
Copy link
Owner

@Elijahg

I was actually sourcing /etc/bashrc.d/dietpi-login.sh when logging in - it's in my .zshrc.

Interesting, dietpi-login.sh sources /DietPi/dietpi/dietpi-globals and these should throw the same Syntax error: "(" unexpected as dietpi-unsupported_terminal.sh does. It as well defines arrays that way: https://github.com/Fourdee/DietPi/blob/master/dietpi/func/dietpi-globals#L591

This made me reading a bid about zsh. Actually the syntax error does not come from zsh, but indeed from sh (dash). zsh supports this way of array assigning and the very most bash syntax.
There are some minor incompatibilities, but nothing you will face with dietpi-globals, AFAIK.

So it seems that spamassassin indeed calls /etc/profile with sh/dash or initiates an interactive sh/dash shell during setup.

However issue solved by moving the script to /etc/bashrc.d. It is anyway no good style to place non purge bourne shell scripts into /etc/profile.d. Perhaps we should rename ours to .bash ending to make things even clearer. .sh actually has a wrong implication.
And jep, since we use #!/bin/bash shebang, it is only a matter, if scripts are sourced, not if they are executed.

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

No branches or pull requests

2 participants