8000 Bite the bullet: remove distutils and setup.py by stefanv · Pull Request #6738 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

Bite the bullet: remove distutils and setup.py #6738

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Feb 20, 2023
Merged

Bite the bullet: remove distutils and setup.py #6738

merged 7 commits into from
Feb 20, 2023

Conversation

stefanv
Copy link
Member
@stefanv stefanv commented Feb 8, 2023

Let's see how far we get!

@stefanv stefanv marked this pull request as draft February 8, 2023 01:05
@stefanv
Copy link
Member Author
stefanv commented Feb 8, 2023

Too early :)

@stefanv stefanv closed this Feb 8, 2023
@stefanv
Copy link
Member Author
stefanv commented Feb 8, 2023

Specifically, meson cannot build sdists (the way we like them, with compiled Cython files) the same way setuptools does. It instead takes a snapshot of the repo.

@stefanv
Copy link
Member Author
stefanv commented Feb 8, 2023 •
edited
Loading

Could build sdist using:

python -c "import setuptools; setuptools.setup()" sdist

with pyproject.toml:

[tool.setuptools.packages.find]
include = ["skimage*"]

But you'd need to specify the version in that setup call as well.

EDIT: pip -m build . --sdist

@eli-schwartz
Copy link
Contributor

Specifically, meson cannot build sdists (the way we like them, with compiled Cython files) the same way setuptools does. It instead takes a snapshot of the repo.

cython/cython#5089 (comment)

:)

However, if you wish to do so anyway you'll want to:

  • Run cython itself as a meson.add_dist_script() to add the .c files to the tarball.
  • use a bit of if/else glue, check if *.c files exist in the source tree:
    • if they do, add those as extension module sources
    • if they don't, use cython_gen.process() instead

@stefanv
Copy link
Member Author
stefanv commented Feb 8, 2023

👀 OK, I can go with that recommendation!

Still, I'd prefer not to package the whole git repo; there's a lot of CI cruft in there.

@eli-schwartz
Copy link
Contributor

Still, I'd prefer not to package the whole git repo; there's a lot of CI cruft in there.

That is a very valid point!

There are two general approaches to solving this:

  • meson.add_dist_script() can be used to make any custom changes to the tarball, which includes deleting files or editing files in addition to running cython
  • Meson uses git archive under the hood, so git's own support for .gitattributes and the export-ignore property can be used to ignore files when producing exported archives -- those will therefore not appear in the sdist either.

@stefanv
Copy link
Member Author
stefanv commented Feb 8, 2023

Awesome, I'm going to try that; thanks!

@stefanv stefanv reopened this Feb 10, 2023
@jarrodmillman jarrodmillman added this to the 0.21 milestone Feb 13, 2023
@stefanv stefanv force-pushed the remove-distutils-and-setup.py branch 12 times, most recently from 15f04e8 to 407dbcd Compare February 18, 2023 01:34
@stefanv stefanv force-pushed the remove-distutils-and-setup.py branch 2 times, most recently from 51d31eb to 17bdf7a Compare February 18, 2023 17:53
@stefanv stefanv marked this pull request as ready for review February 18, 2023 17:53
@stefanv
Copy link
Member Author
stefanv commented Feb 18, 2023

OK @jarrodmillman, this should be good to go now.

Please keep these commits separate, instead of squashing the PR.

@stefanv stefanv force-pushed the remove-distutils-and-setup.py branch from 17bdf7a to 18d2919 Compare February 18, 2023 17:57
@stefanv
Build sdists using build/meson
91bc8ef 
Meson uses git-archive underneath the hood, so what used to the
MANIFEST.in now lives in .gitattributes.

Also adds a devpy command for this action: `./dev.py sdist`.
@stefanv
Use math defines on Windows
0cb2ac4
@stefanv
Remove compilers other than clang
fef0d53
@stefanv stefanv force-pushed the remove-distutils-and-setup.py branch from 0834cc4 to 7e70b61 Compare February 18, 2023 18:20
@stefanv
Copy link
Member Author
stefanv commented Feb 18, 2023

@jarrodmillman Scratch that; windows is now fine, but got some fixes on the Linux side still.

@stefanv stefanv force-pushed the remove-distutils-and-setup.py branch from 7e70b61 to e0d0007 Compare February 18, 2023 18:34
@stefanv
Copy link
Member Author
stefanv commented Feb 18, 2023

@jarrodmillman I followed the new Cython guidance (see #6738 (comment) above) and no longer add compiled Cython files to the sdist. This probably deserves a release note, what do you think?

@stefanv
Copy link
Member Author
stefanv commented Feb 18, 2023

✔️ 😅

@jarrodmillman jarrodmillman modified the milestones: 0.21, 0.20 Feb 19, 2023
@jarrodmillman jarrodmillman merged commit 49cb3a4 into scikit-image:main Feb 20, 2023
@lagru lagru added the 🤖 type: Infrastructure CI, packaging, tools and automation label Feb 26, 2023
@lesteve lesteve mentioned this pull request Jun 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jarrodmillman jarrodmillman Awaiting requested review from jarrodmillman

Assignees
No one assigned
Labels
🤖 type: Infrastructure CI, packaging, tools and automation
Projects
None yet
Milestone
0.20
Development

Successfully merging this pull request may close these issues.
4 participants
@stefanv @eli-schwartz @jarrodmillman @lagru
0