Skip to content

Conversation

lj3954
Copy link
Contributor

@lj3954 lj3954 commented Sep 21, 2024

Type of Change

  • New feature
  • Bug fix

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

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no errors/warnings/merge conflicts.

@adamperkowski
Copy link
Collaborator

This would conflict with #593.

If 2 characters can be read from the file, a line must exist
@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

do you know how shebangs work in the first place?

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.

@adamperkowski
Copy link
Collaborator

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).

As far as I know, sh -e is used in every script expect for d2r-loot-filters.sh, which is yet to be refactored.
And I'm not sure I understand what do you mean by "shebangs ignored".

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

This would conflict with #593.

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.

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

the shebang update looks redundant and could worsen performance, not sure why you think the shebang is getting ignored when its not?

Before this PR, commands are directly launched with sh. It's not sh -e, although that would be an easy fix. However, some scripts may be more convenient to implement with bash instead, or with some other shell offering more features. Script developers should have that option available to them. ArchTitus does have a bash shebang currently. I'm not completely sure if it relies on any bash features, though.

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

the shebang update looks redundant and could worsen performance, not sure why you think the shebang is getting ignored when its not?

Reading two characters, or one line, from each file, will not cause any impact on performance that would be anywhere near noticeable.

@adamperkowski
Copy link
Collaborator

adamperkowski commented Sep 22, 2024

Before this PR, commands are directly launched with sh. It's not sh -e, although that would be an easy fix.

Oh, I understand. That doesn't in any way "dismiss" shebangs defined in the scripts though or cause them to be ignored.

@ghost
Copy link

ghost commented Sep 22, 2024

the shebang update looks redundant and could worsen performance, not sure why you think the shebang is getting ignored when its not?

Before this PR, commands are directly launched with sh. It's not sh -e, although that would be an easy fix. However, some scripts may be more convenient to implement with bash instead, or with some other shell offering more features. Script developers should have that option available to them. ArchTitus does have a bash shebang currently. I'm not completely sure if it relies on any bash features, though.

no not really.. linutil is supposed to be posix complaint.

@ghost
Copy link

ghost commented Sep 22, 2024

arch-titus does completely rely on bashisms though this shouldnt matter since you should be executing it in a live iso

@ghost
Copy link

ghost commented Sep 22, 2024

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

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

no not really.. linutil is supposed to be posix complaint.

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.

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

arch-titus does completely rely on bashisms though this shouldnt matter since you should be executing it in a live iso

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.

@ghost
Copy link

ghost commented Sep 22, 2024

no not really.. linutil is supposed to be posix complaint.

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.

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.

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

Before this PR, commands are directly launched with sh. It's not sh -e, although that would be an easy fix.

Oh, I understand. That doesn't in any way "dismiss" shebangs defined in the scripts though or cause them to be ignored.

Shebangs are used to specify which command launches a script. Before this PR, linutil would launch every single script through sh, regardless of what the shebang specified. That is indeed ignoring the behaviour put forth by the shebang.

@ghost
Copy link

ghost commented Sep 22, 2024

Before this PR, commands are directly launched with sh. It's not sh -e, although that would be an easy fix.

Oh, I understand. That doesn't in any way "dismiss" shebangs defined in the scripts though or cause them to be ignored.

Shebangs are used to specify which command launches a script. Before this PR, linutil would launch every single script through sh, regardless of what the shebang specified. That is indeed ignoring the behaviour put forth by the shebang.

and why would that matter? all scripts in linutil use the sh shebang, besides arch-titus which doesnt really matter anyways.

@ghost
Copy link

ghost commented Sep 22, 2024

no not really.. linutil is supposed to be posix complaint.

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.

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.

He indeed prefers fully POSIX compliant scripts, I'm aware, but not every script will be. ArchTitus is a prime example. For the scripts which do use Bash features, we want to ensure that linutil handles them correctly.

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

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

Before this PR, commands are directly launched with sh. It's not sh -e, although that would be an easy fix.

Oh, I understand. That doesn't in any way "dismiss" shebangs defined in the scripts though or cause them to be ignored.

Shebangs are used to specify which command launches a script. Before this PR, linutil would launch every single script through sh, regardless of what the shebang specified. That is indeed ignoring the behaviour put forth by the shebang.

and why would that matter? all scripts in linutil use the sh shebang, besides arch-titus which doesnt really matter anyways.

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.

@ghost
Copy link

ghost commented Sep 22, 2024

Before this PR, commands are directly launched with sh. It's not sh -e, although that would be an easy fix.

Oh, I understand. That doesn't in any way "dismiss" shebangs defined in the scripts though or cause them to be ignored.

Shebangs are used to specify which command launches a script. Before this PR, linutil would launch every single script through sh, regardless of what the shebang specified. That is indeed ignoring the behaviour put forth by the shebang.

and why would that matter? all scripts in linutil use the sh shebang, besides arch-titus which doesnt really matter anyways.

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

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

no not really.. linutil is supposed to be posix complaint.

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.

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.

He indeed prefers fully POSIX compliant scripts, I'm aware, but not every script will be. ArchTitus is a prime example. For the scripts which do use Bash features, we want to ensure that linutil handles them correctly.

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

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.

@ghost
Copy link

ghost commented Sep 22, 2024

no not really.. linutil is supposed to be posix complaint.

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.

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.

He indeed prefers fully POSIX compliant scripts, I'm aware, but not every script will be. ArchTitus is a prime example. For the scripts which do use Bash features, we want to ensure that linutil handles them correctly.

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

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?

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

ArchTitus is launched through linutil. That means, sh is used to execute it. The only reason it currently works is because on Arch in specific, /bin/sh is symlinked to bash. This isn't guaranteed behaviour. Debian links it to dash, for example. If we use the shebang to launch the shell best suited to the script, we can ensure that there will never be errors caused due to the wrong shell being launched. If someone submits a non-POSIX compliant bash script, it won't be completely nonfunctional until a rewrite, if one is desired.

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 /bin/sh -e if that's what they chose in the shebang.

@lj3954 lj3954 changed the title Respect shebang fix: Respect shebangs in scripts Sep 22, 2024
@ghost
Copy link

ghost commented Sep 22, 2024

ArchTitus is launched through linutil. That means, sh is used to execute it. The only reason it currently works is because on Arch in specific, /bin/sh is symlinked to bash. This isn't guaranteed behaviour. Debian links it to dash, for example. If we use the shebang to launch the shell best suited to the script, we can ensure that there will never be errors caused due to the wrong shell being launched. If someone submits a non-POSIX compliant bash script, it won't be completely nonfunctional until a rewrite, if one is desired.

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 /bin/sh -e if that's what they chose in the shebang.

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?

@ghost
Copy link

ghost commented Sep 22, 2024

and how would this be implemented? im sure that we would have to specifically specify that the scripts are indeed written with bashisms

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

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?

Do you mean people who don't know how to read or write bash specific code, or people who don't have bash installed?
For the former, I think the amount of people who can read all POSIX code but can't read code including bash features, which is quite similar, is very small. Most people who understand one can understand the other, and the differences are typically very minor. The latter group is essentially nonexistent; I would be extremely surprised if there was even a distro which didn't install bash by default.

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 /bin/sh -e instead, that option is still fully available to you, this PR doesn't force you to make any script non POSIX compliant.

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

and how would this be implemented? im sure that we would have to specifically specify that the scripts are indeed written with bashisms

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 /bin/sh -e. That's what has already been suggested on many PRs matching that case. If they do have bashisms, they can have a bash shebang. And if you or someone else wants to refactor them to not use those bash features, that's still possible. This just makes it so scripts that rely on bash aren't completely impossible to add without said refactor happening first.

@ghost
Copy link

ghost commented Sep 22, 2024

and how would this be implemented? im sure that we would have to specifically specify that the scripts are indeed written with bashisms

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 /bin/sh -e. That's what has already been suggested on many PRs matching that case. If they do have bashisms, they can have a bash shebang. And if you or someone else wants to refactor them to not use those bash features, that's still possible. This just makes it so scripts that rely on bash aren't completely impossible to add without said refactor happening first.

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

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 22, 2024

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.

@adamperkowski
Copy link
Collaborator

adamperkowski commented Sep 22, 2024

In the case of bash, I think just about nobody will have a system without it installed

*BSD, Android, Busybox, Alpine Linux, the list goes on.

Co-authored-by: Adam Perkowski <adas1per@protonmail.com>
lj3954 and others added 2 commits September 21, 2024 18:01
Co-authored-by: Adam Perkowski <adas1per@protonmail.com>
@ChrisTitusTech
Copy link
Owner

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.

@ChrisTitusTech ChrisTitusTech merged commit aca42f2 into ChrisTitusTech:main Sep 22, 2024
1 check passed
@lj3954 lj3954 deleted the respect_shebang branch September 22, 2024 16:51
@jeevithakannan2 jeevithakannan2 mentioned this pull request Sep 22, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Shebangs are completely ignored

4 participants