jelmer / dulwich

Pure-Python Git implementation
https://www.dulwich.io/
Other
2.04k stars 390 forks source link

Only ship test modules in wheels required by other packages #1024

Closed KOLANICH closed 5 months ago

jelmer commented 2 years ago

Some other applications reuse Dulwich' tests (e.g. when reimplementing its interfaces), which is why they are included.

I can't remember explicitly adding docs to the wheel, but I'm open to dropping to the docs if that's not a common practice. Is there a reference somewhere that suggests they shouldn't be included?

KOLANICH commented 2 years ago

Some other applications reuse Dulwich' tests (e.g. when reimplementing its interfaces), which is why they are included.

Can those apps clone dulwich repo instead and use tests from it? I really doubt the tests are needed for anything except testing.

I can't remember explicitly adding docs to the wheel

It seems they have been added within #223

but I'm open to dropping to the docs if that's not a common practice. Is there a reference somewhere that suggests they shouldn't be included?

No reference, but usually not included.

Also, one shouldn't use scripts. These is not cross-platform. I recommend migrating to PEP621 and using its scripts (it is different from setuptools scripts, it is not a path to a script, but an entry_point, in fact that section is transformed into a special entry point (for which pip will generate a native binary or script that can be called by just typing its name, depending on the platform), in legacy setuptools config we had to code that entry point explicitly, and it is the right way to specify cli tools).

jelmer commented 2 years ago

Some other applications reuse Dulwich' tests (e.g. when reimplementing its interfaces), which is why they are included.

Can those apps clone dulwich repo instead and use tests from it? I really doubt the tests are needed for anything except testing.

Sure, but isn't that the whole point of having a package manager? We could potentially ship just the specific tests that need to be reused - but that's a non-trivial amount of work. I'll leave this bug open to track that.

I can't remember explicitly adding docs to the wheel

It seems they have been added within #223

but I'm open to dropping to the docs if that's not a common practice. Is there a reference somewhere that suggests they shouldn't be included?

No reference, but usually not included.

Also, one shouldn't use scripts. These is not cross-platform. I recommend migrating to PEP621 and using its scripts (it is different from setuptools scripts, it is not a path to a script, but an entry_point, in fact that section is transformed into a special entry point (for which pip will generate a native binary or script that can be called by just typing its name, depending on the platform), in legacy setuptools config we had to code that entry point explicitly, and it is the right way to specify cli tools).

I'm open to a PR that fixes this (and/or a separate issue to track it) but haven't considered it a high enough priority to work on. The only two scripts that are included in "scripts" probably don't work on windows anyway. The main executable is included as an entry point when setuptools is installed. At some point we should probably drop the distils codepath (used when setuptools is not available) now that it is deprecated.

KOLANICH commented 2 years ago

Sure, but isn't that the whole point of having a package manager? We could potentially ship just the specific tests that need to be reused - but that's a non-trivial amount of work. I'll leave this bug open to track that.

Then they can be a separate package.

jelmer commented 2 years ago

Sure, but isn't that the whole point of having a package manager? We could potentially ship just the specific tests that need to be reused - but that's a non-trivial amount of work. I'll leave this bug open to track that.

Then they can be a separate package.

I think that'd be overkill. Specifically, we need to ship the following modules with the main package:

dimbleby commented 2 years ago

My understanding is that the usual convention is not to include tests or docs in binary distributions, but it's fine to include them in source distributions.

so that would suggest removing the stuff in setup.py that includes them, but compensating as needed in MANIFEST.in to make sure that everything is there.

(I'm not particularly fussed about this either way, I just happened to run into a similar case elsewhere recently and so have a relatively fresh opinion)

KOLANICH commented 2 years ago

My understanding is that the usual convention is

In fact the usual convention is just not to care about it at all and this results in pretty a lot of wheels having garbage within them, from readme, license and other files (readme goes to package metadata usually, license file is captured automatically and also goes to metadata, so no need to include it separately) to tests dir alongside the main package dir (so these tests go to site-packages/tests when installed, possibly conflicting with the same way included tests dir of other packages, so it clearly an unintended mistake, they are usually captured if there is __init__.py file within their dir and package.find is used without needed includes/excludes).

not to include tests or docs in binary distributions, but it's fine to include them in source distributions.

Completely aggree.

but compensating as needed in MANIFEST.in to make sure that everything is there.

doesn't MANIFEST.in also take effect while building wheels (at least I remember I had to add some recursive-includes to get some data (JSONSchema) files into wheels)?

dimbleby commented 2 years ago

doesn't MANIFEST.in also take effect while building wheels ... ?

experiment suggests not, and https://packaging.python.org/en/latest/guides/using-manifest-in/ certainly is only discussing sdists, but I'm happy to defer to anyone who knows better!

webknjaz commented 1 year ago

My understanding is that the usual convention is not to include tests or docs in binary distributions, but it's fine to include them in source distributions.

It is true. It is common to exclude the tests from wheels while keeping them in sdists. There's one exception I can think of, though — test helpers that can be treated as public API. Those specifically exist to be used by other projects. For example, a project could provide pytest fixtures and stubs/mocks/doubles for the end-users to test against, but the tests that verify the library itself don't fall under this category. So it sounds like maybe there could be some restructuring made to make this distinction.

I can't remember explicitly adding docs to the wheel, but I'm open to dropping to the docs if that's not a common practice. Is there a reference somewhere that suggests they shouldn't be included?

There's been numerous discussions across multiple avenues of the PyPA community. But the bottom line is that things outside your main package directory will flood the root of the site-packages. Moreover, pip isn't perfect and if several projects have docs/ and tests/ folders that get installed as site-packages/docs/ and site-packages/docs/, it'll let that happen and that may result in confusion of which files belong to which installable distribution. Sometimes, it'll break things, sometimes it'll work. So the way out of this problem is to move the tests under the importable package directory if you do intend to install them. Or you should separate the public helpers vs the actual project tests and exclude them from the wheel using the exclude_package_data setting.