-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 hatchling as build backend #12269
Conversation
Great work @hoechenberger! I tested an editable install in a workspace outside of the MNE source, and I get all completions in VS Code. On To me, this looks good and could be merged. If we discover that something is missing, or that we want to exclude even more, we can always change it later. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried the sdist, wheel, and an editable install on this branch and all seemed to work fine 👍
tools/generate_codemeta.py needs to be adjusted, but I don't understand the code well enough to do it myself.
What needs to be adjusted there? It just ran without errors for me on your branch and generated a diff
that looked okay with a quick glance.
A sorry, I suppose I was mistaken, we already took care of those changes when we moved all deps into pyproject.toml recently. I got mixed up. |
There was a problem hiding this 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! I'm very glad static analysis is working for you again in VSCode 🙂
@ofek Since you're around – do you know if there are any plans to integrate |
Not at the moment as I don't have time for that. Also I know the maintainer of setuptools_scm which that package is backed by and it seems like a lot of work so I would much rather just outsource to Ronny. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one comment/question. LGTM
"io/edf/gdf_encodes.txt", | ||
[tool.hatch.build] | ||
exclude = [ | ||
"/.*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like it excludes all files at the root level of the repo; if that's right I don't see why we're explicitly excluding things like /Makefile
etc below. Am I misinterpreting /.*
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the same syntax as gitignore and matches any entry at the root that starts with a period
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I would recommend using inclusion overall rather than exclusion to account for new stuff in future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the same syntax as gitignore and matches any entry at the root that starts with a period
ah right, like glob not regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~everything that's tracked in the VCS (git) is included by default, hence I picked the exclusion-based approach to exclude what's tracked in git, but shouldn't be packaged
Do you think that's not a good idea, @ofek?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's perfectly valid but my personal preference for source distributions is to only ship the project and test directories. Totally up to you!
I think we're good to merge here! Thanks everybody for helping test this and for your feedback. Special thanks to @ofek! |
This comment was marked as resolved.
This comment was marked as resolved.
Fixes packaging bug introduced in mne-tools#12269 Addresses the issues raised in: mne-tools/mne-bids-pipeline#825 (comment) mne-tools/mne-bids#1206 (comment) Ultimately, we should start testing the installed package(s), not simply fire up the tests from the root of the checked-out repo.
Fixes packaging bug introduced in mne-tools#12269 Addresses the issues raised in: mne-tools/mne-bids-pipeline#825 (comment) mne-tools/mne-bids#1206 (comment) Ultimately, we should start testing the installed package(s), not simply fire up the tests from the root of the checked-out repo.
Supersedes #12173
Closes #12173
Closes #12169
cc @ofek Let's see how this goes
Editable installs now work as expected.
I built the packages via
python -m build .
both on this branch and on
main
.I diffed the contents of the archives.
Wheel diff
sdist diff
I think this all looks great and removes a few files from the
sdist
that we didn't ever deploy via thewheel
anyway (in other words: this stuff never ended up in user installations).cc @cbrnr @mscheltienne @sappelhoff @agramfort