If the host_env feature is enabled, then enable the _ctypes module on the Android platform.#7402
Conversation
📝 WalkthroughWalkthroughThe pull request adds Android platform support to the RustPython VM by extending platform-specific target dependencies and module configurations to include the Android operating system alongside Linux, macOS, and Windows. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Cargo.toml (1)
15-15: Consider propagating torustpython-stdlibfor consistency.Other features like
host_env(line 14) propagate to bothrustpython-vmandrustpython-stdlib:host_env = ["rustpython-vm/host_env", "rustpython-stdlib?/host_env"]While
ctypesonly propagates torustpython-vm. Sincerustpython-stdlib/ctypesjust forwards torustpython-vm/ctypes, this works correctly, but for consistency you might consider:ctypes = ["rustpython-vm/ctypes", "rustpython-stdlib?/ctypes"]This is a minor consistency suggestion and not required for correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` at line 15, Update the Cargo.toml feature entry for "ctypes" so it also propagates to rustpython-stdlib like other features do; specifically modify the "ctypes" feature definition (currently referencing "rustpython-vm/ctypes") to include the rustpython-stdlib propagation (add "rustpython-stdlib?/ctypes") so the feature list becomes ["rustpython-vm/ctypes", "rustpython-stdlib?/ctypes"] and maintains consistency with other features such as "host_env".crates/vm/Cargo.toml (1)
87-88: Potential redundancy with target-specific dependencies.These optional dependencies overlap with the non-optional target-specific dependencies on lines 111-113:
[target.'cfg(all(any(target_os = "linux", target_os = "macos", target_os = "windows"), not(any(target_env = "musl", target_env = "sgx"))))'.dependencies] libffi = { workspace = true, features = ["system"] } libloading = "0.9"On supported platforms (Linux/macOS/Windows, non-musl, non-SGX),
libffiandlibloadingare always included regardless of thectypesfeature. The optional deps here are only meaningful for platforms outside that target condition where a user explicitly enables--features ctypes.This design works but creates a subtle distinction: the
ctypesfeature is only necessary for "unsupported" platforms. Consider adding a comment to clarify this intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/Cargo.toml` around lines 87 - 88, The optional dependencies libffi and libloading declared in the crate root (libffi = { workspace = true, features = ["system"], optional = true } and libloading = { version = "0.9", optional = true }) overlap with the non-optional target-specific dependencies under the target.'cfg(... )'.dependencies block, so add a brief comment above the optional deps explaining that they exist to enable the "ctypes" feature on unsupported platforms (i.e., when the target cfg does not match Linux/macOS/Windows non-musl/non-SGX) and that on supported platforms the target-specific section will always supply these crates regardless of the ctypes feature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/mod.rs`:
- Around line 41-49: Document that enabling the "ctypes" feature bypasses the
platform guards around the mod _ctypes and therefore may try to build
dependencies (libffi, libloading) that are unsupported on musl and SGX targets;
update Cargo.toml (feature description for "ctypes") and/or the README to
explicitly state the platform constraints and the potential for compile errors
on musl/SGX when users pass --features ctypes, and mention that the conditional
cfg(any(feature = "ctypes", ...)) in mod _ctypes is the reason this opt-in can
override the target checks.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 15: Update the Cargo.toml feature entry for "ctypes" so it also
propagates to rustpython-stdlib like other features do; specifically modify the
"ctypes" feature definition (currently referencing "rustpython-vm/ctypes") to
include the rustpython-stdlib propagation (add "rustpython-stdlib?/ctypes") so
the feature list becomes ["rustpython-vm/ctypes", "rustpython-stdlib?/ctypes"]
and maintains consistency with other features such as "host_env".
In `@crates/vm/Cargo.toml`:
- Around line 87-88: The optional dependencies libffi and libloading declared in
the crate root (libffi = { workspace = true, features = ["system"], optional =
true } and libloading = { version = "0.9", optional = true }) overlap with the
non-optional target-specific dependencies under the target.'cfg(...
)'.dependencies block, so add a brief comment above the optional deps explaining
that they exist to enable the "ctypes" feature on unsupported platforms (i.e.,
when the target cfg does not match Linux/macOS/Windows non-musl/non-SGX) and
that on supported platforms the target-specific section will always supply these
crates regardless of the ctypes feature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2641b466-5ad6-4df5-bee3-2485151d769b
📒 Files selected for processing (4)
Cargo.tomlcrates/stdlib/Cargo.tomlcrates/vm/Cargo.tomlcrates/vm/src/stdlib/mod.rs
crates/vm/src/stdlib/mod.rs
Outdated
| #[cfg(any( | ||
| feature = "ctypes", | ||
| all( | ||
| feature = "host_env", | ||
| any(target_os = "linux", target_os = "macos", target_os = "windows"), | ||
| not(any(target_env = "musl", target_env = "sgx")) | ||
| ) | ||
| ))] | ||
| mod _ctypes; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if _ctypes module or its submodules have any platform guards that would protect against musl/sgx
echo "=== Checking _ctypes module for platform guards ==="
rg -n "cfg.*musl|cfg.*sgx" crates/vm/src/stdlib/_ctypes/
echo ""
echo "=== Checking top-level imports in _ctypes files ==="
rg -n "^use (libffi|libloading)" crates/vm/src/stdlib/_ctypes/Repository: RustPython/RustPython
Length of output: 454
🏁 Script executed:
#!/bin/bash
# Read the mod.rs file to check the cfg conditions
echo "=== Reading crates/vm/src/stdlib/mod.rs around lines 41-49 ==="
sed -n '35,55p' crates/vm/src/stdlib/mod.rs
echo ""
echo "=== Checking for second cfg block around lines 91-98 ==="
sed -n '85,105p' crates/vm/src/stdlib/mod.rs
echo ""
echo "=== Full view of cfg conditions for _ctypes ==="
rg -B2 -A8 'mod _ctypes' crates/vm/src/stdlib/mod.rsRepository: RustPython/RustPython
Length of output: 1668
🏁 Script executed:
#!/bin/bash
# Check if _ctypes.rs has any top-level cfg guards
echo "=== Checking _ctypes.rs for top-level cfg ==="
head -20 crates/vm/src/stdlib/_ctypes.rs
echo ""
echo "=== Check if libffi and libloading are available on musl/sgx ==="
# Check if there are any feature flags or conditional compilation for these deps
rg -n "libffi|libloading" Cargo.toml | head -20Repository: RustPython/RustPython
Length of output: 609
🏁 Script executed:
#!/bin/bash
# Check for documentation about ctypes feature
echo "=== Searching for ctypes in README or docs ==="
find . -name "README*" -o -name "Cargo.toml" -o -name "FEATURES*" | head -10 | xargs grep -l "ctypes" 2>/dev/null || echo "No direct ctypes documentation found"
echo ""
echo "=== Check Cargo.toml for ctypes feature definition ==="
rg -A5 '\[features\]' Cargo.toml | grep -A100 "ctypes"Repository: RustPython/RustPython
Length of output: 294
🏁 Script executed:
#!/bin/bash
# Check if there are any comments in mod.rs explaining this design
echo "=== Searching for comments around the ctypes cfg condition ==="
sed -n '35,55p' crates/vm/src/stdlib/mod.rs | cat -n
echo ""
echo "=== Check if libloading/libffi have musl support ==="
rg -B3 -A3 "libffi|libloading" Cargo.tomlRepository: RustPython/RustPython
Length of output: 1108
🏁 Script executed:
#!/bin/bash
# Check if there are any issues or PRs mentioning musl/sgx and ctypes
echo "=== Searching for any musl-related comments in stdlib ==="
rg -n "musl" crates/vm/src/stdlib/ | head -20
echo ""
echo "=== Check if libffi/libloading have documented musl issues ==="
# Check the Cargo.lock or dependency versions for any indicators
rg -B5 -A5 "libffi.*5\|libloading" Cargo.lock 2>/dev/null | head -30 || echo "Cargo.lock check skipped"Repository: RustPython/RustPython
Length of output: 1938
🏁 Script executed:
#!/bin/bash
# Check if there's any issue or discussion about ctypes and musl in git history or issues
echo "=== Check if there's context in _ctypes.rs about musl/sgx ==="
grep -n "musl\|sgx\|platform" crates/vm/src/stdlib/_ctypes.rs || echo "No platform mentions in _ctypes.rs"
echo ""
echo "=== Verify the exact behavior: what happens when ctypes feature is enabled on musl ==="
# Check if there's a CI configuration that tests this
find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "ctypes\|musl" 2>/dev/null | head -5Repository: RustPython/RustPython
Length of output: 396
🏁 Script executed:
#!/bin/bash
# Check CI configuration for ctypes testing
echo "=== Checking CI for ctypes feature tests ==="
grep -n "ctypes" .github/workflows/ci.yaml
echo ""
echo "=== Checking if musl builds are tested ==="
grep -n "musl" .github/workflows/ci.yaml | head -20
echo ""
echo "=== Check if ctypes and musl are tested together ==="
grep -B5 -A5 "ctypes" .github/workflows/ci.yaml | grep -A10 -B10 "musl"Repository: RustPython/RustPython
Length of output: 282
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or indication of libffi/libloading support on musl
echo "=== Check Cargo.toml dependencies for any musl-related features ==="
grep -B2 -A2 "libffi\|libloading" Cargo.toml
echo ""
echo "=== Look for any conditional dependencies based on target_env ==="
sed -n '/^\[target\./,/^\[/p' Cargo.toml | grep -A5 "musl"
echo ""
echo "=== Check if the ctypes feature is explicitly documented anywhere ==="
find . -name "*.md" -type f | xargs grep -i "ctypes" 2>/dev/null | head -10Repository: RustPython/RustPython
Length of output: 716
Document the platform constraints for the ctypes feature.
The feature = "ctypes" flag bypasses platform checks and allows enabling ctypes on musl and SGX targets. Since _ctypes and its submodules unconditionally depend on libffi and libloading (which are not available on these platforms), users who enable --features ctypes on musl or SGX will encounter compile-time errors. This appears intentional—to allow users to opt-in despite platform restrictions—but should be documented in Cargo.toml or the project README so users understand the platform requirements before attempting to enable this feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/mod.rs` around lines 41 - 49, Document that enabling the
"ctypes" feature bypasses the platform guards around the mod _ctypes and
therefore may try to build dependencies (libffi, libloading) that are
unsupported on musl and SGX targets; update Cargo.toml (feature description for
"ctypes") and/or the README to explicitly state the platform constraints and the
potential for compile errors on musl/SGX when users pass --features ctypes, and
mention that the conditional cfg(any(feature = "ctypes", ...)) in mod _ctypes is
the reason this opt-in can override the target checks.
EtherealPsyche
left a comment
There was a problem hiding this comment.
libffi can actually be used on Android systems, but why is the _ctypes module only enabled when
cfg(all(any(target_os = "linux", target_os = "macos", target_os = "windows"),
not(any(target_env = "musl", target_env = "sgx")))) is satisfied?
For example, Termux provides a libffi (-static) package that can be used to link against a precompiled libffi.
This modification was originally done on my Android phone using Termux, but I accidentally lost the original changes.
Now I have recreated the changes on my computer from memory and created a PR, so there may be mistakes (previously it worked fine when tested on Termux).
youknowone
left a comment
There was a problem hiding this comment.
I believe the ideal cfg for ctypes will be only cfg(feature = "host_env").
Could you please remove ctypes feature but add android (or termux) to the cfg condition? I believe it is not excluded by intend.
|
@youknowone should we start to officially support android as a platform? (like cpython does it from 3.13) |
ShaharNaveh
left a comment
There was a problem hiding this comment.
Just line @youknowone said, I don't see the benefit of having a feature of ctypes, but to gate it behind the host_env feature, unless we would want to go with a feature gate per module (which has it's pros&cons)
|
How do you define official and unofficial platform? If you mean enhancing CI to guarantee it works, I agree. |
https://peps.python.org/pep-0738/ And sure, I'll try to implement a CI step that checks CI on android as well |
… introducing a new feature
|
Okay, let's simply add |
ctypes to force enable _ctypes modulehost_env feature is enabled, then enable the _ctypes module on the Android platform.
youknowone
left a comment
There was a problem hiding this comment.
Thank you so much! welcome to RustPython project
Summary by CodeRabbit
New Features
Chores