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

Use Flit for packaging #589

Merged
merged 10 commits into from
Sep 9, 2022
Merged

Use Flit for packaging #589

merged 10 commits into from
Sep 9, 2022

Conversation

engineervix
Copy link
Contributor

@engineervix engineervix commented Jul 1, 2022

Starting with PEP 621, the Python community selected pyproject.toml as a standard way of specifying project metadata. Given that the future of Python packaging is pyproject.toml (based on PEP 518 and PEP 621, transitioning Python projects to use pyproject.toml is inevitable.

This PR introduces Flit as a packaging tool for the project. Flit prides itself in its provision of a simple way to create and upload pure Python packages and modules to PyPI. It focuses on making the easy things easy for packaging.

TODO:

  • Update README / docs with build instructions
  • Update the relevant Makefiletargets to incorporate Flit

Add `__pycache__/` and `*.py[co]`, as the MANIFEST.in had

- prune **/__pycache__
- global-exclude *.py[co]

Since Flit builds an sdist tarball containing the files that are checked into version control (git or mercurial).
By adding these two declarations to `.gitignore`, we are assured that flit
will not include them when building

For more information, see https://flit.pypa.io/en/latest/pyproject_toml.html#sdist-section
Since they are not used by Flit
Flit currently doesn't support recursive glob patterns (**/)
so we have to hardcode a lot of things to exclude/include

there is, however, an open PR on the Flit repo:
pypa/flit#550
@engineervix engineervix changed the title WIP: Use Flit for packaging Use Flit for packaging Jul 1, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2022

Codecov Report

Merging #589 (78dde34) into main (ebde7a3) will decrease coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #589      +/-   ##
==========================================
- Coverage   91.60%   91.51%   -0.09%     
==========================================
  Files          47       47              
  Lines        3907     3913       +6     
  Branches      593      595       +2     
==========================================
+ Hits         3579     3581       +2     
- Misses        186      188       +2     
- Partials      142      144       +2     
Impacted Files Coverage Δ
wagtail_localize/views/edit_translation.py 85.25% <0.00%> (-0.60%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebde7a3...78dde34. Read the comment docs.

@chris48s
Copy link
Contributor

chris48s commented Jul 1, 2022

Nice. I'll have a proper look over this next week, but I'll flag a couple of quick things now. One is we need to get the netlify builds working:

https://github.com/wagtail/wagtail-localize/blob/main/netlify.toml

Tbh, it should probably work as-is if we upgrade pip:

pip install --upgrade pip
pip install -e.[testing,documentation]

but it would be more idiomatic to use filt. Looks like the equivalent would be:

pip install flit
flit install --extras testing documentation

(untested)

Also it looks like the Github Actions CI builds are passing, but again it would probably be nicer to invoke flit than pip here

and we're going to want to create our cache keys based on hashing pyproject.toml, not setup.py

Can you add those to the list of next things to look at.

As I say, I'll have a more detailed look next week.

@chris48s chris48s self-requested a review July 1, 2022 19:41
Copy link
Contributor

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this so far Victor. Its looking good. I've spent quite a while looking over this as we are both learning as we go here. One of the things I did as part of testing this was I installed it as a local package inside a wagtail bakery instance as a host app. No comments on that - works great but I wanted to confirm that I've tested that particular use-case.

pyproject.toml Show resolved Hide resolved
Comment on lines +84 to +86
include = [
"wagtail_localize/static"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is probably more a clarification for @zerolab when he comes to review it than anything else as it confused me initially:

The reason why we have to explicitly include this here (but we don't have to do this for the templates or .mo files, etc) is because flit respects .gitignore, so if we want to include files in the package build that are not checked into source control we have to tell flit to do it.

pyproject.toml Show resolved Hide resolved
@chris48s
Copy link
Contributor

chris48s commented Jul 5, 2022

Looks like the equivalent would be:

pip install flit
flit install --extras testing documentation

(untested)

Just corecting myself here:

Having played with it locally, the default for flit install is actually --deps all so it will install all optional dependencies by default. This is actually pretty reasonable for local dev and in line with what tools like NPM and poetry do. In CI it might sometimes be useful to just install the subset we need for performance reasons, so actually what we want to do here is explicitly flit install --deps production when we only want the required packages (so we can install fewer things for fast builds when we don't need the optional packages) and then flit install --deps production --extras documentation,testing or flit install --deps production --extras google when we do want specific optional packages.


On Documentation:

Having chewed this over, one nice property of using flit is that if you have a sufficiently recent version of pip and you don't need to build/publish the package, you don't actually need to care that we are using flit as our build system. i.e: even when we migrate to flit

git clone [email protected]:wagtail/wagtail-localize.git
cd wagtail-localize
pip install -e .[testing] -U

will still get you a working development environment that you can use to contribute a feature/bugfix.

Given that, I wonder if we should change the docs at all, or maybe document:

### Using pip

...


### Using flit

...

We can actually make flit mainly a concern for maintainers and not something a casual contributor needs to worry about, which is probably good for community contributions.

Given that it probably makes sense for anything we're expecting to run locally (e.g:

install_command = pip install -e ".[testing]" -U {opts} {packages}
) to continue to run pip rather than flit so you don't actually need it to be installed.

@chris48s
Copy link
Contributor

I don't want this to spill past the end of the rock quarter so I've just pushed the commits to address the review comments myself

@chris48s chris48s marked this pull request as ready for review July 23, 2022 16:10
@chris48s chris48s requested a review from zerolab July 23, 2022 16:10
@@ -1,73 +0,0 @@
#!/usr/bin/env python
Copy link
Collaborator

Choose a reason for hiding this comment

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

the one minor downside to this is that we are likely to lose the "used by" GitHub feature, but we can live with it

Copy link
Contributor

Choose a reason for hiding this comment

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

You may be right. I know for sure that if you switch to poetry, you definitely lose the 'dependents' tab in your dependency graph

I'm not actually sure what happens if you use something like flit or hatch which uses a more 'standard' pyproject.toml layout (poetry basically just shoves everything in a tool.poetry). I don't think we know until we merge it.

The irony of this is: One of the motivators for moving from a turing-complete manifest (setup.py) to a declarative one (pyproject.toml) is it is much easier for analysis tools (stuff like dependabot/renovate, licence complicance checkers, security scanners like snyk, etc) to parse and reason about a toml file that is just data. So it is kind of annoying that sometimes at the moment the reality of switching from setup.py to pyproject.toml means worse support from analysis tools because things move slowly 🙃

Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Took forever to get to this. This is excellent. Thank you @engineervix and @chris48s

A few observations from local testing:

Package time

time make package

setuptools flit
0.38s user 0.72s system 78% cpu 1.405 total 0.30s user 0.55s system 88% cpu 0.956 total

File sizes

type setuptools flit diff
egg 272174 258246 5.12% ⬇️
wheel 518778 458771 11.56% ⬇️
Wheel diff (excluding package metadata files)
deleted:    wagtail_localize/machine_translators/tests/__init__.py
deleted:    wagtail_localize/machine_translators/tests/test_dummy_translator.py
deleted:    wagtail_localize/segments/tests/__init__.py
deleted:    wagtail_localize/segments/tests/test_segment_extraction.py
deleted:    wagtail_localize/segments/tests/test_segment_ingestion.py
deleted:    wagtail_localize/segments/tests/test_segmentvalue.py
deleted:    wagtail_localize/test/__init__.py
deleted:    wagtail_localize/test/apps.py
deleted:    wagtail_localize/test/migrations/0001_initial.py
deleted:    wagtail_localize/test/migrations/__init__.py
deleted:    wagtail_localize/test/models.py
deleted:    wagtail_localize/test/settings.py
deleted:    wagtail_localize/test/urls.py
deleted:    wagtail_localize/test/wagtail_hooks.py
deleted:    wagtail_localize/tests/__init__.py
deleted:    wagtail_localize/tests/test_convert_to_alias.py
deleted:    wagtail_localize/tests/test_edit_translation.py
deleted:    wagtail_localize/tests/test_get_translatable_fields.py
deleted:    wagtail_localize/tests/test_strings.py
deleted:    wagtail_localize/tests/test_submit_translations.py
deleted:    wagtail_localize/tests/test_synctree.py
deleted:    wagtail_localize/tests/test_tasks.py
deleted:    wagtail_localize/tests/test_translation_components.py
deleted:    wagtail_localize/tests/test_translation_model.py
deleted:    wagtail_localize/tests/test_translations_report.py
deleted:    wagtail_localize/tests/test_translationsource_model.py
deleted:    wagtail_localize/tests/test_update_translations.py
deleted:    wagtail_localize/tests/utils.py

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.

5 participants