pyinvoke / invoke

Pythonic task management & command execution.
http://pyinvoke.org
BSD 2-Clause "Simplified" License
4.31k stars 365 forks source link

Don't forget to add `py.typed` file to package metadata #936

Closed sobolevn closed 1 year ago

sobolevn commented 1 year ago

While working on https://github.com/python/typeshed/pull/10100 I've noticed that your project misses py.typed file in the package (version 2.1.0).

See:

» ls .venv/lib/python3.10/site-packages/invoke/
__pycache__/  __init__.py    config.py      executor.py  runners.py    watchers.py
completion/   __main__.py    context.py     loader.py    tasks.py
parser/       _version.py    env.py         main.py      terminals.py
vendor/       collection.py  exceptions.py  program.py   util.py

This line of code should package py.typed as well. Can you please make 2.1.1 release? :)

AlexWaygood commented 1 year ago

@bitprophet, to clarify -- without this change, the py.typed file is not included in wheels uploaded to PyPI. Without the py.typed file, mypy doesn't know to look in site-packages/invoke/ for type hints, so will emit errors when users try to import invoke in their code.

bitprophet commented 1 year ago

Guessing this was an oversight in the typing PR. Can you clarify why this wants to go in package_data and not MANIFEST.in? If I go the latter route (which is normally how I get extra files added to the dists and I'd prefer to be consistent with that) I do now see the py.typed file (and it was definitely not in there beforehand, I checked):

diff --git a/MANIFEST.in b/MANIFEST.in
index ba413a3b..61fc0ece 100644
--- a/MANIFEST.in
+++ b/MANIFEST.in
@@ -5,6 +5,7 @@ recursive-include invoke/completion *
 recursive-include sites *
 recursive-exclude sites/*/_build *
 include dev-requirements.txt
+recursive-include * py.typed
 recursive-include tests *
 recursive-exclude * *.pyc *.pyo
 recursive-exclude **/__pycache__ *
» python setup.py bdist_wheel
<snip>
» unzip -l dist/invoke-2.1.1-py3-none-any.whl|grep typed
        0  05-02-2023 02:04   invoke/py.typed
sobolevn commented 1 year ago

Done!

bitprophet commented 1 year ago

Thanks! I just changed the base branch to the bugfix branch too.

Was going to put a changelog entry in and call it done, but realized: it'd be nice for however you noticed this, to be added to the install test step somehow - if we're gonna have typing support we should make sure it actually works!

Is there a nice concise reproduce case for what happens when py.typed isn't in the dist/wheel? (eg, some arbitrary module that imports Invoke, does its own typechecks, and then runs mypy - or something like that)

I'd probably need to add whatever that test is, to invocations (which happens to be what invoke uses to run most of its CI bits) so I don't think there's a useful test you can add at the unit/integration level, since those already have a source checkout locally. But giving me the repro case is fine :)

bitprophet commented 1 year ago

Oh Github. naturally, changing the base + adding a changelog entry == conflict. Let's see if their TTW tooling lets me resolve that conflict correctly or if I just need to make my own integration branch after all.

(I usually default to just cherry-picking folks' commits instead of trying to force the PR itself to end up 100% correct...figured I would try the 'github way' again. alas!)

EDIT: it appears to have worked! nice. being able to commit into other peoples' forks is extremely, extremely weird still.

bitprophet commented 1 year ago

This is ready for merge but I'd like us to figure out the test angle before I do. Thanks!

AlexWaygood commented 1 year ago

Here's a possible way of testing this in a GitHub Actions workflow:

jobs:
  check-pep561-compliance:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
        with:
          path: invoke
      - uses: actions/setup-python@v4
        with:
          python-version: "3.11"
      - run: |
          pip install mypy
          pip install ./invoke
          mkdir newdir
          cd newdir
          mypy -c "import invoke"
bitprophet commented 1 year ago

Thanks @AlexWaygood, confirmed that works pretty well to blow up when there's no py.typed.

bitprophet commented 1 year ago

Deleted some confusion. Still seeing weird behavior trying to prove/disprove this in my CI-oriented tasks, but I'll figure it out. Maybe not late in the day on a Friday though 😓

bitprophet commented 1 year ago

Cache invalidation: one of the top two compsci problems strikes again. (A py.typed was being preserved in a build directory and making me incredibly confused as I bounced between 'in dists, not in dists, present in source, not present in source'.)

bitprophet commented 1 year ago

Great, I now have the main 2.1 branch failing correctly re: this issue: https://app.circleci.com/pipelines/github/pyinvoke/invoke/312/workflows/c3ac0409-a669-4c1b-83aa-77d44279fb6b/jobs/1923?invite=true#step-106-466

bitprophet commented 1 year ago

Hm, doesn't look like faux-changing the base branch made GH do a rebase. I can 'rebase and merge' but I kinda wanted to 'rebase and prove the tests pass' first. Will try bouncing back and forth a bit (plus main branch now has my it's-gonna-break fix in it too.)

EDIT: nada. whatever. gonna just go back to my local integration branches in future I think 🙃 anyway, merging!

bitprophet commented 1 year ago

Invoke 2.1.2 out now! Thanks again.