-
Notifications
You must be signed in to change notification settings - Fork 346
fix: Respect shebangs in scripts #606
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
Conversation
…commands/ is also checked
This would conflict with #593. |
If 2 characters can be read from the file, a line must exist
The interpreter reads the first 2 characters of a file that's directly executed; if they're '#!', then the file is run through the command after those two characters. |
As far as I know, |
The changes that would conflict are from #247, I've just reintroduced them here because they're common sense changes, and the PR was closed due to inactivity. The conflicting changes would require one of us to paste about 3 lines in a different location. It's not a major conflict. |
Before this PR, commands are directly launched with |
Reading two characters, or one line, from each file, will not cause any impact on performance that would be anywhere near noticeable. |
Oh, I understand. That doesn't in any way "dismiss" shebangs defined in the scripts though or cause them to be ignored. |
no not really.. linutil is supposed to be posix complaint. |
arch-titus does completely rely on bashisms though this shouldnt matter since you should be executing it in a live iso |
scripts dont need be written in bash, a lot of the time you can completely write it in posix, not affecting any functionality at all as seen with the current scripts |
Linutil is a TUI, for now, wrapping a collection of scripts. It doesn't necessitate POSIX compliance. Several people may prefer writing scripts with Bash exclusive features, or providing a script that is directly launched through a different program entirely. Once again, this change doesn't mandate rewriting scripts in bash. All it does is make the executed command match expectations. If you write a bash shebang in your script, you expect bash to be launched. |
The only reason why this works is because Arch uses bash for its /bin/sh symlink. Say someone wanted to implement a similar script for Debian, or another distro, instead? If they relied on bash features, there would be issues caused through launching every script with sh. |
Never said it did mandate writing scripts in bash, and once again, linutil is supposed to be posix compliant.. As said by Chris many times. |
Shebangs are used to specify which command launches a script. Before this PR, linutil would launch every single script through |
and why would that matter? all scripts in linutil use the sh shebang, besides arch-titus which doesnt really matter anyways. |
and whys that? arch-titus runs perfectly, i have ran it many times while trying to fix the subvolume issue. your concerns about the shebangs are noted but at the moment it doesnt really matter as every script is being written with posix compliance and Chris does not want any bash related or python scripts inside of linutil |
ArchTitus is a major feature of the project, it's not insignificant. Once again, why should we exclude the capability to use another shell's features? We do not subscribe to the religion of POSIX. If people want to write POSIX compliant scripts, they should be able to. If people want to rewrite scripts to be POSIX compliant, they should be able to. But they should not be forced to. If someone wants to contribute a script that is written with bash features, they should absolutely be able to. |
and once again, Chris does not want bash scripts or python scripts inside of linutil, any script that is written with bashisms and submitted is denied, I never said arch-titus was insignificant i said the shebang problem does not matter as it should be ran in a live iso |
Chris never said he "didn't want bash related or python scripts inside of linutil", he said he's putting off the idea of different languages for now, and he already has a bash script within it. The only reason ArchTitus runs is because on Arch, /bin/sh is symlinked to Bash by default. If ArchTitus was made for another distro, or any other script with bash code was included in linutil, it would not work if we didn't respect the shebang. Once again. This isn't about making all scripts bash. This isn't about forcing you to do anything. You can write POSIX compliant scripts, someone else can write bash scripts if they prefer that way. This is about ensuring that all scripts are executed as the developer intended, with the shell the developer specified in their shebang. I don't see how this is controversial. |
Arch-titus installs ARCH it cannot be made for another distro... And you're contradicting yourself you literally just said Chris should allow people to submit bash scripts in your previous post. Also I can submit a PR to remove bashisms in arch-titus would that stop you from mentioning it every 5 seconds? |
ArchTitus is launched through linutil. That means, I'm aware ArchTitus is only for arch. But it's just an example. Not every script is written for Arch. You can rewrite whatever script you want to be POSIX compliant. This only makes it so you don't have to, in order to get the script into linutil. POSIX compliant scripts are preferred within the project, but they don't have to be the only scripts included within. And even if they are, this PR still has no disadvantages. It launches the script exactly as the developer intended, with |
I agree with your statements, never said this PR was bad, it solves a dire issue. I'm worried about you saying that we should allow bash scripts into Linutil, why is that? what happens to the people who don't use bash? |
and how would this be implemented? im sure that we would have to specifically specify that the scripts are indeed written with bashisms |
Do you mean people who don't know how to read or write bash specific code, or people who don't have bash installed? Again, this doesn't force you to write scripts in bash. This only offers other people, who like the bash features, or perhaps already have bash scripts ready to be submitted, the option to include the scripts in the project. If you want to rewrite those scripts to not include bashisms, and to use |
The shebang is used to determine which shell is launched. If a PR is submitted with a fully POSIX compliant script that has a bash shebang, we'd recommend they change it to |
again you're not getting my point, as i said if we include bash scripts in linutil how would the user know that its a bash script thats not posix compliant? this would have to be specified somewhere or be under a precondition |
In the case of bash, I think just about nobody will have a system without it installed, but regardless, I've pushed a commit to address your concern. |
*BSD, Android, Busybox, Alpine Linux, the list goes on. |
Co-authored-by: Adam Perkowski <adas1per@protonmail.com>
69c2d1f
to
e7bf1f6
Compare
Co-authored-by: Adam Perkowski <adas1per@protonmail.com>
Thanks for this @lj3954 I approve to the idea of respecting shebangs for scripts that do have bashisms like ArchTitus. Yes, we do try to make POSIX compliant but I can see specific use cases for bash. |
Type of Change
Description
I've brought back the changes from #247, since they heavily improve maintainability, especially of the core crate, which has major modifications in this PR. I've handled its merge conflicts.
The major change in this PR is that linutil core will now read the shebang from any script; the Command struct has been updated to include an executable and arguments in order to launch a script. In the case that a shebang isn't present, we'll return
sh -e
, which is the shebang currently used in most scripts (though, before this PR, never actually being used to execute scripts; sh was called without arguments).I've chosen to make the executable & arguments unique fields, as opposed to a single String, in order to simplify future additions, such as GUIs. Rust's
std::process::Command
, as well as most libraries which have a Command struct, take in a command & a vector of arguments (including the one we're using for the TUI; we're only passing in a formatted String instead because of the addition of multi selection).Testing
Scripts launch correctly, commands run match expectations.
Impact
Performance impact should be negligible, we're only reading the first 2 bytes of each script if no shebang is present, or until the first line break if it is. Scripts which require a certain shell or shell arguments will be executed as intended following this change.
Issues / other PRs related
Additional Information
Checklist