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

gh-129156: Fix variable quoting in android-env.sh script #129321

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented Jan 26, 2025

As reported in #129156, the lack of quoting can cause problems.

I added quoting to variables throughout the script. I used shellcheck to confirm that there were not remaining cases.

@zanieb
Copy link
Contributor Author

zanieb commented Jan 26, 2025

This is a draft because I need to setup a cross-build for Android and test everything works still.

Tested with ./android.py build x86_64-linux-android

Comment on lines -93 to +98
if [ $(uname) = "Darwin" ]; then
export CPU_COUNT=$(sysctl -n hw.ncpu)
if [ "$(uname)" = "Darwin" ]; then
CPU_COUNT="$(sysctl -n hw.ncpu)"
export CPU_COUNT
else
export CPU_COUNT=$(nproc)
CPU_COUNT="$(nproc)"
export CPU_COUNT
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The export is separated from the assignment as a failed command would be ignored otherwise (see https://www.shellcheck.net/wiki/SC2155)

@zanieb zanieb marked this pull request as ready for review January 26, 2025 18:19
@zanieb
Copy link
Contributor Author

zanieb commented Jan 26, 2025

It'd still be nice to test against the shell in the report, but I couldn't reproduce the failure they described with several shells.

@mhsmith
Copy link
Member

mhsmith commented Jan 26, 2025

!buildbot android

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mhsmith for commit 1978ae5 🤖

The command will test the builders whose names match following regular expression: android

The builders matched are:

  • AMD64 Android PR
  • aarch64 Android PR

@mhsmith mhsmith added the needs backport to 3.13 bugs and security fixes label Jan 26, 2025
Copy link
Member

@mhsmith mhsmith 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 to me. Thanks for the fixes!

@freakboy3742: If you're happy, please merge and backport.

Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Possibly a little overzealous on some of the quoting (API level can't have spaces pretty much by definition), but it doesn't hurt to be consistent. Thanks for the PR!

@freakboy3742 freakboy3742 merged commit a49225c into python:main Jan 27, 2025
51 checks passed
@miss-islington-app
Copy link

Thanks @zanieb for the PR, and @freakboy3742 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington-app
Copy link

Sorry, @zanieb and @freakboy3742, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker a49225cc66680129f129d1fcf6e20afb37a1a877 3.13

freakboy3742 pushed a commit to freakboy3742/cpython that referenced this pull request Jan 27, 2025
…pt (pythonGH-129321)

Adds quoting to `android-env.sh` to protect against spaces in paths.
(cherry picked from commit a49225c)

Co-authored-by: Zanie Blue <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jan 27, 2025

GH-129332 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jan 27, 2025
freakboy3742 added a commit that referenced this pull request Jan 27, 2025
…-129321) (#129332)

Adds quoting to `android-env.sh` to protect against spaces in paths.
(cherry picked from commit a49225c)

Co-authored-by: Zanie Blue <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

./android.py configure-host HOST fails because export CXXFLAGS is unquoted
4 participants