Skip to content

Change examples in accordance to review comments in #348 #352

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.

Sign up for GitHub

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

Merged
merged 33 commits into from
Aug 19, 2023
Merged

Change examples in accordance to review comments in #348 #352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
b150e62
Rename hello-world example to hello-world-setuppy
abravalheri Aug 11, 2023
c1af5a7
Rename hello-world-pyprojecttoml example to hello-world
abravalheri Aug 11, 2023
8530b86
Use pyproject.toml for hello-world-script example
abravalheri Aug 11, 2023
41db159
Use pyproject.toml for namespace_package example
abravalheri Aug 11, 2023
c76af32
Use pyproject.toml for html-py-ever example
abravalheri Aug 11, 2023
5c1cce4
Avoid setup.py install in hello-world-setuppy example
abravalheri Aug 11, 2023
624f5db
Avoid setup.py install in rust_with_cffi example
abravalheri Aug 11, 2023
c7c9c7f
Modify noxfile to use build instead of running setup.py
abravalheri Aug 11, 2023
ecba282
Adapt GitHub workflows config to the adsence of setup.py
abravalheri Aug 11, 2023
3772b27
Adequate test-mingw to new hello-world example
abravalheri Aug 11, 2023
f9fb03e
Remove other direct uses of 'setup.py' in workflows/ci.yml
abravalheri Aug 11, 2023
e90228c
Migrate the hello-world-script example to a 'rust' dir
abravalheri Aug 11, 2023
b28f5a9
Migrate the namespace_package example to a 'rust' dir
abravalheri Aug 11, 2023
b85e7bf
Migrate the rust_with_cffi example to a 'rust' dir
abravalheri Aug 11, 2023
95de1bd
Migrate the html-py-ever example to a 'rust' dir
abravalheri Aug 11, 2023
fdc9950
Migrate the hello-world-setuppy example to a 'rust' dir
abravalheri Aug 11, 2023
a075252
Avoid build isolation for test-crossenv in CI workflow
abravalheri Aug 14, 2023
27dd25d
Ensure cffi is installed in test-crossenv
abravalheri Aug 14, 2023
86748d3
List all contents of wheel to facilitate debugging
abravalheri Aug 14, 2023
831918c
Account for `bdist_wheel.plat_name` when given by config file
abravalheri Aug 14, 2023
dc2607d
Account for possibility bdist_wheel is not installed
abravalheri Aug 14, 2023
8930627
Use 'cross-pip wheel' for test-crossenv
abravalheri Aug 14, 2023
71e28fd
Prefer build-system requires to setup_requires in rust_with_cffi
abravalheri Aug 14, 2023
634efa8
Avoid using unzip in test-crossenv
abravalheri Aug 14, 2023
745a781
Add Cross.toml to namespace_package/MANIFEST.in
abravalheri Aug 15, 2023
1d3d921
Disable build isolation for cibuildwheel
abravalheri Aug 15, 2023
085013b
Increase cibuildwheel verbosity for debugging
abravalheri Aug 15, 2023
023dc7b
Do a 2 stage installation for cibuildwheel
abravalheri Aug 15, 2023
81ec0b0
Add workaround for auditwheel
abravalheri Aug 15, 2023
e9b85eb
Reuse logic to get bdist_wheel command object
abravalheri Aug 15, 2023
849cfb1
Add CHANGELOG entry
abravalheri Aug 17, 2023
a808242
Fix linting problem
abravalheri Aug 18, 2023
85df414
Address type checker not dealing with try..except block
abravalheri Aug 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 39 additions & 17 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ jobs:
PYO3_CROSS_LIB_DIR: /Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/lib
run: |
cd examples/namespace_package
pip install wheel
python setup.py bdist_wheel
pip install build wheel
python -m build --no-isolation
ls -l dist/
pip install --force-reinstall dist/namespace_package*_universal2.whl
cd -
Expand Down Expand Up @@ -159,16 +159,18 @@ jobs:

- name: Build an abi3 wheel
shell: bash
env:
# https://github.com/actions/setup-python/issues/26
MACOSX_DEPLOYMENT_TARGET: 10.9
DIST_EXTRA_CONFIG: /tmp/build-opts.cfg
run: |
set -e
cd examples/rust_with_cffi/
python --version
pip install -U wheel
python setup.py bdist_wheel --py-limited-api=cp39
pip install -U build cffi wheel
echo -e "[bdist_wheel]\npy_limited_api=cp39" > $DIST_EXTRA_CONFIG
python -m build --no-isolation
ls -la dist/
env:
# https://github.com/actions/setup-python/issues/26
MACOSX_DEPLOYMENT_TARGET: 10.9

# Now we switch to a differnet Python version and ensure we can install
# the wheel we just built.
Expand Down Expand Up @@ -222,12 +224,20 @@ jobs:
python3.9 -m pip install crossenv
python3.9 -m crossenv "/opt/python/cp39-cp39/bin/python3" --cc $TARGET_CC --cxx $TARGET_CXX --sysroot $TARGET_SYSROOT --env LIBRARY_PATH= --manylinux manylinux1 venv
. venv/bin/activate
build-pip install cffi wheel "setuptools>=62.4"

build-pip install -U pip>=23.2.1 setuptools>=68.0.0 wheel>=0.41.1
cross-pip install -U pip>=23.2.1 setuptools>=68.0.0 wheel>=0.41.1
build-pip install cffi
cross-expose cffi
pip install wheel
pip install -e ../../
python setup.py bdist_wheel --py-limited-api=cp37
cross-pip install -e ../../
cross-pip list

export DIST_EXTRA_CONFIG=/tmp/build-opts.cfg
echo -e "[bdist_wheel]\npy_limited_api=cp37" > $DIST_EXTRA_CONFIG

cross-pip wheel --no-build-isolation --no-deps --wheel-dir dist .
ls -la dist/
python -m zipfile -l dist/*.whl # debug all files inside wheel file
' > build-wheels.sh

docker run --rm -v "$PWD":/io -w /io messense/manylinux2014-cross:${{ matrix.platform.arch }} bash build-wheels.sh
Expand Down Expand Up @@ -269,12 +279,15 @@ jobs:
CARGO: cross
CARGO_BUILD_TARGET: aarch64-unknown-linux-gnu
PYO3_CROSS_LIB_DIR: /opt/python/cp38-cp38/lib
DIST_EXTRA_CONFIG: /tmp/build-opts.cfg
run: |
cd examples/namespace_package
docker build -t cross-pyo3:aarch64-unknown-linux-gnu .
python -m pip install wheel
python setup.py bdist_wheel --plat-name manylinux2014_aarch64
python -m pip install build wheel
echo -e "[bdist_wheel]\nplat_name=manylinux2014_aarch64" > $DIST_EXTRA_CONFIG
python -m build --no-isolation
Comment on lines +286 to +288
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the changes for test-cross and test-crossenv can be reverted and left for now to ease your pain in CI? Given rust_with_cffi/setup.py still exists I'd be ok with leaving those as-is in this PR and dealing with them another time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @davidhewitt, thank you very much for the review.

I can try to do that. For test-crossenv it should be just a matter of reverting 1d3fe5f. Although, I am not sure how the problem with the failures in the CI are related to setup.py vs. DIST_EXTRA_CONFIG as a form of passing build options:

Maybe that is related to the use of a virtual environment in https://github.com/PyO3/setuptools-rust/actions/runs/5834731386/job/15824908490#step:7:6?

I might try a couple of things before, but if they don't work I will proceed with the suggestion of sticking with python setup.py for the problematic tests.

Copy link
Contributor Author

@abravalheri abravalheri Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There is something weird going on with test-crossenv when I try to use build:

+ pip install cffi
Requirement already satisfied: cffi in ./venv/build/lib/python3.9/site-packages (1.15.1)
Collecting pycparser
  Using cached pycparser-2.21-py2.py3-none-any.whl (118 kB)
Installing collected packages: pycparser
Successfully installed pycparser-2.21

Notice:  A new release of pip is available: 23.0.1 -> 23.2.1
Notice:  To update, run: pip install --upgrade pip
+ export DIST_EXTRA_CONFIG=/tmp/build-opts.cfg
+ DIST_EXTRA_CONFIG=/tmp/build-opts.cfg
+ echo -e '[bdist_wheel]\npy_limited_api=cp37'
+ python -m build --no-isolation
* Getting build dependencies for sdist...

ERROR Missing dependencies:
	cffi

I do pip install cffi and it says Requirement already satisfied, then I try to run python -m build --no-isolation and it complains about cffi being a missing dependency.

I am not sure how the cross-expose tools work, but maybe there is a clash with the way build provisions "no-isolation" build enviroments...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

crossenv seems to have a problem with build: benfogle/crossenv#108

ls -la dist/
unzip -l dist/*.whl # debug all files inside wheel file
- uses: uraimo/run-on-arch-action@v2.5.0
name: Install built wheel
with:
Expand Down Expand Up @@ -313,13 +326,16 @@ jobs:
CARGO: cargo-zigbuild
CARGO_BUILD_TARGET: aarch64-unknown-linux-gnu
PYO3_CROSS_LIB_DIR: /opt/python/cp38-cp38/lib
DIST_EXTRA_CONFIG: /tmp/build-opts.cfg
run: |
mkdir -p $PYO3_CROSS_LIB_DIR
docker cp -L $(docker create --rm quay.io/pypa/manylinux2014_aarch64:latest):/opt/python/cp38-cp38 /opt/python
cd examples/namespace_package
python -m pip install wheel
python setup.py bdist_wheel --plat-name manylinux2014_aarch64
python -m pip install build wheel
echo -e "[bdist_wheel]\nplat_name=manylinux2014_aarch64" > $DIST_EXTRA_CONFIG
python -m build --no-isolation
Comment on lines 333 to +336
Copy link
Member

Choose a reason for hiding this comment

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

Potentially you could also rework this test to use the rust_with_cffi example and keep using setup.py for now?

Copy link
Contributor Author

@abravalheri abravalheri Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@davidhewitt, before going back to setup.py, I am comparing the outputs of the logs between the CI in this PR and the main branch.

In the CI logs, we can see that for this PR, setuptools-rust prints

Copying rust artifact from target/aarch64-unknown-linux-gnu/release/libnamespace_package_rust.so to build/lib.linux-x86_64-cpython-38/namespace_package/rust.cpython-38-x86_64-linux-gnu.so

For the main branch the message is different though:

Copying rust artifact from target/aarch64-unknown-linux-gnu/release/libnamespace_package_rust.so to build/lib.linux-x86_64-cpython-38/namespace_package/rust.so

They differ in the target file name rust.cpython-38-x86_64-linux-gnu.so (PR) vs. rust.so (main). This is what makes the import fail when running the checks (the file extension .cpython-38-x86_64-linux-gnu.so is not recognised by the aarch64 version of python, but .so is).

In both cases however the name of the wheel is identical: namespace_package-0.1.0-cp38-cp38-manylinux2014_aarch64.whl (CI for PR, CI for main), which seems to indicate that setting plat_name via DIST_EXTRA_CONFIG works (pypa/wheel's bdist_wheel is picking up the correct parameter).

Do you have any idea why that is happening?

I can see in setuptools_rust/build.py that setuptools-rust seem to be getting the file name from the get_dylib_ext_path method, which seems to re-use build_ext.get_ext_fullpath (+ a bunch of conditional logic to detect if the host and target architectures are the same), build_rust itself also inherits plat_name from the build command (which then is used in the logic comparing target and host architecture).

The problem that I seem to be seen is that both build and build_ext are not aware about the user defined plat_name... bdist_wheel do not seem to take any special provisions to set build and build_ext's plat_name... Instead, it just copies it from bdist when not given, and in turn, bdist will copy it from build.

I wonder if instead of passing this parameter to bdist_wheel, we should instead be passing it to build... I will probably test that theory in a new commit. UPDATE: distutils prevents passing plat_name to build in platforms that are not Windows.

UPDATE: I see now that setuptools-rust gets plat_name from bdist_wheel. However, it uses get_cmdline_options(), which may not be set if we don't use sys.argv for passing arguments (?). Probably that needs to be changed before moving away from direct calls to setup.py.

Copy link
Contributor Author

@abravalheri abravalheri Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that changing the following fixes the problem with test-zigbuild:

--- a/setuptools_rust/setuptools_ext.py
+++ b/setuptools_rust/setuptools_ext.py
@@ -163,9 +163,11 @@ def add_rust_extension(dist: Distribution) -> None:
-                options = self.distribution.get_cmdline_options().get("bdist_wheel", {})
-                plat_name = options.get("plat-name") or self.plat_name
+                bdist_wheel = self.distribution.get_command_obj("bdist_wheel")
+                plat_name = bdist_wheel.plat_name or self.plat_name

(Probably related to the fact that when the test calls python -m build, the plat_name is given via a DIST_EXTRA_CONFIG file instead of sys.argv, but I might be wrong).

ls -la dist/
unzip -l dist/*.whl # debug all files inside wheel file
- uses: uraimo/run-on-arch-action@v2.5.0
name: Install built wheel
with:
Expand All @@ -346,9 +362,15 @@ jobs:
- uses: pypa/cibuildwheel@v2.3.1
env:
CIBW_BUILD: cp39-*
CIBW_BEFORE_BUILD: pip install -e .
CIBW_BEFORE_BUILD: pip install -U 'pip>=23.2.1' 'setuptools>=68.0.0' 'wheel<=0.37.1' && pip install -e . && pip list
# ^-- cap on `wheel` is a workaround for pypa/auditwheel#436
# setuptools needs to be upgraded before installing setuptools-rust
CIBW_ARCHS_MACOS: "x86_64 universal2 arm64"
CIBW_BUILD_VERBOSITY: 1
CIBW_BUILD_VERBOSITY: 3
CIBW_BUILD_FRONTEND: pip # pip allows disabling isolation via env var
CIBW_ENVIRONMENT: PIP_NO_BUILD_ISOLATION=false
# ^-- necessary to use working copy of setuptools-rust,
# (however PIP_NO_BUILD_ISOLATION is counter-intuitive: see pypa/pip#5735)
with:
package-dir: examples/namespace_package

Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
### Added
- Add support for `pyproject.toml` configuration using `[tool.setuptools-rust]` options. [#348](https://github.com/PyO3/setuptools-rust/pull/348)

### Fixed
- Fix `plat_name` handling in the case `bdist_wheel.plat_name` is set via configuration file (e.g., `setup.cfg`). [#352](https://github.com/PyO3/setuptools-rust/pull/352)

## 1.6.0 (2023-04-27)
### Changed
- Prefer passing `--crate-type` option to cargo if "toolchain >= 1.64". [#322](https://github.com/PyO3/setuptools-rust/pull/322)
Expand Down
274 changes: 0 additions & 274 deletions examples/hello-world-pyprojecttoml/Cargo.lock
View file
Open in desktop

This file was deleted.

Loading