-
Notifications
You must be signed in to change notification settings - Fork 76
BUG: Deleting a file from an editable install #282
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
Conversation
Anyone up for a review? |
I don't think piling hacks on top of this solution will bring a polished solution. I think the approach in #279 is much more robust. It currently handles python modules (including modules appearing and disappearing during rebuild, except at the top level). I'm working on resource file support, but piecing together what is needed from the documentation of the |
I would rather prefer merging this first, since I don't know when how far #279 is yet, and fixing this will allow for a release with editable wheels support (so pandas can finally adopt meson). I also left a few comments on #279. importlib internals are not something I'm familiar with at all, so if you could clarify a couple things for me there, that'd be great. Hopefully, this PR shouldn't mess up too much of what you're working on, since its a small ~10 LOC patch. |
@@ -882,7 +884,6 @@ def build_commands(self, install_dir: Optional[pathlib.Path] = None) -> Sequence | |||
( | |||
'meson', | |||
'install', | |||
'--only-changed', |
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.
Can we keep this? It's quite important, at least verbose mode is completely unusable without it.
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.
It doesn't do anything anymore as of this PR.
This is because we nuke the install dir before hand ( to get rid of files that shouldn't exist anymore, since they were deleted), so the destination file will never exist.
For reference, see #87 (comment).
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.
It's unfortunate that we have to do this, but the only other way, I think, is to patch meson, and I don't think we should leave editable installs broken in the meantime.
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.
To me, making the verbose mode effectively unusable seems worse than not cleaning up some now-unused old file (the chance of actual breakage is quite small here). So I'm ambivalent about this change.
That said, I don't use editable installs and there's further improvements in the pipeline, so I'll just register as neutral on this one; fine if others want to merge this.
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.
To clarify, this old file is still importable if the directory had an init.py, which can be confusing at the very least, and maybe cause problems(I'm thinking maybe if someone did a star import?).
if self.uninstall_old: | ||
if os.path.exists(self.install_dir): | ||
shutil.rmtree(self.install_dir) |
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 don't like this approach. IMO saving the list of previous installed files and manually deleting the ones that are not installed by Meson anymore would be a better way to deal with this.
I meant to block #87 on this, but forgot.
This PR should block release of meson-python with editable wheels.