search

GrahamDumpleton / wrapt

A Python module for decorators, wrappers and monkey patching.
BSD 2-Clause "Simplified" License
2.04k stars 230 forks source link

1.13.0rc2 feedback: Build from sdist on Windows fails #186

Closed vphilippon closed 2 years ago

vphilippon commented 3 years ago

Hello!

I have a system running on Python 2.7-32bit on Windows. Since the wheels for that kind of environment aren't published, my system ends up pulling the sdist (.tar.gz) and attempts to build the wheel (as it used to do with versions 1.12.1 and earlier).

But, in the 1.13.0rc2 .tar.gz, in src\wrapt.egg-info\SOURCES.txt, there's this linux-specific entry (which is also an absolute path): /home/runner/work/wrapt/wrapt/src/wrapt/_wrappers.c I'm fairly sure linux-style absolute path shouldn't be there since this is the source for any system to build :) .

I'm not sure what went wrong in the build step for it to go there. I know some sdists simply don't include the .egg-info in the sdist, and I've also seen some sdists that do. I feel like I knew the differences and conditions at some point, but I don't recall anymore, sorry!

I hope this will be of use to you. If you have questions or things you think I can do to help, let me know! Cheers!

GrahamDumpleton commented 3 years ago

Example from SOURCES.txt.

LICENSE
MANIFEST.in
README.rst
setup.cfg
setup.py
/home/runner/work/wrapt/wrapt/src/wrapt/_wrappers.c
src/wrapt/__init__.py
src/wrapt/_wrappers.c
src/wrapt/decorators.py
src/wrapt/importer.py
src/wrapt/wrappers.py
src/wrapt.egg-info/PKG-INFO
src/wrapt.egg-info/SOURCES.txt
src/wrapt.egg-info/dependency_links.txt
src/wrapt.egg-info/not-zip-safe
src/wrapt.egg-info/top_level.txt

@althonos do you have any ideas? Did I break something when I reorganised workflows or changed configs after merge of your PR?

GrahamDumpleton commented 3 years ago

Maybe MANIFEST.in should be more specific somehow. Currently it has:

recursive-include src *.c
vphilippon commented 3 years ago

I think I might have found the issue. From https://packaging.python.org/guides/using-manifest-in/#how-files-are-included-in-an-sdist

all C source files mentioned in the ext_modules or libraries setup() arguments

And here https://github.com/GrahamDumpleton/wrapt/blob/7cb212b7021ec026fcbc928491819166ed18eecc/setup.py#L25 notice the os.path.realpath. This might be the cause of the absolute path entry.

Previously, it was: https://github.com/GrahamDumpleton/wrapt/blob/0baff1b6ac8d64ffb08ea2d1610c7ed90115b9d5/setup.py#L69

Edit: Oh, was the intention to use os.path.relpath instead?

GrahamDumpleton commented 3 years ago

Even when I restore it back to what it was previously an absolute path is still included which is most odd. So seems something else is coming into play as well.

GrahamDumpleton commented 3 years ago

Previous versions of wrapt don't even include that egg info in sdist tar ball. So is likely something to do with the setup.cfg file that was added.

GrahamDumpleton commented 3 years ago

Restoring it back to what it was didn't work for me on my own local macOS machine with either Python 2.7 or 3.9, but when done under GitHub actions which is Linux it seems to now be correct. Need to double check.

althonos commented 3 years ago

Out of curiosity, what is your setuptools version? Given that _wrappers.c appears twice, I'd imagine it's added once because of the MANIFEST.in file, and once because setuptools is pulling it from the extensions in setup.py.

GrahamDumpleton commented 3 years ago

On my local macOS box was probably using pip 21.2.2 at the time.

Anyway, bit delayed as got side tracked on real work as usual, but wrapt 1.13.0rc3 on PyPi should hopefully fix it.

vphilippon commented 3 years ago

Just to let you know that I've tested 1.13.0rc3, and yup, the sdist seems cleaner, and builds on Windows now on my end.

Thanks!