miguelgrinberg / simple-websocket

Simple WebSocket server and client for Python.
MIT License
78 stars 17 forks source link

1.0.0: `__init__.py` and `helpers.py` are missing from `tests/` #31

Closed mgorny closed 11 months ago

mgorny commented 11 months ago
$ tar -tf /tmp/dist/simple-websocket-1.0.0.tar.gz  | grep tests/
simple-websocket-1.0.0/tests/
simple-websocket-1.0.0/tests/test_aioclient.py
simple-websocket-1.0.0/tests/test_aioserver.py
simple-websocket-1.0.0/tests/test_client.py
simple-websocket-1.0.0/tests/test_server.py

However, the __init__.py and helpers.py files needed by the tests are missing from the sdist archive.

miguelgrinberg commented 11 months ago

I do not intend to ship unit tests in source packages. I'm not sure why some of the test files got included, but the correct fix here would be to remove those extra files.

mgorny commented 11 months ago

Why would you do that? Distributions and end users need these files to test the code before upgrading.

miguelgrinberg commented 11 months ago

Distributions and end users who care can download the tests from this repository. The files uploaded to PyPI are for installing, not for testing.

mgorny commented 11 months ago

simple-websocket is a pure Python package, so "installing" always uses wheels instead. Source distributions are then only used by people who explicitly need more than that, so removing files from does not help anyone and harms the people who actually need them.

miguelgrinberg commented 11 months ago

Incorrect. There are platforms that install the source tarball. The Alpine Docker images, for example.

mgorny commented 11 months ago

Do "Alpine Docker image" users mind the archive being 5 KiB larger?

miguelgrinberg commented 11 months ago

There is really no point in having this discussion, I hope you realize that.

If one day I change my mind to include tests, then it would be tests, docs, examples, the whole thing (which is what you get if you download the source release here). And it will be done for all my open source packages, not just the one you happen to have an interest on. My current position is to not do it and that's the end of the story, whether you agree or not.

eli-schwartz commented 11 months ago

Incorrect. There are platforms that install the source tarball. The Alpine Docker images, for example.

Can you please clarify how this works?

Alpine docker containers very famously cannot install wheels from PyPI because Alpine docker containers have a technical impediment: they do not use glibc, but rather musl libc, which means that installing manylinux wheels on Alpine is approximately as useful as installing Windows wheels on macOS.

But Alpine docker images do, in fact, install wheels that bear the wheel tag:

simple_websocket-1.0.0-py3-none-any.whl 

because of the "py3-none-any" part. These are not Windows wheels, these are not macOS wheels, these are not manylinux wheels, and these are not musllinux wheels. They are compatible with all, and exclusive to none.

In fact, pip install on an Alpine docker image will go behind your back and violate your expectations and install the wheels you claim won't be used. Because as far as pip is concerned, it is allowed to.

It is possible to tell pip not to do that, of course. You can run

pip install --only-binary ":all:" simple-websocket

But I would argue this is pretty dumb to do in a docker container, where workflows are heavily, heavily biased towards lightweight effects wherever possible. Building wheels is slow (comparatively) even when it's a pure python wheel. You have to download and install setuptools, wheel, etc. and store them in a pip cache (that may end up in one of your docker layers!!), hitting up the network multiple times to download big huge chunky files. Setuptools is an entire megabyte, and that's just the wheel, the sdist for setuptools is twice as big. The wheel for wheel is twice as big as an sdist for simple-websocket would be if tests and docs were included... and the sdist for wheel is four times as big.

Using sdists for simple-websocket when a wheel works perfectly fine would be absolutely a terrible idea in Alpine docker containers. In a situation where speedy results are extremely important, building an sdist results in network bandwidth usage getting a 100x multiplier or more just to download the build backend, and then on top of that you have to fork and execute python scripts (the pyproject_hooks module's command runner, which executes your build backend's build_wheel() API which may then go run another dozen subprocesses) in order to build, so it is also sloooooooooooooooooooooooooooow.

Why on earth would you ever, ever, ever use sdists in a situation where the artifact size is even slightly relevant? I do not understand.

You are simply wrong. You're technically wrong, because you are wrong that Alpine docker containers use sdists.

...

If one day I change my mind to include tests, then it would be tests, docs, examples, the whole thing (which is what you get if you download the source release here). And it will be done for all my open source packages, not just the one you happen to have an interest on.

For the record I would like to see tests and docs and examples and the whole thing, for all your open source packages.

There are some things that do deserve to be excluded from sdists. The main thing that leaps to mind is the .github/ directory, .gitignore, and .readthedocs.yaml although different projects have different sets of files which are intended for maintainers or CI or used specifically by git.

That being said, I mainly responded here to correct an incorrectness about Alpine docker containers, not to try to pressure you into changing your mind.

eli-schwartz commented 11 months ago

By the way, for any packages that distribute compiled C extensions and need platform-specific wheels (does not use "py3-none-any" tag), please upload musllinux wheels for Alpine docker containers. This is pretty easy to do for example using cibuildwheel.

It is a real lifesaver for Alpine users, I promise.

See https://peps.python.org/pep-0656/ for more details on musllinux wheels for Alpine (and other musl libc distributions, Alpine isn't the only one).

miguelgrinberg commented 11 months ago

@eli-schwartz We can spend days and days discussing the chaotic world of Python packaging. Sadly, I have little interest in doing that. The comment I made about Alpine was a general comment, not a comment specifically directed at this package, which as you correctly stated, is pure Python without C code.

The fact remains that today, all my packages are built with only source code in the sdist file. Why some test files slipped for this package I do not know, I'll have to find out. If you want or need a source package with everything in it, the sdist is not it, you have that in my GitHub releases. If some day I change my mind and decide to change how I build my PyPI sdist packages I will make a change in my release script and from then on the change will apply to all my packages. Today is not going to be a day I'll make changes, so you guys will have to deal with it for the time being. Thanks.