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

Fix sdist file inclusion to just pack the Git-tracked files #3324

Merged
merged 2 commits into from
Nov 29, 2021

Conversation

webknjaz
Copy link
Member

The current setup mixes up incompatible technologies — setuptools-scm and check-manifest. The former uses a setuptools hook to auto-include all the Git-tracked files but following the recommendations from the latter adds extra non-tracked rubbish from various build processes on top.

Some big blobs inflate the sdist too much. Strictly speaking, they shouldn't even be in Git but it is what it is. I've originally fixed this about 2.5 years ago by adding a single exclusion via #1924.
Later on, that setup has been broken by #2943 at which point it allowed random build artifacts to slip through the misconfiguration.

This change does not attempt to exclude small text files because they do not affect the size of the sdist too much (less than 1KB). Adding extra entries introduces undesired maintenance burden and is therefore not a commonly accepted practice when it comes to setuptools-scm managed projects. The considerations are explained in #3314 (comment) and pypa/setuptools-scm#190 (comment).

See the commit messages for more specific details.

Supersedes #3314.

PR Type

  • Bugfix Pull Request

This is necessary because `setuptools-scm` that is currently
integrated, already includes all of the Git-tracked files to sdist.

This means that only a single exclusion is needed for huge image blobs
present in the Git tree now.
This is required because it tries to analyze `MANIFEST.in` without
understanding the context of the current packaging setup. It does not
even look into the actual sdist either.

Because of this, it makes incorrect assumptions, makes false claims
(like that some files are not included when in fact they are — via
the `setuptool-scm` integration hook). It also produces harmful
recommendations that mess up what's actually included and excluded
if they are being followed without understanding the context.
@webknjaz webknjaz merged commit 461cbd3 into ansible:main Nov 29, 2021
@patchback
Copy link

patchback bot commented Nov 29, 2021

Backport to stable/3.4: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 461cbd3 on top of patchback/backports/stable/3.4/461cbd3e8f84b25e9da5d6a60e8ab94b41ca7b08/pr-3324

Backporting merged PR #3324 into main

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. Make sure you have an upstream repo added as a remote too. In these
    instructions you'll refer to it by the name upstream. If you don't
    have it, here's how you can add it:
    $ git remote add upstream https:/ansible-community/molecule.git
  3. Ensure you have the latest copy of upstream and prepare a branch
    that will hold the backported code:
    $ git fetch upstream
    $ git checkout -b patchback/backports/stable/3.4/461cbd3e8f84b25e9da5d6a60e8ab94b41ca7b08/pr-3324 upstream/stable/3.4
  4. Now, cherry-pick PR Fix sdist file inclusion to just pack the Git-tracked files #3324 contents into that branch:
    $ git cherry-pick -x 461cbd3e8f84b25e9da5d6a60e8ab94b41ca7b08
    If it'll yell at you with something like fatal: Commit 461cbd3e8f84b25e9da5d6a60e8ab94b41ca7b08 is a merge but no -m option was given., add -m 1 as follows intead:
    $ git cherry-pick -m1 -x 461cbd3e8f84b25e9da5d6a60e8ab94b41ca7b08
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Fix sdist file inclusion to just pack the Git-tracked files #3324 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/stable/3.4/461cbd3e8f84b25e9da5d6a60e8ab94b41ca7b08/pr-3324
  7. Create a PR, ensure that the CI is green. If it's not — update it so that
    the tests and any other checks pass. This is it!
    Now relax and wait for the maintainers to process your pull request
    when they have some cycles to do reviews. Don't worry — they'll tell you if
    any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https:/sanitizers/patchback-github-app.

Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

I manually checked the output and it looks mostly ok. The only regression that I did spot was that it started to include .github and .zuul.d folders and I see no use for these in sdist.

@webknjaz
Copy link
Member Author

This is not a regression but an intended outcome as proven by the linked discussions and linked explanations. Did you read my comments?

@cidrblock
Copy link
Contributor

+1

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.

4 participants