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

Binary distributions such as wheels include C sources #6399

Closed
1 task done
musicinmybrain opened this issue Dec 8, 2021 · 5 comments · Fixed by #6400
Closed
1 task done

Binary distributions such as wheels include C sources #6399

musicinmybrain opened this issue Dec 8, 2021 · 5 comments · Fixed by #6400
Labels

Comments

@musicinmybrain
Copy link
Contributor

Describe the bug

Binary distributions such as wheels include C sources (_find_header.c, _find_header.h, _helpers.c, _http_parser.c, _http_writer.c, and _websocket.c), but these sources don’t provide value in binary distributions and their size is significant.

See also aio-libs/frozenlist#250.

To Reproduce

$ python3 -m venv _e
$ . _e/bin/activate
(_e) $ pip install aiohttp
(_e) $ ls -l _e/lib64/python*/site-packages/aiohttp/*.{c,h,pyx,px*}

Expected behavior

The C sources should ideally be present in the sdist but not in the bdist, where they don’t really provide any value.

Logs/tracebacks

$ python3 -m venv _e
$ . _e/bin/activate
(_e) $ pip install aiohttp
(_e) $ ls -l _e/lib64/python*/site-packages/aiohttp/*.{c,h,pyx,px*}
-rw-rw-r--. 1 ben ben   4998 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_cparser.pxd
-rw-rw-r--. 1 ben ben 187570 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_find_header.c
-rw-rw-r--. 1 ben ben    170 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_find_header.h
-rw-rw-r--. 1 ben ben     68 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_find_header.pxd
-rw-rw-r--. 1 ben ben   2007 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_headers.pxi
-rw-rw-r--. 1 ben ben 212982 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_helpers.c
-rw-rw-r--. 1 ben ben   1049 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_helpers.pyx
-rw-rw-r--. 1 ben ben 978187 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_http_parser.c
-rw-rw-r--. 1 ben ben  26571 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_http_parser.pyx
-rw-rw-r--. 1 ben ben 221270 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_http_writer.c
-rw-rw-r--. 1 ben ben   4575 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_http_writer.pyx
-rw-rw-r--. 1 ben ben 138420 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_websocket.c
-rw-rw-r--. 1 ben ben   1561 Dec  7 23:42 _e/lib64/python3.10/site-packages/aiohttp/_websocket.pyx

Python Version

$ python --version
Python 3.10.0

aiohttp Version

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.8.1
[…]

multidict Version

$ python -m pip show multidict
Name: multidict
Version: 5.2.0
[…]

yarl Version

$ python -m pip show yarl
Name: yarl
Version: 1.7.2
[…]

OS

Fedora Linux 35—but this issue is platform-independent

Related component

Server

Additional context

A suggested fix is to add to setup(…) in setup.py:

    exclude_package_data={"": ["*.c", "*.h"]},

which will exclude these sources from binary distributions without affecting source distributions.

It’s possible to exclude some of the Cython files as well, but in frozenlist the preference was to keep them in the bdist for debugging purposes.

PR to follow.

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
musicinmybrain added a commit to musicinmybrain/aiohttp that referenced this issue Dec 8, 2021
This does not affect source distributions, and Cython sources (.pyx) are
still installed.

Fixes aio-libs#6399.
@webknjaz
Copy link
Member

webknjaz commented Dec 8, 2021

The C sources should ideally be present in the sdist but not in the bdist, where they don’t really provide any value.

At the same time, you mentioned the debug purposes. You said that only PYX files are necessary to debug but in fact, Cython adds mapping marker comments in the generated C-files that can later be used by coveragepy. I would imagine that the same markers could be used by the tools that provide debugging functionality. Besides, for debugging, it's actually important to see the C sources.
So keeping the PYX files but removing their C/H counterparts feels like a half-measure and we probably should either remove all or keep all.

@asvetlov
Copy link
Member

asvetlov commented Dec 8, 2021

@webknjaz I was the person who suggested keeping PYX files for frozendict.
We ship compiled C Extensions (.so and .pyd files) with high optimization level and without embedded debug info.
aiohttp does nothing specific actually, it is the default configuration.
The default is reasonable, as debugging usually requires optimizations disabled (-O0 mode). As the result, everything is executed as slow as possible :)
So, C source file doesn't help well with optimized binary code. Also, Python users almost never debug C Extensions with C debugger (gdb, lldb, etc.) I personally did it only when developed C Extensions sometimes and never when used external C Extensions in my projects.

Cython is different.
When Cython raises an exception, it provides a traceback collected by __Pyx_AddTraceback calls.
The traceback contains dummy python frames that point on lines in the corresponding PYX file.
Thus, embedded PYX helps with error debugging/handling.
This mode is enabled by default (honestly I never wanted to learn how to disable it) and pretty fast.
It is useful when you operate with Python debugging tools and not related to C debugger, e.g. gdb.

@musicinmybrain
Copy link
Contributor Author

We ship compiled C Extensions (.so and .pyd files) with high optimization level and without embedded debug info.

The debug info actually is present:

$ python3 -m venv _e
$ . _e/bin/activate
(_e) $ pip install frozenlist
(_e) $ ls -lh _e/lib64/python3.10/site-packages/frozenlist/*.so
-rwxrwxr-x. 1 ben ben 489K Dec  8 09:22 _e/lib64/python3.10/site-packages/frozenlist/_frozenlist.cpython-310-aarch64-linux-gnu.so
(_e) $ file _e/lib64/python3.10/site-packages/frozenlist/*.so
_e/lib64/python3.10/site-packages/frozenlist/_frozenlist.cpython-310-aarch64-linux-gnu.so: ELF 64-bit LSB shared object, ARM aarch64, version 1 (SYSV), dynamically linked, BuildID[sha1]=639c62bc066bc2c3f7064fff46d8427f837155a3, with debug_info, not stripped
(_e) $ strip _e/lib64/python3.10/site-packages/frozenlist/*.so
(_e) $ ls -lh _e/lib64/python3.10/site-packages/frozenlist/*.so
-rwxrwxr-x. 1 ben ben 71K Dec  8 09:25 _e/lib64/python3.10/site-packages/frozenlist/_frozenlist.cpython-310-aarch64-linux-gnu.so

musicinmybrain added a commit to musicinmybrain/aiohttp that referenced this issue Dec 8, 2021
This does not affect source distributions, and Cython sources (.pyx) are
still installed.

Fixes aio-libs#6399.
@asvetlov
Copy link
Member

asvetlov commented Dec 8, 2021

@musicinmybrain you are right, debug sections are present in so files.
I doubt if we should strip them by default.

@musicinmybrain
Copy link
Contributor Author

@musicinmybrain you are right, debug sections are present in so files. I doubt if we should strip them by default.

I agree—in my opinion, debug sections are valuable for end-users and shouldn’t be stripped by default.

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

Successfully merging a pull request may close this issue.

3 participants