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

More packaging fixes #3314

Closed
wants to merge 2 commits into from
Closed

More packaging fixes #3314

wants to merge 2 commits into from

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Nov 25, 2021

  • Use modern build packaging
  • Correct errors from Manifest.in which caused accidental inclusion
    of .mypy_cache folders from various locations in source tree
  • Avoid tox bug with editale when setup.py is missing
  • Final archive should now be less than 300kb

Related: tox-dev/tox#2197

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of unnecessary lines in the manifest. Drop them.

MANIFEST.in Outdated Show resolved Hide resolved
include *.txt
include *.yaml
include *.yml
prune zuul.d
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want to exclude it from the sdist?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see any use to include CI config in sdist, same with .github folder. On the other hand I kept test related files so you can still run tests from source archive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of the sdist is to provide the sources for stuff, as much as possible. It's right there in the name. The users needing a small thing will use the wheel anyway (and it's configured separately). Many things use sdists as the source of truth, in particular — the downstream packagers. So don't exclude things without a really compelling reason.

MANIFEST.in Outdated
include *.yaml
include *.yml
prune zuul.d
prune .github
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintaining such exclusions is rather thankless. It's not worth the maintenance burden. Plus if you exclude something — add a comment with the motivation for each entry.

MANIFEST.in Outdated Show resolved Hide resolved
@@ -1,24 +1,31 @@
prune asset
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this line is the only entry that actually makes sense to be in this file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is far from true, i tried this myself but the result caused problems in multiple places, one of them was the check-manifest. The other was that extra built docs were included and that desired json/yaml were not included from inside src/molecule. On the other hand if you put generic recursive includes for those two extensions you may endup getting stuff like src/molecule/test/.mypy_cache/.../foo.json which are excluded by default.

I know that testing behavior of Manifest.in is quite tricky as it is highly dependent on what you may have on disk, like temp files, stuff that are not VCS tracked.

check-manifest command is very useful for debugging as it reports both kind of unexpected exclusions or inclusions but still fails to give correct recommendations or to spot the bug I mentioned.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so your problem was because of the recursive inclusion and not a bug in setuptools-scm. That makes sense, then.

MANIFEST.in Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
Correct errors from Manifest.in which caused accidental inclusion
of .mypy_cache folders from various locations in source tree
@ssbarnea
Copy link
Member Author

@webknjaz If you can find some reliable source like official packaging docs claiming that CI/CD config should be included in sdist I will do that. AFAIK, the size is still important and we should not include stuff in bulk. I am not saying I need to remove every file but at least those folders that can influence the final size in a significant way.

I really doubt any downstream maintainer relies on sdist archives instead of having a fork of the upstream repository. It would be insane to maintain an active package using only sdists.

In fact most people strip even the tests from sdist, but we do not.

@webknjaz
Copy link
Member

@webknjaz If you can find some reliable source like official packaging docs claiming that CI/CD config should be included in sdist I will do that. AFAIK, the size is still important and we should not include stuff in bulk. I am not saying I need to remove every file but at least those folders that can influence the final size in a significant way.

I'm a reliable source. A lot of things are still tribal knowledge, unfortunately. But I prefer to follow what the setuptools maintainer does for reducing the maintenance burden of looking after hundreds of projects on PyPI.
I agree that some extra excludes can be useful but if it doesn't cause problems reported by the actual users, I'd rather not give up the ease of maintaining this packaging setup.

I really doubt any downstream maintainer relies on sdist archives instead of having a fork of the upstream repository. It would be insane to maintain an active package using only sdists.

Well, I was speaking based on my experience and prior research. I can only speak about Gentoo and Fedora but they do exactly this — they point at sdists in the corresponding package specs of the respective ecosystems: https://github.com/gentoo/gentoo/blob/39729ace/app-admin/ansible-lint/ansible-lint-5.2.1.ebuild#L12 + https://src.fedoraproject.org/rpms/python-molecule/blob/c93b515b/f/python-molecule.spec#_20.
I attended PackagingCon three weeks ago and there were packagers from a lot of ecosystems, including distros and they all were in agreement on this.

In fact most people strip even the tests from sdist, but we do not.

And when they do, the downstream maintainers get especially unhappy about this. They come to the upstream projects asking to fix this problem. This was specifically emphasized in the talk of the Fedora's Python packagers, brought up and confirmed in the conversations on matrix, and I've heard this in the past from downstream maintainers in other places on the internet.

@webknjaz
Copy link
Member

Duplicate of #3324

@webknjaz webknjaz marked this as a duplicate of #3324 Nov 29, 2021
@webknjaz webknjaz closed this Nov 29, 2021
@ssbarnea ssbarnea deleted the fix/tox branch July 11, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants