Skip to content

take into account alternate sysroot for /bin/bash used by run_cmd#4646

Merged
boegel merged 5 commits intoeasybuilders:developfrom
casparvl:exec_command_with_sysroot
Sep 18, 2024
Merged

take into account alternate sysroot for /bin/bash used by run_cmd#4646
boegel merged 5 commits intoeasybuilders:developfrom
casparvl:exec_command_with_sysroot

Conversation

@casparvl
Copy link
Copy Markdown
Contributor

@casparvl casparvl commented Sep 18, 2024 •
edited
Loading

EasyBuild's run_cmd runs commands in a subshell. The shell used is defined in the exec_cmd. However, in context where the use of a sysroot is configured, one would want to use <sysroot>/bin/bash instead. This PR implements that change. Note that since run_cmd is also called a few times before set_up_configuration is called, we need to be able to disable the check for the build_option('sysroot') for those instances (similar to how this is already don for build_option('hooks')).

Solves this issue.

Edit: hmmm, I made this PR, and then realized, this is probably one of the components that is very different in EB 5.0. And it seems to be indeed...

Someone with more EB 5.0 knowledge needs to give me a pointer of how to fix this for EB 5.0 :)

casparvl casparvl and others added 3 commits September 18, 2024 10:20
Make sure the run_cmd respects sysroot when picking the shell, i.e. u…
7bf4073 
…se sysroot/bin/bash instead of /bin/bash if sysroot is set
Make sure we only check for sysroot after set_up_configuration is cal…
73a53a8 
…led. Any command run before that with run_cmd should use the regular /bin/bash
Also add with_sysroot=False here
347e64c
@casparvl casparvl added the EESSI Related to EESSI project label Sep 18, 2024
@casparvl
Copy link
Copy Markdown
Contributor Author

casparvl commented Sep 18, 2024

Ok, this issue actually does not exist for EB 5.0: the whole run_cmd is now replaced by run_shell_cmd, and that has a different way of determining the shell command. See this piece of code:

    # use bash as shell instead of the default /bin/sh used by subprocess.run
    # (which could be dash instead of bash, like on Ubuntu, see https://wiki.ubuntu.com/DashAsBinSh)
    # stick to None (default value) when not running command via a shell
    if use_bash:
        bash = shutil.which('bash')
        _log.info(f"Path to bash that will be used to run shell commands: {bash}")
        executable, shell = bash, True
    else:
        executable, shell = None, False

at least for EESSI, the result is that the bash from the prefix is used. That means our issue no longer exists. I'm closing this PR, as it is not relevant.

@casparvl casparvl closed this Sep 18, 2024
@casparvl casparvl mentioned this pull request Sep 18, 2024
Closed
@boegel boegel added this to the 4.x milestone Sep 18, 2024
@boegel boegel added the bug fix label Sep 18, 2024
@casparvl casparvl reopened this Sep 18, 2024
@boegel
Copy link
Copy Markdown
Member

boegel commented Sep 18, 2024

@casparvl As discussed, I think it's worth considering this to be merged into develop, since there's a (small) window of opportunity to get this included in EasyBuild v4.9.4 (which we'll be releasing because of a bug that was introduced in the GCC easyblock, see here).

I'll take a thorough look into this right now...

@boegel
be a bit more careful with sysroot in run_cmd, add logging + add dedi…
f7c3cb1 
…cated test for run_cmd using with_sysroot=True
@boegel boegel modified the milestones: 4.x, release after 4.9.3 Sep 18, 2024
@boegel boegel changed the title Exec command with sysroot take into account alternate sysroot for /bin/bash used by run_cmd Sep 18, 2024
@boegel boegel modified the milestones: release after 4.9.4, 4.9.4 Sep 18, 2024
boegel
boegel approved these changes Sep 18, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@boegel boegel merged commit 3b7295f into easybuilders:develop Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@boegel boegel boegel approved these changes

Assignees

No one assigned

Labels

bug fix EESSI Related to EESSI project

Projects

None yet

Milestone

4.9.4

Development

Successfully merging this pull request may close these issues.

2 participants

@casparvl @boegel