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

Rewrite into more POSIX-compliant, portable, robust, composable and readable shell scripts #1908

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

Conversation

9ao9ai9ar
Copy link

@9ao9ai9ar 9ao9ai9ar commented Oct 17, 2024

Closes #1855 and fixes #1810.

The rewrite is mostly complete, but there are still some parts left to fix, mainly in prefsCleaner.sh. Although currently I have marked the pull request as draft, I consider it structurally stable enough for review. Once I have merged the changes in 9ao9ai9ar:posix-sh-bugfix, I will mark this pull request as ready for review, or ready to be merged. Unfortunately, I can not tell when that will be, as I:

  1. no longer have as much free time as before;
  2. need time to study Firefox's source code and documentation and do some experimentation on my own to ensure the critical parts of the new code are correct and optimal;
  3. need time to test and tick off the items in the test matrix below, which I came up with based on the supported build hosts and targets of Firefox;
  4. need time to think about the different approaches to update and prune the user.js and prefs.js files, respectively, e.g. Linux auto updater (update when Firefox starts) #1696.

This is not the first POSIX solution to have ever been requested or presented, though certainly the most comprehensive one yet, so I hope this time the pull request really gets merged back to upstream and not closed off due to circumstances.

Target Platforms for Modern Desktop Versions of Firefox

Tier-1

  • Windows
  • macOS
  • Fedora
  • Debian
  • Ubuntu

Tier-2

  • openSUSE
  • Arch Linux
  • Alpine Linux
  • NixOS

Tier-3

  • FreeBSD
  • OpenBSD
  • NetBSD
  • Solaris (11+)

Non-Tiered

  • OpenIndiana
  • Haiku (Iceweasel)

POSIX-Compatible Shells

  • bash 3 (macOS)
  • bash
  • osh
  • pdksh
  • ksh93u+(m)
  • mksh
  • ash
  • dash
  • posh
  • hush
  • zsh (via emulate sh)
  • yash
  • mrsh
  • gash
  • pbosh

@MagicalDrizzle
Copy link
Contributor

prefCleaner works fine for me, however updater doesn't seem to be able to append overrides.

@MagicalDrizzle
Copy link
Contributor

Seems to be just a broken check? This fixes it and updater seems to work fine now.
if ! [ "$SKIPOVERRIDE" ]; > if [ "$SKIPOVERRIDE" = false ];

@MagicalDrizzle
Copy link
Contributor

MagicalDrizzle commented Oct 24, 2024

Error in both scripts - you were checking for the literal file named SCRIPT_FILE:

// from
SCRIPT_FILE=$(preadlink "$0") && [ -f SCRIPT_FILE ] || exit 1
// to
SCRIPT_FILE=$(preadlink "$0") && [ -f $SCRIPT_FILE ] || exit 1

and for your missing /bin/sh question - no it doesn't seem to work.

@9ao9ai9ar 9ao9ai9ar changed the title Make shell scripts more portable and POSIX-compliant Rewrite into more POSIX-compliant, portable, robust, composable and readable shell scripts Oct 24, 2024
@jamesericdavidson
Copy link

@9ao9ai9ar Thoughts on using #!/usr/bin/env? sh could be busybox, the gimped version of Bash (which is not POSIX-compliant), etc.

updater.sh Show resolved Hide resolved
updater.sh Outdated Show resolved Hide resolved
@9ao9ai9ar
Copy link
Author

9ao9ai9ar commented Oct 29, 2024

@9ao9ai9ar Thoughts on using #!/usr/bin/env? sh could be busybox, the gimped version of Bash (which is not POSIX-compliant), etc.

The POSIX way to do it is to write a program to overwrite the shebang with the path of sh as determined by getconf PATH at install-time (see the relevant section in the POSIX document), but I think that is overkill and I would just ask the users to change it themselves if needed. My personal opinion is /bin/sh is better or at least no worse than /usr/bin/env sh because /usr might not be mounted, and the former allows specifying options to sh as an added bonus. I might reconsider the choice if /usr/bin/env sh somehow turns out to work better for the subset of operating systems the scripts are intended to support, but that doesn't seem likely now that I've verified modern Firefox is not available on Solaris 10.

@aartoni
Copy link

aartoni commented Nov 16, 2024

Hey @9ao9ai9ar @MagicalDrizzle I've noticed that progress on this PR is slowing down, so I'm willing to create a checklist of things to ensure that the script is working properly, and then make a GitHub workflow to test the script on different platforms.

Here are some things to be checked:

  • normal execution returns EX_OK
  • check every non-zero EX_*
  • user.js gets updated
  • user-overrides.js is respected

This will tick the GitHub available runners off the list of things that testers have to check manually. Manual tester can then copy the workflow to test locally.

What do you think? An alternative would be splitting this PR into smaller PRs.

9ao9ai9ar and others added 2 commits December 1, 2024 05:38
The rewrite focuses on the following five areas of interest:
1. Portability. The scripts should be useable across a number of Unix
   operating systems and shells; goes hand in hand with POSIX-
   compliance.
2. Robustness. Fail early; borrow from more battle-tested open source
   code; pass all valid ShellCheck checks.
3. Composability. Put everything inside functions; make the scripts
   dot source friendly.
4. Consistency. Abstract away terminal color codes with tput;
   uniform diagnostic messages and standardized use of status codes
   and redirections.
5. Readability. Extensive comments and descriptive names; use here-
   docs to ease writing multiline messages.

Known behavioral changes:
1. Changed the way the options are parsed and acted on.
   For example, when both the -p and -l options of updater.sh are
   specified, -l will be ignored. The old behavior would depend on the
   order of the options passed, where the last one wins, and the
   profile path passed as the argument to -p couldn't be named 'list'
   or it would be treated as if the option -l was specified.
2. All temporary files are created using mktemp, so users won't find
   them in the working directory anymore should an error occur and
   they were not removed as a result of that.
@9ao9ai9ar 9ao9ai9ar requested a review from sertonix November 30, 2024 21:46
@9ao9ai9ar 9ao9ai9ar marked this pull request as ready for review November 30, 2024 21:46
@MagicalDrizzle
Copy link
Contributor

@9ao9ai9ar prefsCleaner works, updater only partly - works only if there's already an user.js file. It won't download a new user.js if doesn't exist, then will try to backup the nonexistent user.js file which fails.

+ userjs_backup=/home/demo/Downloads/profile/userjs_backups/user.js.backup.2024-12-01_0511
+ mkdir -p /home/demo/Downloads/profile/userjs_backups
+ cp /home/demo/Downloads/profile/user.js /home/demo/Downloads/profile/userjs_backups/user.js.backup.2024-12-01_0511
cp: cannot stat '/home/demo/Downloads/profile/user.js': No such file or directory
+ userjs_backup=

@9ao9ai9ar 9ao9ai9ar marked this pull request as draft December 7, 2024 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Make Updater.sh shell agnostic updater.sh returns with exit code 0 when failing in update_userjs
5 participants