Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion mesonpy/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,8 @@ def build_editable(self, directory: Path, verbose: bool = False) -> pathlib.Path
hook_name={_as_python_declaration(hook_module_name)},
project_path={_as_python_declaration(self._source_dir)},
build_path={_as_python_declaration(self._build_dir)},
install_dir={_as_python_declaration(install_path)},
uninstall_old = True,
import_paths={_as_python_declaration(import_paths)},
top_level_modules={_as_python_declaration(self.top_level_modules)},
rebuild_commands={_as_python_declaration(rebuild_commands)},
Expand Down Expand Up @@ -882,7 +884,6 @@ def build_commands(self, install_dir: Optional[pathlib.Path] = None) -> Sequence
(
'meson',
'install',
'--only-changed',
Copy link
Contributor

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.

Copy link
Member Author

@lithomas1 lithomas1 Jan 29, 2023

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).

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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?).

'--destdir',
os.fspath(install_dir or self._install_dir),
*self._meson_args['install'],
Expand Down
12 changes: 12 additions & 0 deletions mesonpy/_editable.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import functools
import importlib.abc
import os
import shutil
import subprocess
import sys
import warnings
Expand Down Expand Up @@ -63,6 +64,8 @@ def __init__(
hook_name: str,
project_path: str,
build_path: str,
install_dir: str,
uninstall_old: bool,
import_paths: List[str],
top_level_modules: List[str],
rebuild_commands: List[List[str]],
Expand All @@ -72,6 +75,8 @@ def __init__(
self._hook_name = hook_name
self._project_path = project_path
self._build_path = build_path
self.install_dir = install_dir
self.uninstall_old = uninstall_old
self._import_paths = import_paths
self._top_level_modules = top_level_modules
self._rebuild_commands = rebuild_commands
Expand Down Expand Up @@ -115,6 +120,9 @@ def _proc(self, command: List[str]) -> None:
@functools.lru_cache(maxsize=1)
def rebuild(self) -> None:
self._debug(f'{{cyan}}{{bold}}+ rebuilding {self._project_path}{{reset}}')
if self.uninstall_old:
if os.path.exists(self.install_dir):
shutil.rmtree(self.install_dir)
Comment on lines +123 to +125
Copy link
Member

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.

for command in self._rebuild_commands:
self._proc(command)
self._debug('{cyan}{bold}+ successfully rebuilt{reset}')
Expand Down Expand Up @@ -148,6 +156,8 @@ def install(
hook_name: str,
project_path: str,
build_path: str,
install_dir: str,
uninstall_old: bool,
import_paths: List[str],
top_level_modules: List[str],
rebuild_commands: List[List[str]],
Expand All @@ -163,6 +173,8 @@ def install(
hook_name,
project_path,
build_path,
install_dir,
uninstall_old,
import_paths,
top_level_modules,
rebuild_commands,
Expand Down
1 change: 1 addition & 0 deletions tests/packages/scipy-like/mypkg/submod/deleted.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
print('Deleted file exists')
33 changes: 33 additions & 0 deletions tests/test_wheel.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ def test_scipy_like(wheel_scipy_like):
f'mypkg/cy_extmod{EXT_SUFFIX}',
'mypkg/submod/__init__.py',
'mypkg/submod/unknown_filetype.npq',
'mypkg/submod/deleted.py'
}
if sys.platform in {'win32', 'cygwin'}:
# Currently Meson is installing .dll.a (import libraries) next
Expand Down Expand Up @@ -278,3 +279,35 @@ def test_editable_broken_non_existent_build_dir(
venv.pip('install', os.path.join(tmp_path, mesonpy.build_editable(tmp_path)))

assert venv.python('-c', 'import plat; print(plat.foo())').strip() == 'bar'


def test_editable_syncs_removed_file(
package_scipy_like,
editable_scipy_like,
venv,
tmp_path,
):
# Test that a deleted file is removed from the install dir
shutil.rmtree(tmp_path) # copytree requires dest not to exist
shutil.copytree(package_scipy_like, tmp_path)
mesonpy_dir = os.path.join(package_scipy_like, '.mesonpy')
if os.path.isdir(mesonpy_dir):
venv.pip('uninstall', '-y', 'mypkg')
shutil.rmtree(mesonpy_dir)

os.chdir(tmp_path)
venv.pip('install', os.path.join(tmp_path, mesonpy.build_editable(tmp_path)))

assert venv.python('-c', 'import mypkg.submod.deleted').strip() == 'Deleted file exists'

# Delete a file and check if it is removed
# (in this case, cannot be imported)
os.remove(os.path.join(tmp_path, 'mypkg/submod/deleted.py'))
assert not os.path.exists(os.path.join(tmp_path, 'mypkg/submod/deleted.py'))

try:
# Use assert to fail the test if the call succeeds
assert not venv.python('-c', 'import mypkg.submod.deleted')
except subprocess.CalledProcessError:
# We're expecting this to fail since the file got deleted
pass