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

Add [tool.maturin.include] and [tool.maturin.exclude] #1255

Merged
merged 17 commits into from
Nov 9, 2022

Conversation

mbrobbel
Copy link
Contributor

@mbrobbel mbrobbel commented Nov 8, 2022

Based on the discussion in #1253, this is another attempt to allow users to override ignore files in Python modules by adding [tool.maturin.include] and [tool.maturin.exclude] similar to Poetry. This deprecates [tool.maturin.sdist-include].

After some feedback on this approach, I'll add:

  • Documentation updates
  • Integration tests

Closes #1253

@netlify
Copy link

netlify bot commented Nov 8, 2022

Deploy Preview for maturin-guide ready!

Name Link
🔨 Latest commit 66e3421
🔍 Latest deploy log https://app.netlify.com/sites/maturin-guide/deploys/636bc1db300a53000863e22d
😎 Deploy Preview https://deploy-preview-1255--maturin-guide.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

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

Thanks a lot for looking into this!

src/pyproject_toml.rs Show resolved Hide resolved
src/source_distribution.rs Outdated Show resolved Hide resolved
Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

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

This already looks pretty good, should be ready soon when you are done with tests and documentation changes.

BTW don't forget to add a changelog entry: https://github.com/PyO3/maturin/blob/main/Changelog.md#unreleased

src/source_distribution.rs Outdated Show resolved Hide resolved
@mbrobbel mbrobbel marked this pull request as ready for review November 8, 2022 15:51
Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

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

I think we are missing a file list check for wheel, unfortunately we don't have a existing infrastructure for this kind of test yet.

tests/run.rs Outdated Show resolved Hide resolved
tests/run.rs Outdated Show resolved Hide resolved
test-crates/pyo3-mixed-include-exclude/pyo3-config.txt Outdated Show resolved Hide resolved
@mbrobbel
Copy link
Contributor Author

mbrobbel commented Nov 8, 2022

I think we are missing a file list check for wheel, unfortunately we don't have a existing infrastructure for this kind of test yet.

I added a simple wheel file check for the test crate that was added in this PR. It helped to find an error in the target paths for files to be included in wheels.

src/lib.rs Outdated Show resolved Hide resolved
tests/common/other.rs Outdated Show resolved Hide resolved
tests/run.rs Outdated Show resolved Hide resolved
Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

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

We can simplify this test crate.

@messense messense added sdist Source distribution wheel Wheel labels Nov 9, 2022
Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@messense
Copy link
Member

messense commented Nov 9, 2022

bors r+

bors bot added a commit that referenced this pull request Nov 9, 2022
1255: Add `[tool.maturin.include]` and `[tool.maturin.exclude]` r=messense a=mbrobbel

Based on the discussion in #1253, this is another attempt to allow users to override ignore files in Python modules by adding `[tool.maturin.include]` and `[tool.maturin.exclude]` similar to [Poetry](https://python-poetry.org/docs/pyproject/#include-and-exclude). This deprecates `[tool.maturin.sdist-include]`.

After some feedback on this approach, I'll add:
- [x] Documentation updates
- [x] Integration tests

Closes #1253


Co-authored-by: Matthijs Brobbel <[email protected]>
@messense
Copy link
Member

messense commented Nov 9, 2022

@bors
Copy link
Contributor

bors bot commented Nov 9, 2022

Canceled.

@messense
Copy link
Member

messense commented Nov 9, 2022

bors delegate+

@bors
Copy link
Contributor

bors bot commented Nov 9, 2022

✌️ mbrobbel can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@messense
Copy link
Member

messense commented Nov 9, 2022

bors r+

bors bot added a commit that referenced this pull request Nov 9, 2022
1255: Add `[tool.maturin.include]` and `[tool.maturin.exclude]` r=messense a=mbrobbel

Based on the discussion in #1253, this is another attempt to allow users to override ignore files in Python modules by adding `[tool.maturin.include]` and `[tool.maturin.exclude]` similar to [Poetry](https://python-poetry.org/docs/pyproject/#include-and-exclude). This deprecates `[tool.maturin.sdist-include]`.

After some feedback on this approach, I'll add:
- [x] Documentation updates
- [x] Integration tests

Closes #1253


Co-authored-by: Matthijs Brobbel <[email protected]>
@bors
Copy link
Contributor

bors bot commented Nov 9, 2022

Already running a review

@messense
Copy link
Member

messense commented Nov 9, 2022

Still need to fix the sdist test: https://github.com/PyO3/maturin/actions/runs/3427943992/jobs/5711566272, sdist shouldn't include .pyc or .so files.

@bors
Copy link
Contributor

bors bot commented Nov 9, 2022

Build failed:

@mbrobbel
Copy link
Contributor Author

mbrobbel commented Nov 9, 2022

Still need to fix the sdist test: https://github.com/PyO3/maturin/actions/runs/3427943992/jobs/5711566272, sdist shouldn't include .pyc or .so files.

I'm confused about how these files end up in the source distribution and that this fails for the added test but not for the one this was based on? Shouldn't the root .gitignore prevent those files from being added?

@messense
Copy link
Member

messense commented Nov 9, 2022

I'm not sure, perhaps you can try add the ignores to https://github.com/PyO3/maturin/pull/1255/files#diff-130021135ea4ccb1cd3ba6e252477d65142317ffba1e4a10ad7af791d30c5b12 test-crates/pyo3-mixed-include-exclude/pyo3_mixed_include_exclude/.gitignore

If you want to debug it, use the alpine:edge docker image.

@messense
Copy link
Member

messense commented Nov 9, 2022

You can also try modify .github/workflows/test.yml and add

    - name: Setup tmate session
      uses: mxschmitt/action-tmate@v3

to test-alpine job and get a ssh shell.

@messense
Copy link
Member

messense commented Nov 9, 2022

I wasn't able to reproduce it locally using alpine:edge docker container, looks like we have to debug it with action-tmate.

@messense
Copy link
Member

messense commented Nov 9, 2022

cargo package --list --allow-dirty didn't filter them

[
    "Cargo.toml",
    "Cargo.toml.orig",
    "README.md",
    "check_installed/check_installed.py",
    "pyo3_mixed_include_exclude/__init__.py",
    "pyo3_mixed_include_exclude/__pycache__/__init__.cpython-310.pyc",
    "pyo3_mixed_include_exclude/exclude_this_file",
    "pyo3_mixed_include_exclude/include_this_file",
    "pyo3_mixed_include_exclude/pyo3_mixed_include_exclude.cpython-310-x86_64-linux-gnu.so",
    "pyo3_mixed_include_exclude/python_module/__init__.py",
    "pyo3_mixed_include_exclude/python_module/__pycache__/__init__.cpython-310.pyc",
    "pyo3_mixed_include_exclude/python_module/__pycache__/double.cpython-310.pyc",
    "pyo3_mixed_include_exclude/python_module/double.py",
    "pyproject.toml",
    "src/lib.rs",
    "tests/test_pyo3_mixed_include_exclude.py",
    "tox.ini",
]

Seems to go back to this

// Technically, `ignore` crate should handle this,
// but somehow it doesn't on Alpine Linux running in GitHub Actions,
// so we do it manually here.
// See https://github.com/PyO3/maturin/pull/1187#issuecomment-1273987013

@messense
Copy link
Member

messense commented Nov 9, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 9, 2022

@bors bors bot merged commit ee86fd8 into PyO3:main Nov 9, 2022
@mbrobbel
Copy link
Contributor Author

mbrobbel commented Nov 9, 2022

Thank you for the help and quick feedback to get this in so quickly!

@mbrobbel mbrobbel deleted the include-exclude branch November 9, 2022 16:58
@messense messense added this to the 0.14.0 milestone Nov 11, 2022
@messense messense added the configuration Configuration label Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Configuration sdist Source distribution wheel Wheel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants