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

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

Closed
KOLANICH opened this issue Sep 2, 2022 · 9 comments
Closed

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

KOLANICH opened this issue Sep 2, 2022 · 9 comments

Comments

@KOLANICH
Copy link

KOLANICH commented Sep 2, 2022

No description provided.

@jelmer
Copy link
Owner

jelmer commented Sep 2, 2022

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
Copy link
Author

KOLANICH commented Sep 2, 2022

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
Copy link
Owner

jelmer commented Sep 3, 2022

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
Copy link
Author

KOLANICH commented Sep 3, 2022

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
Copy link
Owner

jelmer commented Sep 7, 2022

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:

  • dulwich.tests.utils
  • dulwich.tests.test_object_store

@jelmer jelmer changed the title Docs and tests appear inside built wheels and are installed Only ship test modules req Sep 7, 2022
@jelmer jelmer changed the title Only ship test modules req Only ship test modules required by other packages Sep 7, 2022
@jelmer jelmer changed the title Only ship test modules required by other packages Only ship test modules in wheels required by other packages Sep 7, 2022
@dimbleby
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link

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.

jelmer added a commit that referenced this issue Jan 22, 2023
jelmer added a commit that referenced this issue Apr 13, 2024
jelmer added a commit that referenced this issue Apr 13, 2024
jelmer added a commit that referenced this issue Apr 13, 2024
jelmer added a commit that referenced this issue Apr 13, 2024
@jelmer jelmer closed this as completed in 245331a Apr 14, 2024
jelmer added a commit that referenced this issue Apr 14, 2024
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 a pull request may close this issue.

4 participants