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

[ubu-chroot] Fix argument passing #7

Merged
merged 9 commits into from May 30, 2022
Merged

[ubu-chroot] Fix argument passing #7

merged 9 commits into from May 30, 2022

Conversation

Thaodan
Copy link
Contributor

@Thaodan Thaodan commented Mar 29, 2022

Fix argument parsing and a few other clean ups:

  • Invert Debian version check, remove Ubuntu 10.04 workaround
  • Remove 'exec' command
  • Sync passwd and shadow so login with su works
  • Create HOME dir also if not mounting it

Drop support for Ubuntu 10.04 and instead show warning if unknown Ubuntu
version was found.

Signed-off-by: Björn Bidar <bjorn.bidar@jolla.com>
It is deprecated since at least 7 years..

It was deprecated in mer-sdk-chroot in: sailfishos/sdk-setup@3fd5ecb
Copy link
Contributor

@sledges sledges left a comment

Choose a reason for hiding this comment

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

Looks good! One suggestion below.

Also it's funny how before your fix ubu-chroot -r /path/to/ubu bash -c "echo a; echo b" outputs


b

and after your fix it outputs

a
b

:)

Comment on lines 304 to 341


if grep --quiet \
--invert-match \
--word-regexp "wheezy\|jessie\|bullseye\|sid" \
"${uburoot}"/etc/debian_version ; then
# This is for ubuntu 12.04+ (arg quoting for sudo is different)
echo Unknown ubuntu version >&2
exit 1
fi
if [ $# -eq 0 ] ; then
set -- /usr/bin/env \
bash --init-file \
/parentroot/usr/share/ubu-chroot/mer-ubusdk-bash-setup -i
fi

setarch x86_64 chroot "${uburoot}" \
/usr/bin/env su -s /bin/bash -l $user -- \
-c "$(cat <<EOF
[ -d "$cwd" ] && cd "$cwd"
export MERSDKUBU=1

exec $(printf "%q " "${@}")
EOF
Copy link
Contributor

Choose a reason for hiding this comment

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

While trying to test this I got su: Authentication failure when entering ubu-chroot. Rewriting with sudo did the trick:

Suggested change
if grep --quiet \
--invert-match \
--word-regexp "wheezy\|jessie\|bullseye\|sid" \
"${uburoot}"/etc/debian_version ; then
# This is for ubuntu 12.04+ (arg quoting for sudo is different)
echo Unknown ubuntu version >&2
exit 1
fi
if [ $# -eq 0 ] ; then
set -- /usr/bin/env \
bash --init-file \
/parentroot/usr/share/ubu-chroot/mer-ubusdk-bash-setup -i
fi
setarch x86_64 chroot "${uburoot}" \
/usr/bin/env su -s /bin/bash -l $user -- \
-c "$(cat <<EOF
[ -d "$cwd" ] && cd "$cwd"
export MERSDKUBU=1
exec $(printf "%q " "${@}")
EOF
if grep --quiet \
--invert-match \
--word-regexp "wheezy\|jessie\|bullseye\|sid" \
"${uburoot}"/etc/debian_version ; then
# This is for ubuntu 12.04+ (arg quoting for sudo is different)
echo Unknown ubuntu version >&2
exit 1
fi
if [ $# -eq 0 ] ; then
set -- /usr/bin/env \
bash --init-file \
/parentroot/usr/share/ubu-chroot/mer-ubusdk-bash-setup -i
fi
setarch x86_64 chroot "${uburoot}" \
/usr/bin/sudo -i -u $user \
exec bash -c "export MERSDKUBU=1; if [ -d \"$cwd\" ]; then cd \"$cwd\"; fi; exec $(printf "%q " "$@")"

The last line is the only main change, but I had to also remove the EOF again to make this suggestion into an applicable patch:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do that instead of an heredoc without escaping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While trying to test this I got su: Authentication failure when entering ubu-chroot. Rewriting with sudo did the trick:

The last line is the only main change, but I had to also remove the EOF again to make this suggestion into an applicable patch:)

pwconv should fix this, I'll look.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do that instead of an heredoc without escaping?

That worked only after I split commands with ;, because it becomes a one-liner when using this mnemonic:

setarch x86_64 chroot ${uburoot} \
        /usr/bin/sudo -i -u $user \
                exec bash -c "$(cat <<END
export MERSDKUBU=1;
[[ -d "$cwd" ]] && cd "$cwd";
exec $(printf "%q " "$@")
END
)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is because of the redundant exec you have there, it will spawn another shell before executing bash.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing exec still fails without ;

setarch x86_64 chroot ${uburoot} \
        /usr/bin/sudo -i -u $user \
                bash -c "$(cat <<END
export MERSDKUBU=1
[[ -d "$cwd" ]] && cd "$cwd"
exec $(printf "%q " "$@")
END
)"
$ ubu-chroot -r /path/to/ubu
bash: line 0: export: `-d': not a valid identifier
bash: line 0: export: `/home/user': not a valid identifier
bash: line 0: export: `]]': not a valid identifier
$ 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please test gain, should be fixed now.

Copy link
Contributor

@sledges sledges left a comment

Choose a reason for hiding this comment

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

It works now thanks. I recommend to provide a safe-guard PR to dhd submodule before merging this PR.
And as you already know, after merging this PR, ubu-chroot -r /path/to/ubu "echo a; echo b" will no longer work and will require adding sh -c.

'mer-android-chroot --help' claims it expects 'CMD [ARGS]...' but actually it
treated the (concatenated) arguments as a single argument to 'sh -c',
failing to e.g. properly pass arguments with spaces.

This also deduplicates the code for the two use cases.

Similar as in: sailfishos/sdk-setup@01310c8

Signed-off-by: Björn Bidar <bjorn.bidar@jolla.com>
Signed-off-by: Björn Bidar <bjorn.bidar@jolla.com>
Signed-off-by: Björn Bidar <bjorn.bidar@jolla.com>
Read: man 1 su

Signed-off-by: Björn Bidar <bjorn.bidar@jolla.com>
@Thaodan Thaodan force-pushed the refactor_entershell branch 2 times, most recently from d18254a to dceabe4 Compare April 5, 2022 11:27
@Thaodan
Copy link
Contributor Author

Thaodan commented Apr 5, 2022 via email

rpm/android-tools-hadk.spec Outdated Show resolved Hide resolved
Signed-off-by: Björn Bidar <bjorn.bidar@jolla.com>
If a user has an older ubu-chroot --pty might not be present in su.

Signed-off-by: Björn Bidar <bjorn.bidar@jolla.com>
Signed-off-by: Björn Bidar <bjorn.bidar@jolla.com>
Copy link
Contributor

@sledges sledges left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this properly.

@Thaodan Thaodan merged commit 43b1c93 into master May 30, 2022
@mkosola mkosola deleted the refactor_entershell branch May 31, 2022 08:43
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.

None yet

2 participants