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

Drop support for py3.6; introduce test_distutils, refactor hpy.devel, fix setuptools integration #338

Merged
merged 27 commits into from
Aug 3, 2022

Conversation

antocuni
Copy link
Collaborator

@antocuni antocuni commented Aug 2, 2022

I wanted to fix #322, but then I realized that it was more involved than expected, because depending on which version of setuptools you use, you might end up monkey-patching either the stdlib distutils or the setuptools vendored distutils.

To better understand the various problems and to aid the refactoring, I introduced test_distutils.py: some of the logic was already tested by test_pof.sh, but eventually we should migrate all the tests to test_distutils.py, IMHO.

…y test, create one at the beginning and clone it muliple times. This is much faster
… universal ABI module. Also, introduce a @monkeypatch decorator which is a much nicer way to monkey-patch bdist_egg.write_stub
All the logic was to replace has_ext_modules with our own version, we our
version is the very same code as the distutils' one.  I'm not even sure
whether it was ever necessary, because distutils does not seem to have changed
since py3.7 at least.

So, I think that this is safe to remove, but I hope it's not a Chesterton's
fence.
@antocuni
Copy link
Collaborator Author

antocuni commented Aug 2, 2022

@hodgestar btw, maybe you have an opinion about be42806?
From git history I seem to understand what you were the original author of the code which I removed, do you remember why it was necessary?

@hodgestar
Copy link
Contributor

@hodgestar btw, maybe you have an opinion about be42806? From git history I seem to understand what you were the original author of the code which I removed, do you remember why it was necessary?

For each subcommand (e.g. build_ext) distutils maintains a list of (name, func) where func returns True if the subcommand needs to run. Because HPy extensions live in their own list, the existing func for build_ext would return False (it just checks if ext_modules is empty or not).

It's possible that this was only needed for the old distutils from outside setuptools? This stuff is so twisty it's hard to remember anything for more than a few seconds.

@antocuni antocuni changed the title WIP: introduce test_distutils, refactor hpy.devel introduce test_distutils, refactor hpy.devel Aug 2, 2022
@antocuni antocuni marked this pull request as ready for review August 2, 2022 14:02
@antocuni
Copy link
Collaborator Author

antocuni commented Aug 2, 2022

For each subcommand (e.g. build_ext) distutils maintains a list of (name, func) where func returns True if the subcommand needs to run. Because HPy extensions live in their own list, the existing func for build_ext would return False (it just checks if ext_modules is empty or not).

yes, I understood this part of the logic. But my point is that we were substituding build.has_ext_module with a new function which looked exactly the same; this was ours:

      def build_has_ext_modules(self):
            return self.distribution.has_ext_modules()

and this is the one in CPython's distutils: in 3.6.0 was already identical:
https://github.com/python/cpython/blob/v3.6.0/Lib/distutils/command/build.py#L146-L147

The default implementation works out of the box with hpy because it delegates to self.distribution, which we do monkey-patch.

It's possible that this was only needed for the old distutils from outside setuptools? This stuff is so twisty it's hard to remember anything for more than a few seconds.

Could be, but according to git blame, it seems to have been like this at least since Sep 30, 2000 😅
python/cpython@64d855a

Maybe there was a random version of setuptools for which this change was necessary?

Anyway, I wondered whether I was missing anything obvious and/or non-obvious-but-that-you-could-remember, but apart from that I'm pretty sure that the current logic works, because all the tests (old and new) are passing :).

Also, the branch is ready to be merged IMHO.
The only slightly controversial point is that I had to drop support for py3.6, because newer versions of setuptools (which we rely on now) don't work with that.
But I guess it's fine to drop, since numpy and scipy support only >=3.8.

@antocuni antocuni changed the title introduce test_distutils, refactor hpy.devel Drop support for py3.6; introduce test_distutils, refactor hpy.devel, fix setuptools integration Aug 2, 2022
Copy link
Contributor

@fangerer fangerer left a comment

Choose a reason for hiding this comment

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

Thanks, @antocuni . Good to see a PR from you again 🙂

.github/workflows/ci.yml Show resolved Hide resolved
if resource.endswith(".hpy.so"):
log.info("stub file already created for %s", resource)
return
write_stub.super(resource, pyfile)
Copy link
Contributor

Choose a reason for hiding this comment

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

When building NumPy/HPy and doing rm -rf build before python3 setup.py --hpy-abi=universal build_ext -if (or when building the first time), then it fails with:

error: [Errno 2] No such file or directory: 'build/lib.macosx-12.3-x86_64-3.8-pydebug/numpy/core/_multiarray_umath.py'

This is because the parent dir build/lib.macosx-12.3-x86_64-3.8-pydebug/numpy/core does not yet exist.
Could we maybe create the parent dir of the stub here (if it does not exist)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't understand. This is a new bug, or an existing one?

@fangerer
Copy link
Contributor

fangerer commented Aug 3, 2022

The only slightly controversial point is that I had to drop support for py3.6, because newer versions of setuptools (which we rely on now) don't work with that.

Not controversial. 3.6 is EOL.

…ng on here. Also, put the string '3.7' so that it's easier to grep when we will want to drop support for it
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.

setup.py based installation can mix universal and CPython ABI
3 participants