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

thisroot.sh: if exe contains qemu, use /proc/$$/comm instead #14251

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

wdconinc
Copy link
Contributor

This Pull request:

This PR aims to address #14085 by making thisroot.sh identify the shell correctly even when run under qemu, for example in emulated docker containers.

Changes or fixes:

When qemu is run, then /proc/$$/comm contains the name of the command that is emulated.

Checklist:

  • tested changes locally
  • updated the docs (if necessary) N/A

This PR fixes #14085.

Tests

Before:

(Fri Dec-15 2:11:32pm)-(CPU 20.2%:0:Net 1)-(root:/)-(64K:21)
> source thisroot.sh
ERROR: must "cd where/root/is" before calling ". bin/thisroot.sh" for this version of "aarch64-binfmt-P"!
(ERROR)-(Exit Code 1)-(General error)
(Fri Dec-15 2:11:37pm)-(CPU 20.2%:0:Net 1)-(root:/)-(64K:21)

After:

(Fri Dec-15 2:12:57pm)-(CPU 20.2%:0:Net 1)-(root:/)-(16M:23)
> source thisroot.sh
(Fri Dec-15 2:13:00pm)-(CPU 20.2%:0:Net 1)-(root:/)-(16M:23)

@phsft-bot
Copy link

Can one of the admins verify this patch?

@kkauder
Copy link

kkauder commented Dec 15, 2023

Not an admin, but I tested it and it does fix #14085

Copy link

Test Results

       10 files         10 suites   2d 1h 17m 38s ⏱️
  2 485 tests   2 485 ✔️ 0 💤 0
23 773 runs  23 773 ✔️ 0 💤 0

Results for commit 8d87b0b.

@guitargeek
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@guitargeek
Copy link
Contributor

Let's see if this doesn't break anything. @wdconinc, @kkauder, if we merge this, do you need a backport to the 6.30 branch so this makes it into the 6.30.02 patch release?

@wdconinc
Copy link
Contributor Author

I'd leave the backport decision up to you. We can patch this into our user environments without a backport too.

Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Thanks! If this fixes the shell detection when using qemu, then it should be merged 👍

@guitargeek
Copy link
Contributor

I'd leave the backport decision up to you. We can patch this into our user environments without a backport too.

Ok, I prefer backports over people patching ROOT 🙂 There is a very low risk of breaking something here, since the PF only affects the case where exe contains qemu, which simply didn't work before.

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.

thisroot.sh does not recognize bash when running in qemu-x86_64
4 participants