Change examples in accordance to review comments in #348

[abravalheri]

This PR attempts to target a few review comments in #348, specifically:

---

Request to change examples from setup.py to pyproject.toml:

#348 (review)

Given that setup.py configuration is sort of deprecated, how do you feel about going further and migrating all examples (except perhaps for a hello-world-setuppy to test the setup.py form)?

I tried to implement this from commits c0034a9 to e00ab30.

However there was a bunch of stuff that was difficult to understand because I don't have experience with Rust or setuptools-rust, so while I was implementing the change I also added a bunch of comments kind of explaining for myself what was going on. I am not 100% sure if the comments are correct, or if maintainers agree with the comments, or if the maintainers think the comments are desirable in the examples. So please let me know and that can be changed.

A few other remarks:

• I renamed hello-world-pyprojecttoml to hello-world and hello-world to hello-world-setuppy.

• I removed direct calls to python setup.py install from the test suite, since these are deprecated. If preferred I can add something calling python -m build.

• I did not migrate the CFFI example to pyproject.toml because it uses a special cffi_modules keyword which is implemented by the cffi dependency itself and is not native to setuptools, with no pyproject.toml-equivalent. It is possible to migrate everything else to pyproject.toml and just leave setup(cffi_modules=["cffi_module.py:ffi"]) behind, but I was not sure the maintainers would like the split. Please let me know if that should be changed.

• I assumed that the configuration in the hello-world-script example that uses a RustExtension with Binding.Exec is equivalent to a RustBin and turned the following

  RustExtension(
      {"hello-world-script": "hello_world.hello-world-script"},
      binding=Binding.Exec,
      script=True,
      args=["--profile", "release-lto"],
  )

  into the TOML-equivalent:

  [[tool.setuptools-rust.bins]]
  target = "hello-world-script"
  args = ["--profile", "release-lto"]

---

Request to change the examples to use the same directory:

#348 (comment):

Personally I would prefer if we left the rust directory as src, because that's what the rest of the examples do. I think there's no good reason to diverge here, but I'm also happy to be persuaded that rust is better. If rust is agreed to be better, we should update the rest of the examples to use it too.

...
#348 (comment)

Fair enough, let's keep rust directory here 👍

Not sure if there is an agreement about which directory is better, but it seems that the maintainer understand the controversy with the src directory and appreciated the effort to side-step this controversy¹.

I intentionally left the commits implementing this change at the end of the PR so they can be reverted (or git rest --hard), if this change is not desirable.

---

Please let me know if that is not what you had in mind and we can iterate over the implementation.

P.S.: I will work on changing the docs in a separated PR.

Footnotes

1. A "Python-first" developer would be confused with Rust files in src since the directory is historically used in Python packaging for Python files. A "Rust-first" developer would be confused with Python files in the src directory since Cargo also uses it as default. So the approach explored here follows the "explicit is better than implicit" mantra. ↩

[davidhewitt]

Thanks! I am happy with all the changes proposed here. You will probably need to change references in .github/workflows/ci.yml to use python -m build invocations instead of setup.py, as you noted elsewhere.

[davidhewitt]

CI looking painful 😬

[davidhewitt]

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.

[abravalheri]

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:

• In https://github.com/PyO3/setuptools-rust/actions/runs/5834731386/job/15824909577#step:3:232 we can see that the error in CI for test-crossenv seems unrelated (somehow it is missing decimal, which is supposedly happening before build creates an isolated environment.)

• In https://github.com/PyO3/setuptools-rust/actions/runs/5834731386/job/15824908490#step:7:488), we can see that Rust seems to be having problems to find Python information and failing with:

  error: no Python 3.x interpreter found

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.

[abravalheri]

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...

[abravalheri]

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

[davidhewitt]

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

[abravalheri]

@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.

[abravalheri]

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).

[davidhewitt]

The cibuildwheel failure looks like it's because it's now building with pyproject.toml / PEP517 build isolation, which is causing the released setuptools-rust to be installed rather than the one from this branch.

Probably the most practical solution is to overwrite the PEP517 build requirements as a setup step in the cibuildwheel job, I think the below sed might work:

sed -i 's/requires = ["setuptools", "wheel", "setuptools-rust"]/requires = ["setuptools", "wheel", "${{ github.workspace }}"]/g" examples/namespace_package/pyproject.toml

---

I think the cross failure is caused because python -m build is building an sdist first, and the wheel from that, but the Cross.toml file is not included in the sdist. I guess solutions are either to include that file in the sdist, or to just do python -m build --wheel and skip the sdist.

[davidhewitt]

You may need to call cmd.ensure_finalized(), see _py_limited_api() in build.py. (Maybe these two methods can share logic?)

[abravalheri]

For this specific scenario skipping ensure_finalized should be fine because build_rust already implements the same fallback for plat_name than bdist_wheel does (i.e. copying from build), so we are just interested in the value given via sys.argv/config file, which is available right after setuptools instantiates the command object inside get_command_obj.

But I agree that it would be nicer to share the logic between these 2 objects, so I will go ahead and implement it once I know what to do with the problem in auditwheel.

[abravalheri]

Thank you very much @davidhewitt for the pointers!

The cibuildwheel failure looks like it's because it's now building with pyproject.toml / PEP517 build isolation, which is causing the released setuptools-rust to be installed rather than the one from this branch.

I think I managed to get cibuildwheel to work without build isolation (very tricky...¹), but now auditwheel is failing due to pypa/auditwheel#436: https://github.com/PyO3/setuptools-rust/actions/runs/5871529409/job/15921095642?pr=352#step:4:390

I think the cross failure is caused because python -m build is building an sdist first, and the wheel from that, but the Cross.toml file is not included in the sdist. I guess solutions are either to include that file in the sdist, or to just do python -m build --wheel and skip the sdist.

I went with adding it to MANIFEST.in.

Footnotes

1. When I use build isolation I have to upgrade setuptools first and only then install the working version of setuptools-rust. ↩

[davidhewitt]

I think we might be about to get a green ci... 👀

(This has been a truly heroic effort, thank you for all your help!)

[davidhewitt]

This is really great, thank you so much for this.

I think there's just the CHANGELOG entry to add for fixed plat name when it comes from DIST_EXTRA_CONFIG and also the refactoring of that code to be shared with _py_limited_api?

Once that's done, let's ship this in 1.7.0 ⛵

[abravalheri]

Hi @davidhewitt thank you very much for the help.

I implemented the changes, but there seems to be something off with crates.io today breaking the CI:

  Caused by:
    failed to query replaced source registry `crates-io`

  Caused by:
    download of 3/s/syn failed

  Caused by:
    failed to download from `[https://index.crates.io/3/s/syn`](https://index.crates.io/3/s/syn%60)

Other than that, my (hopefully) final remarks on this PR are:

• I started trying to only tackle the examples structure/config, but unfortunately it evolved to be much more than that and ended up being a PR to make the CI build-isolation-ready. Sorry for mixing the concerns :P
• I left on the CI config some statements that clearly target debugging (e.g. increasing verbosity, pip list or unzip -l dist/*.whl). I found that these statements helped me to identify the root cause of the errors, and that in the future they might come handy again.
  Because of that I decided to leave them, for now, in the PR. But I am happy to remove them, please just let me know.

[davidhewitt]

This is brilliant as-is. Thank you very much for all the effort invested here; the CI and examples are in much better shape for all of this. I see you've also opened an issue with cibuildwheel to suggest further improvements upstream ❤️

I'll merge now and rebase the 1.7.0 release with a view to putting live early next week :)