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

packaging: Explicit add data files to the manifest #7876

Closed
wants to merge 1 commit into from

Conversation

schopin-pro
Copy link

We've started seeing some failures to build in Debian and Ubuntu that can be traced to those files missing from the manifest when we're calling setup.py install.

Now, this started failing fairly recently, so I'm guessing it's actually a change of behaviour in the SCM plugin and/or setuptools. It makes sense though, since we're packaging from the tarball, which isn't in the Git context needed to actually include those data files.

We've started seeing some failures to build in Debian and Ubuntu that
can be traced to those files missing from the manifest when we're
calling setup.py install.

Now, this started failing fairly recently, so I'm guessing it's actually
a change of behaviour in the SCM plugin and/or setuptools. It makes
sense though, since we're packaging from the tarball, which isn't in the
Git context needed to actually include those data files.

Signed-off-by: Simon Chopin <simon.chopin@canonical.com>
@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Oct 16, 2023

There are 2 (AFAIK) working workflows:

  • installing the pypi .tar.gz package with pip install (it contains and installs these files)
  • building this package from a git checkout with python setup.py sdist (having a git checkout is important here, because setuptools_scm manages most of the manifest: everything committed in git gets into the package. In MANIFEST.in there are only the exceptions to that general rule.)
  • using pip install -e . from the git checkout
  • not sure what's different in the debian workflow, in python setup.py install or whether there were changes in setuptools_scm.

@RonnyPfannschmidt does that ring some bell?

BTW, I rather would not like to start again manually managing the manifest like suggested in this PR, we did this in the past and it is error prone.

@schopin-pro
Copy link
Author

I completely understand you not wanting to actually adopt this patch, it's mostly a workaround for the issue I've been seeing. I'm not the Debian maintainer for borgbackup2, just a Ubuntu developer that happened upon the build failure and tried to make things better.

The following is based on wild guesses and assumptions:

  • The tarball you upload to PyPI is the result of python setup.py sdist, so pip install doesn't actually run setup.py, and instead directly puts the files where it needs without running setup.py.
  • On the other hand, Debian packaging tries to redo everything from first principles, and seeing a setup.py in the tarball (which is the one from PyPI), assumes it will work.
  • Something changed at some point in the setuptools ecosystem that broke that "first principles" rebuild if SCM data isn't available. Maybe it used to be "in case of doubt, ship the file with" and now it has been flipped?

Are the tarballs on your GH releases the same as the ones on PyPI?

Anyway, I'll ask some people more versed into the fine art of Python packaging in Debian to have a look here.

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Oct 16, 2023

@schopin-pro The tarballs I put on github releases are precisely the ones I uploaded to pypi (plus the gpg signature, which pypi does not accept any more). But don't confuse these files with the archives autogenerated by github (I can't avoid that), these zip and tgz archives are not pypi / pip packages.

@stefanor
Copy link

My guess is that this is caused by our (Debian's) default cleaning egg-info, which we recently turned on.

If you are relying on setuptools_scm to locate files to ship (and not listing them in MANFEST.in), then we are leaving those files out when regenerating the egg-info.

This PR looks correct in light of that.

@stefanor
Copy link

not sure what's different in the debian workflow, in python setup.py install or whether there were changes in setuptools_scm.

What's different is that we aren't in a git checkout.

@ThomasWaldmann
Copy link
Member

If you don't work based on a git checkout, you must use the sdist package from us as starting point, otherwise you will be missing information, because setuptools_scm needs the git information to build the manifest.

@stefanor
Copy link

Or you apply this change.

If you don't want to support work from a tarball, and accept patches related to it, then you may as well drop your MANIFEST.in entirely.

@stefanor
Copy link

From our side, the reason we delete the existing egg-info is that when we build the package, it usually changes the egg-info. Building it cleanly makes the builds more reproducible (and stops our tooling from detecting changes not carried in a patch file).

@ThomasWaldmann
Copy link
Member

@stefanor see the comment at top of the MANIFEST.in.

Basically, we already aren't using it for all files committed to git that shall be in the manifest, but there are a few exceptions and that's why we need it still, to enter these exceptions.

@stefanor
Copy link

@stefanor see the comment at top of the MANIFEST.in.

Aha, I see, sorry missed that. Perfectly hidden in the diff on this PR by default :)

I came as a drive-by reviewer, at the request of @schopin-pro.

@ThomasWaldmann
Copy link
Member

BTW, there were some packaging related changes in 1.4-maint (that just had its first release in form of 1.4.0a1):

  • less usage of setup.py (calling it directly is deprecated since quite a while), but rather using pip and build.
  • move all metadata from setup.* to pyproject.toml
  • move conftest.py into the package, so it is installed and the tests can be run from the installed package (see issues about that)

Maybe it would be worth doing some early Debian/Ubuntu packaging and testing trials to see if this improved the situation for you.

@ThomasWaldmann
Copy link
Member

Closing this, not needed if one uses the correct workflow.

Even if one does not use a git checkout, one can get this right by using the sdist .tgz provided by us and installing it using pip.

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 this pull request may close these issues.

3 participants