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

WIP: Shellcheck script files in initrd #862

Draft
wants to merge 43 commits into
base: master
Choose a base branch
from

Conversation

Thrilleratplay
Copy link
Contributor

@Thrilleratplay Thrilleratplay commented Oct 20, 2020

  • Shell scripts without a file extension in /initrd renamed with .sh for consistency and simplifying shellcheck glob search.
  • Any #!/bin/ash shebangs are changed to #!/bin/sh as shellcheck does not have ash support.
  • Rewrite commands to be generic POSIX shell If possible, add shelllcheck exception where it is not.
  • Use relative source paths to validate imported variables and functions.
  • Manually test changes

Resolve shellcheck errors:

To investigate:

  • /initrd/sbin/config-dhcp.sh - variables not defined, does not seem to be functional.

@tlaurion
Copy link
Collaborator

@Thrilleratplay Waoooow!!!! Awesome initiative!!!!

@tlaurion
Copy link
Collaborator

* `/initrd/sbin/config-dhcp.sh` - variables not defined, does not seem to be functional.

Is called from https://github.com/osresearch/heads/blob/624faa1a9d9c7794927757ff49fbb567d6d031fb/config/busybox.config#L954 while I implemented DHCP configuration here.

@Thrilleratplay
Copy link
Contributor Author

@tlaurion Thank you for pointing out where it is referenced. I am going to make changes to /sbin/config-dhcp.sh separately and should read up on how busybox interacts with DHCP scripts. Likely for now, I will just make an exception for the undeclared variables that are used in the script.

@Thrilleratplay
Copy link
Contributor Author

Closing this in favor of #872

@tlaurion tlaurion reopened this Jul 29, 2022
@tlaurion tlaurion marked this pull request as draft July 29, 2022 17:53
@tlaurion
Copy link
Collaborator

tlaurion commented Jul 29, 2022

This is preferred approach over #872 (now closed).

The logic behind this is that bash is not fully supported under busybox, even as of today, where having scripts declaring bash interpreter vs ash will be misleading.

What changed (I think) since thn is that ASH is supported from shellcheck. This is actually DASH compliant, and we could get around shellcheck error SC2187 by doing either:

#!/bin/ash
# shellcheck shell=dash

As reported there, having shellcheck check for dash vs ash will report errors saying that echo -e is not supported under dash (which shellcheck supports). We will have to mute those errors as well, since we use echo -e under Heads.

So the workaround would be something in the lines of (until either busybox is fully supporting bash or shellcheck is fully supporting ash):

#!/bin/ash
# shellcheck shell=ash #we foce support for ash even if shellcheck complains it only supports dash
# shellcheck disable=SC3036 #echo -e is supported under ash but not dash

As you see, nothing perfect again.

The desired path is actually to have posix compliant scripts (where shellcheck currently complains for coding standards, but the scripts are compliant as of now).

Discussion happening over #885. Cross-linking from there to here.

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