Skip to content

Commit

Permalink
Port from cffi to the cpython crate. (#9593)
Browse files Browse the repository at this point in the history
### Problem

When initially porting the engine to rust, we used `cffi` because it had a mode that avoided the need to actually link against Python. But somewhere along the way, we began linking against Python, and needed to add C shims to expose a proper module. Though complex, it has served us well for a few years.

But during that same time, Rust python-integration crates like `cpython` have matured, providing significant safety and usability benefits over a raw C API.

### Solution

Port to the `cpython` crate. When compared to usage of CFFI, this eliminates the passing of raw pointers back and forth across the ffi boundary (improving errors), and allows for direct access to Python objects from Rust (while the GIL is acquired). The `cpython` crate was chosen over PyO3 because it doesn't require a nightly Rust compiler, and is [vouched for](#7369 (comment)) by the mercurial developers.

The `cpython` crate does not use PEP-384, and updating it to do so would be a significant undertaking: for example, the `PyObject::call` interface relies on `PyTuple`, which is not present under PEP-384. It would likely also erase some of the performance and usability benefit of this API. So in order to switch to this crate, @Eric-Arellano did a ton of pre-work (culminating in #9881) to switch away from publishing ABI3 wheels, and toward publishing a wheel-per-python3.

### Result

`./pants list ::` is 8-10% faster, usage is easier and safer, and there is less code to maintain. Additionally, since [pyoxidizer](https://pyoxidizer.readthedocs.io/en/latest/index.html) (see #7369) relies on the `cpython` crate, this helps to unblock potentially switching to distributing Pants as a native binary with an embedded Python.
  • Loading branch information
stuhood authored May 31, 2020
1 parent 56cd35f commit 9f2c65b
Show file tree
Hide file tree
Showing 37 changed files with 2,576 additions and 3,999 deletions.
9 changes: 1 addition & 8 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -528,10 +528,6 @@ jobs:
if: commit_message !~ /\[ci skip-rust-tests\]/
name: Clippy (Rust linter)
os: linux
python:
- '2.7'
- '3.6'
- '3.7'
script:
- ./build-support/bin/ci.py --clippy
stage: Test Pants
Expand Down Expand Up @@ -1597,13 +1593,10 @@ jobs:
dist: xenial
env:
- CACHE_NAME=rust_tests.linux
- LD_LIBRARY_PATH="/opt/python/3.6.7/lib:${LD_LIBRARY_PATH}"
if: commit_message !~ /\[ci skip-rust-tests\]/
name: Rust tests - Linux
os: linux
python:
- '2.7'
- '3.6'
- '3.7'
script:
- ./build-support/bin/ci.py --rust-tests
stage: Test Pants
Expand Down
1 change: 0 additions & 1 deletion 3rdparty/python/requirements.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
ansicolors==1.1.8
beautifulsoup4>=4.6.0,<4.7
cffi==1.14.0
coverage>=4.5,<4.6
dataclasses==0.6
docutils>=0.15,<0.17
Expand Down
9 changes: 7 additions & 2 deletions build-support/bin/generate_travis_yml.py
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,6 @@ def linux_fuse_shard() -> Dict:
"os": "linux",
"dist": "xenial",
"sudo": "required",
"python": ["2.7", "3.6", "3.7"],
"before_install": _linux_before_install()
+ [
"sudo apt-get install -y pkg-config fuse libfuse-dev",
Expand Down Expand Up @@ -672,7 +671,13 @@ def rust_tests_linux() -> Dict:
**_RUST_TESTS_BASE,
**linux_fuse_shard(),
"name": "Rust tests - Linux",
"env": ["CACHE_NAME=rust_tests.linux"],
"env": [
"CACHE_NAME=rust_tests.linux",
# TODO: Despite being successfully linked at build time, the Linux rust tests
# shard is unable to locate `libpython3.6m.so.1.0` at runtime without this pointer:
# OSX appears to be fine.
'LD_LIBRARY_PATH="/opt/python/3.6.7/lib:${LD_LIBRARY_PATH}"',
],
}


Expand Down
27 changes: 0 additions & 27 deletions build-support/bin/native/bootstrap_cffi.sh

This file was deleted.

7 changes: 4 additions & 3 deletions build-support/bin/native/bootstrap_code.sh
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@ function _build_native_code() {
# Change into $REPO_ROOT in a subshell. This is necessary so that we're in a child of a
# directory containing the rust-toolchain file, so that rustup knows which version of rust we
# should be using.
# NB: See Cargo.toml with regard to the `extension-module` feature.
cd "${REPO_ROOT}"
"${REPO_ROOT}/build-support/bin/native/cargo" build ${MODE_FLAG} \
--manifest-path "${NATIVE_ROOT}/Cargo.toml" -p engine_cffi
"${REPO_ROOT}/build-support/bin/native/cargo" build --features=extension-module ${MODE_FLAG} \
--manifest-path "${NATIVE_ROOT}/Cargo.toml" -p engine
) || die
echo "${NATIVE_ROOT}/target/${MODE}/libengine_cffi.${LIB_EXTENSION}"
echo "${NATIVE_ROOT}/target/${MODE}/libengine.${LIB_EXTENSION}"
}

function bootstrap_native_code() {
Expand Down
11 changes: 11 additions & 0 deletions build-support/bin/native/cargo.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ set -e
REPO_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")" && cd ../../.. && pwd -P)

export PY=${PY:-python3}
# Consumed by the cpython crate.
export PYTHON_SYS_EXECUTABLE="${PY}"

# Exports:
# + CARGO_HOME: The CARGO_HOME of the Pants-controlled rust toolchain.
Expand All @@ -15,8 +17,17 @@ export PY=${PY:-python3}
# shellcheck source=build-support/bin/native/bootstrap_rust.sh
source "${REPO_ROOT}/build-support/bin/native/bootstrap_rust.sh"

# Exposes:
# + activate_pants_venv: Activate a virtualenv for pants requirements, creating it if needed.
#
# This is necessary for any `cpython`-dependent crates, which need a python interpeter on the PATH.
# shellcheck source=build-support/pants_venv
source "${REPO_ROOT}/build-support/pants_venv"

bootstrap_rust >&2

activate_pants_venv

download_binary="${REPO_ROOT}/build-support/bin/download_binary.sh"

# The following is needed by grpcio-sys and we have no better way to hook its build.rs than this;
Expand Down
7 changes: 7 additions & 0 deletions build-support/pants_venv
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ MESSAGE
}

function activate_pants_venv() {
# Allow for multiple calls to `activate_pants_venv`.
if [ -n "$VENV_ACTIVATED_FOR_PANTS" ]; then
return
else
export VENV_ACTIVATED_FOR_PANTS="true"
fi

fingerprint=""
for req in "${REQUIREMENTS[@]}"; do
fingerprint="${fingerprint}$(fingerprint_data < "${req}")"
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/backend/python/rules/pex.py
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ async def create_pex(
argv.extend(pex_debug.iter_pex_args())

if python_setup.resolver_jobs:
argv.extend(["--jobs", python_setup.resolver_jobs])
argv.extend(["--jobs", str(python_setup.resolver_jobs)])

if python_setup.manylinux:
argv.extend(["--manylinux", python_setup.manylinux])
Expand Down
4 changes: 1 addition & 3 deletions src/python/pants/cache/artifact.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,6 @@ def extract(self):
# Note(yic): unlike the python implementation before, now we do not update self._relpath
# after the extraction.
try:
self.NATIVE_BINARY.decompress_tarball(
self._tarfile.encode(), self.artifact_extraction_root.encode()
)
self.NATIVE_BINARY.decompress_tarball(self._tarfile, self.artifact_extraction_root)
except Exception as e:
raise ArtifactError("Extracting artifact failed:\n{}".format(e))
1 change: 0 additions & 1 deletion src/python/pants/engine/internals/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ python_library(
sources=['native.py'],
dependencies=[
':native_engine_so',
'3rdparty/python:cffi',
'3rdparty/python:setuptools',
'src/python/pants/engine:addresses',
'src/python/pants/engine:fs',
Expand Down
2 changes: 0 additions & 2 deletions src/python/pants/engine/internals/engine_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,6 @@ def test_include_trace_error_raises_error_with_trace(self):
1 Exception encountered:
Traceback (most recent call last):
File LOCATION-INFO, in call
val = func(*args)
File LOCATION-INFO, in nested_raise
fn_raises(x)
File LOCATION-INFO, in fn_raises
Expand Down
Loading

0 comments on commit 9f2c65b

Please sign in to comment.