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

Add build option to run commands via bash -c #154

Merged
merged 4 commits into from Aug 6, 2023
Merged

Add build option to run commands via bash -c #154

merged 4 commits into from Aug 6, 2023

Conversation

dimkr
Copy link
Contributor

@dimkr dimkr commented Aug 6, 2023

/bin/sh is bash in Puppy, but not in all distros. I want to switch to dash as /bin/sh in dpup, to avoid the overhead of bash.

However, some gtkdialog-based tools assume that the shell is bash (for example, they use export -f and call the exported functions via <action>) and things "just work" because system() runs sh -c.

This PR adds a built option, -Dbash=true, which makes gtkdialog run commands via bash -c instead of sh -c.

@dimkr dimkr requested a review from 01micko August 6, 2023 06:09
meson_options.txt Show resolved Hide resolved
meson_options.txt Show resolved Hide resolved
@dimkr dimkr merged commit f526a0f into master Aug 6, 2023
2 checks passed
@dimkr dimkr deleted the feature/bash branch August 6, 2023 10:42
@step-
Copy link
Collaborator

step- commented Aug 6, 2023

/bin/sh is bash in Puppy, but not in all distros. I want to switch to dash as /bin/sh in dpup, to avoid the overhead of bash.

However, some gtkdialog-based tools assume that the shell is bash (for example, they use export -f and call the exported functions via <action>) and things "just work" because system() runs sh -c.

This PR adds a built option, -Dbash=true, which makes gtkdialog run commands via bash -c instead of sh -c.

Kudos to @dimkr for addressing a seriously gray area of gtkdialog usage. This reminds me of gtkwialog by, I think, wiak. Gtkwialog didn't take off in Puppy, when it was presented, although I think it got added to DebianDogs, along with gtkdialog.

I'm not in favor of this approach. I think that if running via bash is a build-time option, most script authors won't be aware (unless the binary is named gtkdialog-bash or gtkdialog-dash but let's assume that the binary won't change name, it will be gtkdialog as it's always been). Then some venerable Puppy scripts might mysteriously break where /bin/sh -> dash, and fixing them in a dash environment may need some serious rethinking, because dash essentially "blacklists" some environment entries.

I think that the choice of which shell to use should be a run-time option on the command-line, not a build-time option. This forces new script authors to be aware of the limits imposed by any shell. If the choice of a shell becomes a command-line option, say --shell=, it should be made mandatory: no --shell= option specified, no run. Why? Because now, essentially, we have /bin/sh -> ??? (undetermined) for a script that gets used on non-Puppy systems. Therefore, to fix the gray area, an author must be explicit about the shell they're coding against.

If you prefer, --shell= could be replaced by or augmented with a new mandatory tag on line 1 of the script, like <shell>/bin/bash</shell>. (A more audacious proposal could be <interpreter>/bin/bsh</interpreter>, <interpreter>/usr/bin/awk</interpreter>, and so on with possibilities...)

In 2021 I compiled gtkdialog on Mint and wrote fitt, a not-yet-released bash script (#!/bin/bash) that should work on Fatdog64 812 (/bin/sh -> sh) and Mint 19.3 (/bin/sh -> dash). I bumped against dash's environment "blacklisting". I ended up changing my code as follows.

One - Right before running gtkdialog, the bash script does:

# Export functions via file for compatibility with Mint (19.3 dash 0.5.8-2.10)
# https://stackoverflow.com/questions/38079864
export BASH_ENV="$TMPD/func.sh"            # sourced by bash -c
declare -fx > "$BASH_ENV"

Two - In gtkdialog replace all:

<action>exported_bash_function arg1 arg2...</action>

With

<action>bash -c "exported_bash_function \"$@\"" arg0 arg1 arg2... </action>

In other words, the gtkdialog script is explicit about the underlying shell type, bash in this case. This solution needs a fork and two execs for each <action>. By changing gtkdialog to require declaring the intended shell upfront, with --shell=, <shell> or whatever, one exec could be spared.

@dimkr
Copy link
Contributor Author

dimkr commented Aug 6, 2023

I'm not in favor of this approach

Same here, but that's the only way I found to make existing gtkdialog-based stuff "just work" without changes and any "opt-in" flag in gtkdialog. The old stuff is full of bashisms, not just the export -f thing.

I'm open to other ideas, but prefer to avoid complex solutions.

(A bit of context: I have a small fork of woof-CE at https://github.com/vanilla-dpup/woof-CE/tree/vanilladpup-11.0.x, which removes all kinds of legacy stuff and builds a very Debian-y dpup. It adds some new "wizards" but they're yad based, and I want to keep some existing gtkdialog based tools until I have a replacement ready.)

@step-
Copy link
Collaborator

step- commented Aug 6, 2023

I'm not in favor of this approach

Same here, but that's the only way I found to make existing gtkdialog-based stuff "just work" without changes and any "opt-in" flag in gtkdialog.

This is where we think differently. You prefer no "opt-in" flag in gtkdialog. I prefer to explicitly opt-in by declaring the underlying shell. I know this will break all existing scripts, because currently they don't declare the shell they use. But fixing such breakage is very simple. Search-replace all lines that run gtkdialog and add --shell=/bin/bash. Done. No need to involve script authors or edit the scripts in other ways. This is something that a system integrator can do.

The old stuff is full of bashisms, not just the export -f thing.

I know, I agree.

I'm open to other ideas, but prefer to avoid complex solutions.

My idea is to force new authors to be explicit about the shell requirement, with --shell= or <shell>.

(A bit of context: I have a small fork of woof-CE at https://github.com/vanilla-dpup/woof-CE/tree/vanilladpup-11.0.x, which removes all kinds of legacy stuff and builds a very Debian-y dpup. It adds some new "wizards" but they're yad based, and I want to keep some existing gtkdialog based tools until I have a replacement ready.)

Your fork is transitioning away from gtkdialog, correct? Then I think it would be better to revert this PR in woof-ce and keep it only for your fork. You can change gtkdialog there. Rename the binary there to gtkdialog-dash, if you like, and replace all invocations of gtkdialog in the fork. I'm saying revert this PR in woof-ce because it doesn't solve the gray area, I think.

@dimkr
Copy link
Contributor Author

dimkr commented Aug 6, 2023

But fixing such breakage is very simple.

In woof-CE's code - yes, it's a simple find/replace operation. The problem is the big number of scripts in .pet packages - some are years old, still in active use, and I have no idea who has the credentials to upload new packages with the fix.

Your fork is transitioning away from gtkdialog, correct? Then I think it would be better to revert this PR in woof-ce and keep it only for your fork.

My plan is to enable this build flag only in my fork, because that's the only place where it's needed. Currently, my fork carries this PR as a patch but this patch might be useful for other "Puppy-adjucement" distros, without breaking anything for existing users.

(There's no "gray area" because every Puppy ever built has bash as sh and so many scripts expect this, so this PR won't break anything even if woof-CE starts building gtkdialog with this flag enabled.)

@step-
Copy link
Collaborator

step- commented Aug 6, 2023

Thank you for addressing all my points. I think, all goals and constraints considered, this PR a good thing.

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.

None yet

3 participants