Skip to content

Conversation

@bollwyvl
Copy link

@bollwyvl bollwyvl commented Mar 16, 2025

Congratulations on 1.36.1, and as always thank you for maintaining this tool!

Downstream we noted that the tests folder is incomplete, failing to import with tests.common being missing. We shipped just getting those files from the GitHub-generated tarball, but it is generally to convenient to have a single canonical, immutable file (and SHA) to track.

I don't think sdist-only files can be added directly in pyproject.toml with the setuptools legacy backend, so this PR adds a one-liner MANIFEST.in to preserve all the files in tests in the sdist.

Thanks again!

@bollwyvl bollwyvl marked this pull request as ready for review March 16, 2025 15:34
@coveralls
Copy link

Coverage Status

coverage: 99.825%. remained the same
when pulling 2076efe on bollwyvl:fix-tests-in-sdist
into 27d05bd on adrienverge:master.

adrienverge added a commit that referenced this pull request Mar 17, 2025
This commit fixes a problem reported by Nicholas Bollweg at
#725. Recent commit a3e1325
"CI: Publish PyPI releases using GitHub Actions workflows" and automatic
publication of version 1.36.1 revealed a problem that had probably been
there for some time: most documentation and test files weren't included
in the source distribution when running `python -m build` (unless an
`yamllint.egg-info` from a previous run listed them).

I see two solutions:
1. Add plugin `setuptools-scm` in `pyproject.toml`:
       requires = ["setuptools >= 61", "setuptools-scm >= 8"]
   This would add all files tracked by Git in the sdist, including a few
   unneeded for sdist: `.flake8`, `.github`, `.readthedocs.yaml`…
2. Declare extra files to embed in `MANIFEST.in`. This is what this
   commit does.

I checked that:
- All files packages before 1.36.0 are correctly included now:
      tar -tvf dist/yamllint-1.36.0.tar.gz | cut -b65- \
        > /tmp/sdist-files-before
      rm -rf yamllint.egg-info && python -m build && \
        tar -tvf dist/yamllint-1.36.1.tar.gz | cut -b65- \
        > /tmp/sdist-files-after
      git diff --no-index /tmp/sdist-files-before /tmp/sdist-files-after
- These extra files are still not installed by `pip install` (they are
  not needed):
      pip install yamllint==1.36.0 && \
        tree ~/.local/lib/python3.13/site-packages/yamllint \
        > /tmp/pip-install-before
      pip install dist/yamllint-1.36.1.tar.gz && \
        tree ~/.local/lib/python3.13/site-packages/yamllint \
        > /tmp/pip-install-after
      git diff --no-index /tmp/pip-install-before /tmp/pip-install-after
adrienverge added a commit that referenced this pull request Mar 17, 2025
This commit fixes a problem reported by Nicholas Bollweg at
#725. Recent commit a3e1325
"CI: Publish PyPI releases using GitHub Actions workflows" and automatic
publication of version 1.36.1 revealed a problem that had probably been
there for some time: most documentation and test files weren't included
in the source distribution when running `python -m build` (unless an
`yamllint.egg-info` from a previous run listed them).

I see two solutions:
1. Add plugin `setuptools-scm` in `pyproject.toml`:
       requires = ["setuptools >= 61", "setuptools-scm >= 8"]
   This would add all files tracked by Git in the sdist, including a few
   that aren't really needed: `.flake8`, `.github`, `.readthedocs.yaml`…
2. Declare extra files to embed in `MANIFEST.in`. This is what this
   commit does.

I checked that:
- All files packages before 1.36.0 are correctly included now:
      tar -tvf dist/yamllint-1.36.0.tar.gz | cut -b65- \
        > /tmp/sdist-files-before
      rm -rf yamllint.egg-info && python -m build && \
        tar -tvf dist/yamllint-1.36.1.tar.gz | cut -b65- \
        > /tmp/sdist-files-after
      git diff --no-index /tmp/sdist-files-before /tmp/sdist-files-after
- These extra files are still not installed by `pip install` (they are
  not needed):
      pip install yamllint==1.36.0 && \
        tree ~/.local/lib/python3.13/site-packages/yamllint \
        > /tmp/pip-install-before
      pip install dist/yamllint-1.36.1.tar.gz && \
        tree ~/.local/lib/python3.13/site-packages/yamllint \
        > /tmp/pip-install-after
      git diff --no-index /tmp/pip-install-before /tmp/pip-install-after
@adrienverge
Copy link
Owner

Hello Nicholas,

Thanks for the report!

Indeed this needs fixing. The present PR does not fully resolve the problem, because other files (e.g. documentation in docs/, CHANGELOG.rst, CONTRIBUTING.rst, etc.) are also missing.
Also we don't want to use a greedy pattern like*.*, otherwise we would ship dozens of unneeded files like tests/rules/common.pyc or the contents of tests/__pycache__.

So I implemented #726 instead. Can you confirm it solves the problem on your side?

adrienverge added a commit that referenced this pull request Mar 17, 2025
This commit fixes a problem reported by Nicholas Bollweg at
#725. Recent commit a3e1325
"CI: Publish PyPI releases using GitHub Actions workflows" and automatic
publication of version 1.36.1 revealed a problem that had probably been
there for some time: most documentation and test files weren't included
in the source distribution when running `python -m build` (unless an
`yamllint.egg-info` from a previous run listed them).

I see two solutions:
1. Add plugin `setuptools-scm` in `pyproject.toml`:
       requires = ["setuptools >= 61", "setuptools-scm >= 8"]
   This would add all files tracked by Git in the sdist, including a few
   that aren't really needed: `.flake8`, `.github`, `.readthedocs.yaml`…
2. Declare extra files to embed in `MANIFEST.in`. This is what this
   commit does.

I checked that:
- All files packages before 1.36.0 are correctly included now:
      tar -tvf dist/yamllint-1.36.0.tar.gz | cut -b65- \
        > /tmp/sdist-files-before
      rm -rf yamllint.egg-info && python -m build && \
        tar -tvf dist/yamllint-1.36.1.tar.gz | cut -b65- \
        > /tmp/sdist-files-after
      git diff --no-index /tmp/sdist-files-before /tmp/sdist-files-after
- These extra files are still not installed by `pip install` (they are
  not needed):
      pip install yamllint==1.36.0 && \
        tree ~/.local/lib/python3.13/site-packages/yamllint \
        > /tmp/pip-install-before
      pip install dist/yamllint-1.36.1.tar.gz && \
        tree ~/.local/lib/python3.13/site-packages/yamllint \
        > /tmp/pip-install-after
      git diff --no-index /tmp/pip-install-before /tmp/pip-install-after
@bollwyvl
Copy link
Author

Sure, explicit is better than implicit!

Just shipped the (tested) 1.36.2 from conda-forge/yamllint-feedstock#46 with just content from the PyPI sdist.

Frankly the best way to ensure this kind of thing is to have a CI excursion that does the equivalent of what a downstream would do:

  • in one job
    • build the sdist
    • upload with actions/upload-artifacts
  • in another job (in the same workflow, and maybe on another python)
    • don't check out the code
    • download with actions/upload-artifacts
    • install the sdist
      • pip check to catch any runtime dependency issues
    • install the sdist with the [test] extra
      • unpack (just) the tests
      • run the tests with -W and a coverage threshold to catch any packaging or test dependency issues
    • reinstall the sdist with [docs] extra
      • build the docs with -W to catch any docs issues

@bollwyvl bollwyvl closed this Mar 17, 2025
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