feat(assure_command_shell): assure proper installation of a shell #2368

FGrose · 2023-05-28T20:10:10Z

Introduce a new meta module, 99~sh, that will check that a command shell has been specified after module checks have been performed and before executable dependencies are checked. This avoids implicit installations of a command shell that skip the dracut module checks that the shell might need.

This PR now includes 5 commits to implement the feature:

feat(module-postprocess): provide a postprocessing loop for modules
feat(99~sh): assure explicit installation of a command shell
feat(99~no~sh): allow a build without any command shell
fix(00bash): fail if Bash below version 4.4 is linked to /bin/sh
refactor(99squash): gather code into the squash module

2 (indirectly) and 3, 4, & 5 above use the postprocessing feature directly to achieve the intention.

Completes fix to #1530 augmenting #1534.

An alternative command shell, for example, zsh can be installed by providing an appropriate module-setup.sh at ../modules.d/00zsh/ and running dracut with ln -sf zsh /bin/sh.

Checklist

[✔] I have tested it locally
[✔] I have reviewed and updated documentation in dracut.modules.7.asc & HACKING.md

LaszloGombos · 2023-05-29T03:40:00Z

busybox is also supported as a shell

FGrose · 2023-05-29T15:30:15Z

@LaszloGombos > busybox is also supported as a shell
Updated to include busybox in supported list.

LaszloGombos · 2023-05-29T17:39:49Z

Updated to include busybox in supported list.

Thanks.

1./ In my mental model, dracut.sh is kind of a package/dependency manager for dracut modules.
When we add/remove new dracut modules we should try to avoid keep changing the "package manager" itself.

In order to do this, you can try to introduce a new dracut "meta package" module for shell selection. Perhaps you can call it "sh" module. I hope you can model it after the dbus or network meta module. Please take a close look at #2181 for a good way to do this.

Ideally this would result in moving the assure_command_shell logic to the sh meta package. The base dracut module should be made dependent of the new "sh" meta dracut package.

2./ The current PR seems to be breaking test (and likely introducing regression). Please check test next time you upload the new revision of the PR.

Thank you so much for taking this on.

LaszloGombos

Overall direction is great, but needs some cleanup

LaszloGombos · 2023-06-01T23:50:56Z

modules.d/00_sh/module-setup.sh

+    # Assure a command shell.
+    [[ " $add_dracutmodules $force_add_dracutmodules " == @(*\ dash\ *|*\ bash\ *|*\ mksh\ *|*\ busybox\ *) ]] || {
+        shells='dash bash mksh busybox'
+        _shell=$(realpath -e /bin/sh)


dracut -m "bash" --> should install bash into initramfs regardless if what /bin/sh points to on the host
dracut -m "sh" --> should pick the dracut module based on what /bin/sh points to on the host
dracut -m "sh bash" --> should result in same initramfs as dracut -m "bash" --> should install bash into initramfs regardless if what /bin/sh points to on the host

dracut -m "sh dash bash" --> should install both dash and bash into initramfs regardless if what /bin/sh points to on the host (it is not clear to me what would be the deterministic rule for "${initdir}/bin/sh" to point to in this scenario, but this is an existing issue and not in scope for this PR". It might be that in this scenario the order of dracut modules on the command line is significant.)

The refactored PR meets the above intentions without requiring the user to request the meta module. It operates automatically.

LaszloGombos · 2023-06-01T23:51:57Z

modules.d/00_sh/module-setup.sh

+
+# Prerequisite check(s) for module.
+check() {
+


Existing modules might be already forcing particular shells, so you should be checking for that.

This can be done with the following pattern:

for module in $shells; do if dracut_module_included "$module"; then _shell="$module" break fi done

The refactored PR respects the shell requests of other modules.

LaszloGombos · 2023-06-01T23:54:02Z

modules.d/00_sh/module-setup.sh

+}
+
+# Module dependency requirements.
+depends() {


depends() function should return the shell selected. To keep it simple, maybe move all of the code from check() to depends(). check() could always just return 255 unconditionally (as long as base module marks it as a dependency).

'depends() function should return the shell selected.' This is achieved if needed.
The 99base module only expects that some shell be installed and linked to /bin/sh. This is achieved.

LaszloGombos · 2023-06-01T23:54:56Z

modules.d/00bash/module-setup.sh

    inst /bin/bash

+    # local - (available since bash-4.4 2016-09-15) automates the


You should remove all of these changes from the bash module from this PR, I think they will be confusing the reviewers. Please make a separate independent PR for the bash module change.

00bash and 99squash module changes are included in separate commits.
They are needed to accomplish the intention of installing the command shell only after all installation conditions have been met.

LaszloGombos · 2023-06-01T23:55:33Z

modules.d/00_sh/module-setup.sh

@@ -0,0 +1,33 @@
+#!/bin/bash


Module should be called 00sh not 00_sh.

The module name '99~sh' is required for proper ordering of the code.
It is a meta module enforcing some~shell—not to be confused with any current sh.

LaszloGombos · 2023-06-01T23:57:00Z

modules.d/80test-root/module-setup.sh

@@ -22,6 +22,7 @@ install() {
    inst_binary "${dracutbasedir}/dracut-util" "/usr/bin/dracut-util"
    ln -s dracut-util "${initdir}/usr/bin/dracut-getarg"
    ln -s dracut-util "${initdir}/usr/bin/dracut-getargs"
+    [[ -L /bin/sh ]] || ln -sf dash /bin/sh


instead of this line, just make test-root module dependent on the "dash" module

depends() { echo "dash debug" }

Another PR is also making the same change for somewhat different reasons - #2354 . It is good to align on this between PRs.

This change might not even be necessary if you fix the dependency for the base module (see below).

This seems to have been resolved.

LaszloGombos · 2023-06-01T23:58:33Z

modules.d/99base/module-setup.sh

-        inst_multiple bash
-        (ln -s bash "${initdir}/bin/sh" || :)
-    fi
-
    # add common users in /etc/passwd, it will be used by nfs/ssh currently


Glad we can remove this now, but you should also make the base module dependent on the new "sh" module, like the following

depends() { echo "sh udev-rules" }

This is not needed with the refactored approach.

LaszloGombos · 2023-06-11T21:21:05Z

By chance I just run into the following (non-fatal) error running the following fro ma git checkout

dracut.sh --list-modules -l

/home/usr/canary/dracut-prlist/dracut-init.sh: line 255: -D: command not found
dracut[E]: FAILED:  -D /var/tmp/dracut.U7jRKD/initramfs /bin/sh
dracut[I]: Executing: /home/usr/canary/dracut-prlist/dracut.sh --list-modules -l

Just wondering would this PR also resolve this (as it moves some code into dracut module itself) ?

FGrose · 2023-06-12T00:23:31Z

@LaszloGombos re: /bin/sh not found
See PR #2394.

LaszloGombos · 2023-06-13T00:22:44Z

This is what I would recommend for depends (with not much error handling). Not sure how much error handling is actually needed and how likely that /bin/sh is broken..

Let's try to keep this simple first and just introduce the concept of a new module without adding a bunch of additional use-cases

depends() {                                                                                                                                                                                                  
    shells='dash bash mksh busybox'                                                                                                                                                                          
    for shell in $shells; do                                                                                                                                                                                 
        if dracut_module_included "$shell"; then                                                                                                                                                             
            echo "$shell"                                                                                                                                                                                    
            return 0                                                                                                                                                                                         
        fi                                                                                                                                                                                                   
    done                                                                                                                                                                                                     
                                                                                                                                                                                                             
    shell=$(realpath -e /bin/sh)                                                                                                                                                                             
    shell=${shell##*/}                                                                                                                                                                                       
                                                                                                                                                                                                             
    echo "$shell"                                                                                                                                                                                            
    return 0                                                                                                                                                                                                 
}

stale · 2023-07-14T23:50:04Z

This issue is being marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. If this is still an issue in the latest release of Dracut and you would like to keep it open please comment on this issue within the next 7 days. Thank you for your contributions.

Henrik66 · 2023-07-15T15:24:53Z

@FGrose Do you plan on working on this PR more based on feedback from @LaszloGombos ? I am asking because I am also available try to help with this PR. Thank you @FGrose .

FGrose · 2023-07-15T19:05:00Z

@FGrose Do you plan on working on this PR more based on feedback from @LaszloGombos ? I am asking because I am also available try to help with this PR. Thank you @FGrose .

@Henrik66 - Yes, I have a refactoring that I am testing now.

LaszloGombos · 2023-07-27T16:13:21Z

I am very sad to see a completly new complex rewrite instead of iterating over the previous version.

In order for this to ever land, we would need two independent approvals for:

This a use-case that upstream should solve
This a solution that we can maintain upstream

feat(module-postprocess): provide a postprocessing loop for modules

Can you please elaborate why it is not possible to solve this problem without introducing a new module interface ? What would be a use case that would fail without this complex modification in the core logic of dracut.

Do you see any use-case of postprocessing for any other dracut modules ?

@Henrik66 If you still interested I think you should explore #2368 (comment). That should at least make it easier to compare and contrast the two solutions.

LaszloGombos · 2023-07-28T12:23:26Z

@FGrose Perhaps can you make an independent PR just for

refactor(99squash): gather code into the squash module

This PR is simply too big and impacts too many parts and people for it to ever land with our current processes.

Provide code for postprocessing the build after all modules have been loaded by enrolling those modules with a postprocess() function in their module_setup.sh into the variable $mods_to_postprocess. Update the documentation in dracut.modules.7.asc and HACKING.md files. Also, include a missing dinfo message before including modules. This feature is demonstrated in three upcoming commits: feat(99~no~sh): allow a build without any command shell fix(00bash): fail if Bash below version 4.4 is linked to /bin/sh refactor(99squash): gather code into the squash module

Introduce a new meta module '99~sh' and assure that its depends() function runs at the end of the 'for_each_module_dir()' loop by using LC_COLLATE=C in that function so that the '99~sh' meta module sorts after other normally named modules. Background: Implicit installation of command shells may occur during the *** Resolving executable dependencies *** phase of dracut.sh with the default flag $DRACUT_RESOLVE_LAZY=1. Any module script file with a shebang interpreter directive will trigger the installation of that command interpreter, typically /bin/sh, unless that interpreter has already been installed. Such implicit installations bypass any conditions encoded in their dracut module dependency checks and should be precluded. If the dracut -m, --modules option is used (outside of -m 'auto'), no module-setup.sh level code is invoked during the 'for_each_module_dir check_module' loop for any modules other than those specified in the -m option arguments. For this reason, in this case invoke module_depends ~sh. To allow an initramfs to be built without any command shell, as currently allowed (such as when the -m option is used or when incrementally building the initramfs with --rebuild), a new meta module '99~no~sh' will be introduced in a following commit.

Install a null link to /bin/sh in order to satisfy the executable dependency checks and remove it in a postprocess() function. A no-command-shell-build may be desired when the -m, --modules option is used or when incrementally building an initramfs with --rebuild. This module provides an opt-out to the automatic specification of a command shell introduced with 99~sh.

At least bash-4.4 (from 2016-09-15) is required for proper xtrace logging when Bash is the initramfs command shell. This requirement was introduced in commit 10cf8e4 on 2022-12-30 with the feature, local -, which automates the restoration of local xtrace & other set options.

Move squash-module-specific code from dracut.sh to /99squash/module-setup.sh. Use dracut's new module_postprocess() feature. Make shell installation explicit, preferring busybox, if available. Clarify a comment about the dracut directory files. Also, make new shellcheck adjustments.

Only include bash into the generated initramfs if a dracut module explicitly depends on it. Implemented as a new dracut module that picks busybox shell on alpine. Based on the idea discussed upstream: dracutdevs/dracut#2368 (comment)

FGrose requested review from haraldh and johannbg as code owners May 28, 2023 20:10

github-actions bot added base Issues related to the base module bash Issues related to the bash module modules Issue tracker for all modules labels May 28, 2023

FGrose mentioned this pull request May 28, 2023

fix(mksh): implicit install requires printf #2362

Closed

FGrose force-pushed the shell branch from e394223 to d1b61f8 Compare May 28, 2023 20:41

FGrose force-pushed the shell branch from d1b61f8 to 1248575 Compare May 29, 2023 15:26

FGrose force-pushed the shell branch from 1248575 to cb03a14 Compare June 1, 2023 14:58

github-actions bot added the test Issues related to testing label Jun 1, 2023

FGrose force-pushed the shell branch from cb03a14 to dc7b816 Compare June 1, 2023 15:46

LaszloGombos reviewed Jun 1, 2023

View reviewed changes

stale bot added the stale communication is stuck label Jul 14, 2023

LaszloGombos removed the stale communication is stuck label Jul 15, 2023

LaszloGombos added this to the dracut-060 milestone Jul 15, 2023

FGrose marked this pull request as draft July 15, 2023 12:25

FGrose force-pushed the shell branch from dc7b816 to 37a7218 Compare July 27, 2023 02:25

github-actions bot added squash Issues related to the squash module and removed test Issues related to testing labels Jul 27, 2023

FGrose force-pushed the shell branch from 37a7218 to 72bb585 Compare July 27, 2023 02:45

FGrose force-pushed the shell branch from 72bb585 to 11c28aa Compare July 28, 2023 01:01

FGrose force-pushed the shell branch from 11c28aa to eeae801 Compare July 29, 2023 00:25

FGrose added 4 commits July 29, 2023 01:33

FGrose force-pushed the shell branch from eeae801 to 718077f Compare July 29, 2023 05:41

FGrose force-pushed the shell branch from 718077f to 55e50c1 Compare July 29, 2023 05:56

Henrik66 mentioned this pull request Jul 30, 2023

feat(sh): shell meta module #2459

Closed

3 tasks

LaszloGombos modified the milestones: dracut-060, dracut-061 Oct 30, 2023

LaszloGombos added the enhancement Issue adding new functionality label Nov 1, 2023

LaszloGombos removed this from the dracut-061 milestone Nov 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(assure_command_shell): assure proper installation of a shell #2368

feat(assure_command_shell): assure proper installation of a shell #2368

FGrose commented May 28, 2023 •

edited

Loading

LaszloGombos commented May 29, 2023

FGrose commented May 29, 2023

LaszloGombos commented May 29, 2023 •

edited

Loading

LaszloGombos left a comment •

edited

Loading

LaszloGombos Jun 1, 2023 •

edited

Loading

FGrose Jul 27, 2023 •

edited

Loading

LaszloGombos Jun 1, 2023 •

edited

Loading

FGrose Jul 27, 2023

LaszloGombos Jun 1, 2023 •

edited

Loading

FGrose Jul 27, 2023

LaszloGombos Jun 1, 2023 •

edited

Loading

FGrose Jul 27, 2023

LaszloGombos Jun 1, 2023

FGrose Jul 27, 2023

LaszloGombos Jun 1, 2023 •

edited

Loading

FGrose Jul 27, 2023

LaszloGombos Jun 1, 2023

FGrose Jul 27, 2023

LaszloGombos commented Jun 11, 2023

FGrose commented Jun 12, 2023

LaszloGombos commented Jun 13, 2023 •

edited

Loading

stale bot commented Jul 14, 2023

Henrik66 commented Jul 15, 2023

FGrose commented Jul 15, 2023

LaszloGombos commented Jul 27, 2023

LaszloGombos commented Jul 28, 2023 •

edited

Loading

		inst /bin/bash

		# local - (available since bash-4.4 2016-09-15) automates the

feat(assure_command_shell): assure proper installation of a shell #2368

Are you sure you want to change the base?

feat(assure_command_shell): assure proper installation of a shell #2368

Conversation

FGrose commented May 28, 2023 • edited Loading

Checklist

LaszloGombos commented May 29, 2023

FGrose commented May 29, 2023

LaszloGombos commented May 29, 2023 • edited Loading

LaszloGombos left a comment • edited Loading

Choose a reason for hiding this comment

LaszloGombos Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

FGrose Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

LaszloGombos Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaszloGombos Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaszloGombos Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaszloGombos Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaszloGombos commented Jun 11, 2023

FGrose commented Jun 12, 2023

LaszloGombos commented Jun 13, 2023 • edited Loading

stale bot commented Jul 14, 2023

Henrik66 commented Jul 15, 2023

FGrose commented Jul 15, 2023

LaszloGombos commented Jul 27, 2023

LaszloGombos commented Jul 28, 2023 • edited Loading

FGrose commented May 28, 2023 •

edited

Loading

LaszloGombos commented May 29, 2023 •

edited

Loading

LaszloGombos left a comment •

edited

Loading

LaszloGombos Jun 1, 2023 •

edited

Loading

FGrose Jul 27, 2023 •

edited

Loading

LaszloGombos Jun 1, 2023 •

edited

Loading

LaszloGombos Jun 1, 2023 •

edited

Loading

LaszloGombos Jun 1, 2023 •

edited

Loading

LaszloGombos Jun 1, 2023 •

edited

Loading

LaszloGombos commented Jun 13, 2023 •

edited

Loading

LaszloGombos commented Jul 28, 2023 •

edited

Loading