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

move generation of pari bindings sources out of build_ext #163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kiwifb
Copy link
Member

@kiwifb kiwifb commented Aug 15, 2024

I have been sitting on this one for a while as I was seeing it as a corner case that only shows up in a rare workflows. But I know think it is evidence of something done incorrectly that could affect everyone if setuptools tighten up the install process.

So, what is this supposed to fix:
In the normal production of binary wheel, you would get the sdist, build the wheel for a version of python and start from scratch for every version of python.

In Gentoo where we allow for multiple python on the system, you unfold the sdist and since building python package is essentially an out of source process, you build for all your python implementations from the same source directory (usually).

In this setup the current setup.py will generate the .pxd files for the first python implementation being built and they will be shipped for that python implementation. Further python implementations will not generate a new version of .pxd files and will use the ones generated by the first implementation - but they will not get shipped (even with package_data saying that they should be). This is essentially because the .pxd files are generated in the source tree.

This PR makes the .pxd files to be generated in the python implementation build tree instead of the source tree. This ensure that they are generated with the right version of python and always shipped.

Given the observed behaviour, I have concerns that a future version of setuptools could just stop shipping .pxd files generated in the source directory as part of the build process.

# Generate auto-generated sources from pari.desc
# This needs to be done before build/build_ext so the generated pxd is moved
# to the build directory and installed with newer setuptools.
rebuild()
Copy link

Choose a reason for hiding this comment

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

This will make this run even on just building the sdist, which does not seem correct

Copy link
Member Author

Choose a reason for hiding this comment

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

I had not thought of that because I did not produce sdist off course. It may need to be in build.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just did a basic test by adding a build class and running rebuild there. The .pxd are only generated once but are now properly shipped with each python implementation. I would prefer if .pxd where always re-generated but that may be considered an efficiency win. I will check what happens to sdist.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that still generate and incorporate .pxd in sdist. I may have to use MANIFEST.in instead to exclude them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I keep forgetting. This is not about all .pxd files, it is about just auto_paridecl.pxd. The sdist is fine putting rebuild in a build class.

Copy link
Member Author

Choose a reason for hiding this comment

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

And auto_paridecl.pxd is also not included in sdist with my current PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants