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

Clean up MANIFEST.in #986

Merged
merged 3 commits into from
Feb 23, 2022
Merged

Clean up MANIFEST.in #986

merged 3 commits into from
Feb 23, 2022

Conversation

dairiki
Copy link
Contributor

@dairiki dairiki commented Jan 13, 2022

Potentially, in dev environment, a lot of cruft was being included in dists. This results in overly large installs in tox envs.

This commit tightens up the recursive-include rules in MANIFEST.in.

It also adds check-manifest to the the commands run in the lint tox env, in an attempt to make sure we're not leaving anything out.

Potentially, in dev environment, a lot of cruft was being included in
dists. This results in overly large installs in tox envs.

This commit tightens up the recursive-include rules in `MANIFEST.in`.

It also adds [check-manifest][] to the the commands run in the `lint`
tox env, in an attempt to make sure we're not leaving anything out.

[check-manifest]: https://pypi.org/project/check-manifest/
@dairiki
Copy link
Contributor Author

dairiki commented Jan 14, 2022

NB: This PR removes the tests and example subdirectories from the sdist (along with a number of git and development config files.) I'm not sure whether that will give anyone gas. But note that anyone who wants that stuff can either get it from the github repo (developers), or from the github tarball (packagers).

Maybe we should also omit the javascript & css source from the sdist?

@yagebu
Copy link
Contributor

yagebu commented Feb 17, 2022

setuptools_scm should include all git-tracked files in the sdist, so we could reduce the MANIFEST.in to just the exclude rules, right?

This PR removes the tests and example subdirectories from the sdist

I'd be for keeping the tests in the sdist. It is the historic behaviour of distutils to include tests and while there seems to be no clear consensus on on the exact purpose of the "sdist", I'm with the following answer in that thread (https://discuss.python.org/t/purpose-of-an-sdist/4639/5).
I see distribution package maintainers as the main recipients of the sdist (pip-installing end users should get wheels and Lektor developers get the git repo) and they will usually want to run the tests (see, e.g., https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/ or https://wiki.archlinux.org/title/Python_package_guidelines). I don't want them to have to go through the trouble of cloning the repo and getting the JS build to work.

@jcharaoui
Copy link

I think .eslintrc.js should also be omitted: Debian considers its presence in the Python package as a packaging error.

@dairiki
Copy link
Contributor Author

dairiki commented Feb 17, 2022

setuptools_scm should include all git-tracked files in the sdist, so we could reduce the MANIFEST.in to just the exclude rules, right?

Well, that's kind of cool!

As it turns out, check-manifest (which I had been using to tune the MANIFEST.in) does not appear to play nicely with setuptools_scm. Check-manifest ignores git when building a test sdist. As a result, the test sdist built by check-manifest differs from the "real" sdist (built via build.) It turns out that even after these patches (as they stand now) all kinds of cruft (e.g. .github/, .gitignore, as well as example/ and tests/) is getting put into the sdist.

(My first thought was how does that work when installing from an sdist, since in that case git metadata is not available? But quick — not necessarily conclusive — testing indicates that it does indeed work. I guess the manifest is coming from the .egg-info/SOURCES.txt included in the sdist.)

We do need to explicitly include anything that's not in git (e.g. the webpack output.)


I'd be for keeping the tests in the sdist.

I don't have strong feelings, either way. I still see sdists as primarily an instrument of distribution/installation (though, admittedly, given the existence of wheels, that purpose is pretty much obsolete, at least for pure-python distributions.)

What about the example subdirectory?
Also, what about the js/css source in lektor/admin/static/{js,scss}?

(Again, I don't have strong feelings either way, but none of this feels required for installation to me.)

I see distribution package maintainers as the main recipients of the sdist (pip-installing end users should get wheels and Lektor developers get the git repo)

As a third (or fourth?) alternative, some distributions appear to use the github-provided archives (e.g. https://github.com/lektor/lektor/archive/refs/tags/v3.3.1.tar.gz), which include everything (no more, no less) that's in git.


Which brings up a point: @jcharaoui: I looked quickly at the Debian packaging and it appears that it builds from the github archive (rather than the sdist). That's okay, but when building from clean git source, one has to first run webpack (e.g. via make build-js) to generate the web assets used by the admin GUI. It appears that that is not being done. As a result the Debian package does not include the webpack output under lektor/admin/static/gen/, so I suspect that lektor server will not work correctly.

(The sdist on PyPI does include the webpack output.)

@dairiki
Copy link
Contributor Author

dairiki commented Feb 17, 2022

@yagebu: Tangential question: would it be possible to move all the npm/node/tsx config files (the *.js and *.json files) that are in lektor/admin into lektor/admin/static? All the js, CSS, etc. seems to be below lektor/admin/static.

Maybe even better, what about moving all the webpack source to a top-level directory (maybe named webpack)? Webpack output would still go to lektor/admin/static/gen (or maybe just lektor/admin/static). This seems cleaner than having the js source buried so deeply. And I don't think there's any reason for the js/css source code to be included in the lektor package tree.

Maybe package.json should be in the top-level directory (alongside pyproject.toml, etc.) even?

In MANIFEST.in, we only need to manually include generated files.
But, we also need to exclude a bunch of stuff that is in git which
we don't want in the sdist.

Also, disuse `check-manifest`, since it doesn't do us much good
alongisde `setuptools_scm`.
@yagebu
Copy link
Contributor

yagebu commented Feb 18, 2022

What about the example subdirectory?

No strong preference, this to me is part of "documentation" of which we don't have a whole lot to include, so I don't think this needs to be shipped either (in particular since there's lektor quickstart to get going, one doesn't need to copy over the example folder).

Also, what about the js/css source in lektor/admin/static/{js,scss}?

Exclude.

As a third (or fourth?) alternative, some distributions appear to use the github-provided archives (e.g. https://github.com/lektor/lektor/archive/refs/tags/v3.3.1.tar.gz), which include everything (no more, no less) that's in git.

I believe that archive can't be used to build a sdist (with setuptools_scm) since the version and the list of files can't be deduced from the contents of the archive.

Maybe even better, what about moving all the webpack source to a top-level directory

I was gonna suggest that at some point and I'm happy to do that in a followup PR :) This is exactly what I did for Fava and that has worked very well. Although I'd name the directory frontend or admin_frontend since that seems more descriptive (and I want to switch away from webpack soon :D)

@jcharaoui
Copy link

Just as an aside, it's Debian's policy is to build everything from source: it's not allowed to distribute code compiled by upstreams. This is in part why Python packagers in Debian prefer to pull code from git releases instead of PyPI since the latter makes it easier to mistakenly distribute pre-compiled code.

@dairiki I'm currently preparing an package update for 3.3.1, see https://salsa.debian.org/debian/lektor/

@dairiki
Copy link
Contributor Author

dairiki commented Feb 23, 2022

@jcharaoui That's fine, but then you'll have to arrange to manually compile the javascript and CSS used by the admin frontend. Normally that's done before we build sdists and wheels for PyPI by running make build-js, which, essentially does:

cd lektor/admin
npm install
npm run webpack

This generates a number of files — js, CSS, and fonts and images — in lektor/admin/static/gen which are used by the admin frontend when running lektor server (but are not present in the git repo).

(All of those paths quoted above are about to change in PR #1003.)

Obviously, this means you'll need npm installed in the build environment. I'm not familiar enough with the Debian build tools to give specific suggestions.

@dairiki dairiki merged commit fc5d20d into lektor:master Feb 23, 2022
@dairiki dairiki deleted the bug.clean-manifest branch February 23, 2022 21:00
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