From 7d0ceb0f6a2eeeeb8728720fd0aabb23315c5ec3 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Sat, 29 Apr 2023 13:32:52 -0400 Subject: [PATCH 1/9] Call super() in stop(), and eat ProcessLookupError in kill() Should help some with issues like #910 --- invoke/runners.py | 8 +++++++- sites/www/changelog.rst | 3 +++ tests/runners.py | 20 +++++++++++++++----- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/invoke/runners.py b/invoke/runners.py index adde96af..2735b478 100644 --- a/invoke/runners.py +++ b/invoke/runners.py @@ -1289,7 +1289,12 @@ def start(self, command, shell, env): def kill(self): pid = self.pid if self.using_pty else self.process.pid - os.kill(pid, signal.SIGKILL) + try: + os.kill(pid, signal.SIGKILL) + except ProcessLookupError: + # In odd situations where our subprocess is already dead, don't + # throw this upwards. + pass @property def process_is_finished(self): @@ -1328,6 +1333,7 @@ def returncode(self): return self.process.returncode def stop(self): + super().stop() # If we opened a PTY for child communications, make sure to close() it, # otherwise long-running Invoke-using processes exhaust their file # descriptors eventually. diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 31dc4205..4b540c46 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -2,6 +2,9 @@ Changelog ========= +- :bug:`910` Add more rigor around subprocess/runner shutdown to avoid spurious + exceptions & also fix downstream issues in libraries like Fabric. Reported by + Orlando Rodríguez. - :support:`901 backported` (via :issue:`903`) Tweak test suite ``setup`` methods to be named ``setup_method`` so pytest stops whining about it. Patch via Jesse P. Johnson. diff --git a/tests/runners.py b/tests/runners.py index eed840d3..94c63d8b 100644 --- a/tests/runners.py +++ b/tests/runners.py @@ -1402,11 +1402,6 @@ def run_always_stops_timer(self): runner.run(_) runner._timer.cancel.assert_called_once_with() - def stop_cancels_timer(self): - runner = self._mocked_timer() - runner.stop() - runner._timer.cancel.assert_called_once_with() - def timer_aliveness_is_test_of_timing_out(self): # Might be redundant, but easy enough to unit test runner = Runner(Context()) @@ -1430,6 +1425,12 @@ def always_runs_no_matter_what(self): runner.run(_) runner.stop.assert_called_once_with() + def cancels_timer(self): + runner = self._runner() + runner._timer = Mock() + runner.stop() + runner._timer.cancel.assert_called_once_with() + class asynchronous: def returns_Promise_immediately_and_finishes_on_join(self): # Dummy subclass with controllable process_is_finished flag @@ -1534,6 +1535,15 @@ def _run(self, *args, **kwargs): def _runner(self, *args, **kwargs): return _runner(*args, **dict(kwargs, klass=_FastLocal)) + class stop: + @mock_subprocess() + def calls_super(self): + # Re #910 + runner = self._runner() + runner._timer = Mock() # twiddled by parent class stop() + runner.run(_) + runner._timer.cancel.assert_called_once_with() + class pty: @mock_pty() def when_pty_True_we_use_pty_fork_and_os_exec(self): From 5658046df4c9cef7be121b902cbedec6e425bc34 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Sat, 29 Apr 2023 14:07:09 -0400 Subject: [PATCH 2/9] Cut 2.0.1 --- invoke/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/invoke/_version.py b/invoke/_version.py index c359da63..f55a4f18 100644 --- a/invoke/_version.py +++ b/invoke/_version.py @@ -1,2 +1,2 @@ -__version_info__ = (2, 0, 0) +__version_info__ = (2, 0, 1) __version__ = ".".join(map(str, __version_info__)) From 03060dea521c8c5b384a38987ec0cc0aced9743e Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Sat, 29 Apr 2023 14:10:06 -0400 Subject: [PATCH 3/9] 2.0.1 changelog Not sure what happened there, oops. --- sites/www/changelog.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 4b540c46..00292927 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -2,6 +2,7 @@ Changelog ========= +- :release:`2.0.1 <2023-04-29>` - :bug:`910` Add more rigor around subprocess/runner shutdown to avoid spurious exceptions & also fix downstream issues in libraries like Fabric. Reported by Orlando Rodríguez. From 13de77f96ec96ce6827d7d001aa814e6f195e017 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Sat, 29 Apr 2023 14:57:45 -0400 Subject: [PATCH 4/9] Add integration test proving #934 regression tests OK under 2.0 --- integration/_support/package/tasks/__init__.py | 7 +++++++ integration/_support/package/tasks/module.py | 6 ++++++ integration/main.py | 6 ++++++ 3 files changed, 19 insertions(+) create mode 100644 integration/_support/package/tasks/__init__.py create mode 100644 integration/_support/package/tasks/module.py diff --git a/integration/_support/package/tasks/__init__.py b/integration/_support/package/tasks/__init__.py new file mode 100644 index 00000000..40ba1e01 --- /dev/null +++ b/integration/_support/package/tasks/__init__.py @@ -0,0 +1,7 @@ +from invoke import Collection + +# Issue #934 (from #919) only seems to trigger on this style of 'from . import +# xxx' - a vanilla self-contained tasks/__init__.py is still fine! +from . import module + +ns = Collection(module) diff --git a/integration/_support/package/tasks/module.py b/integration/_support/package/tasks/module.py new file mode 100644 index 00000000..05f37ee5 --- /dev/null +++ b/integration/_support/package/tasks/module.py @@ -0,0 +1,6 @@ +from invoke import task + + +@task +def mytask(c): + print("hi!") diff --git a/integration/main.py b/integration/main.py index 7a86a80f..162c0fbf 100644 --- a/integration/main.py +++ b/integration/main.py @@ -65,6 +65,12 @@ def bad_collection_exits_nonzero(self): assert not result.stdout assert result.stderr + @trap + def package_style_collections_internally_importable(self): + # After merging #919 blew this up and unit tests did not detect! + result = run("cd _support/package && inv -l") + assert "mytask" in result.stdout + def loads_real_user_config(self): path = os.path.expanduser("~/.invoke.yaml") try: From 1b4b70e213735cade3bda3f5831352cb6653928f Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Sat, 29 Apr 2023 15:31:35 -0400 Subject: [PATCH 5/9] new test twiddle for 2.1 version integration tests --- integration/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/main.py b/integration/main.py index 10ff99cb..0ad801b5 100644 --- a/integration/main.py +++ b/integration/main.py @@ -64,7 +64,7 @@ def bad_collection_exits_nonzero(self): @trap def package_style_collections_internally_importable(self): # After merging #919 blew this up and unit tests did not detect! - result = run("cd _support/package && inv -l") + result = run("cd package && inv -l") assert "mytask" in result.stdout def loads_real_user_config(self): From 9b68e3088ab643810ef0ded6b79f151a4389db0d Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 1 May 2023 21:13:06 -0400 Subject: [PATCH 6/9] Fix #934 by adding sys.modules twiddling to Loader --- invoke/loader.py | 1 + 1 file changed, 1 insertion(+) diff --git a/invoke/loader.py b/invoke/loader.py index bed47a4b..ba3003dd 100644 --- a/invoke/loader.py +++ b/invoke/loader.py @@ -77,6 +77,7 @@ def load(self, name: Optional[str] = None) -> Tuple[ModuleType, str]: sys.path.insert(0, path) # Actual import module = module_from_spec(spec) + sys.modules[spec.name] = module # so 'from . import xxx' works spec.loader.exec_module(module) return module, os.path.dirname(spec.origin) msg = "ImportError loading {!r}, raising ImportError" From d6c58d2036fbb0101654e81bbc8a673995ebd029 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Sat, 29 Apr 2023 14:57:45 -0400 Subject: [PATCH 7/9] Add tests proving #934 regression is OK under 2.0 --- integration/_support/package/tasks/__init__.py | 7 +++++++ integration/_support/package/tasks/module.py | 6 ++++++ integration/main.py | 6 ++++++ tests/_support/namespacing.py | 2 +- tests/_support/package/__init__.py | 5 +++++ tests/_support/subspace/__init__.py | 5 +++++ tests/_support/subspace/module.py | 6 ++++++ tests/loader.py | 10 +++++++--- 8 files changed, 43 insertions(+), 4 deletions(-) create mode 100644 integration/_support/package/tasks/__init__.py create mode 100644 integration/_support/package/tasks/module.py create mode 100644 tests/_support/subspace/__init__.py create mode 100644 tests/_support/subspace/module.py diff --git a/integration/_support/package/tasks/__init__.py b/integration/_support/package/tasks/__init__.py new file mode 100644 index 00000000..40ba1e01 --- /dev/null +++ b/integration/_support/package/tasks/__init__.py @@ -0,0 +1,7 @@ +from invoke import Collection + +# Issue #934 (from #919) only seems to trigger on this style of 'from . import +# xxx' - a vanilla self-contained tasks/__init__.py is still fine! +from . import module + +ns = Collection(module) diff --git a/integration/_support/package/tasks/module.py b/integration/_support/package/tasks/module.py new file mode 100644 index 00000000..05f37ee5 --- /dev/null +++ b/integration/_support/package/tasks/module.py @@ -0,0 +1,6 @@ +from invoke import task + + +@task +def mytask(c): + print("hi!") diff --git a/integration/main.py b/integration/main.py index 7a86a80f..162c0fbf 100644 --- a/integration/main.py +++ b/integration/main.py @@ -65,6 +65,12 @@ def bad_collection_exits_nonzero(self): assert not result.stdout assert result.stderr + @trap + def package_style_collections_internally_importable(self): + # After merging #919 blew this up and unit tests did not detect! + result = run("cd _support/package && inv -l") + assert "mytask" in result.stdout + def loads_real_user_config(self): path = os.path.expanduser("~/.invoke.yaml") try: diff --git a/tests/_support/namespacing.py b/tests/_support/namespacing.py index bb9408b8..f2ab7768 100644 --- a/tests/_support/namespacing.py +++ b/tests/_support/namespacing.py @@ -1,6 +1,6 @@ from invoke import Collection, task, call -from package import module +from subspace import module @task diff --git a/tests/_support/package/__init__.py b/tests/_support/package/__init__.py index e69de29b..eb57056f 100644 --- a/tests/_support/package/__init__.py +++ b/tests/_support/package/__init__.py @@ -0,0 +1,5 @@ +from invoke import Collection + +from . import module + +ns = Collection(module) diff --git a/tests/_support/subspace/__init__.py b/tests/_support/subspace/__init__.py new file mode 100644 index 00000000..eb57056f --- /dev/null +++ b/tests/_support/subspace/__init__.py @@ -0,0 +1,5 @@ +from invoke import Collection + +from . import module + +ns = Collection(module) diff --git a/tests/_support/subspace/module.py b/tests/_support/subspace/module.py new file mode 100644 index 00000000..17a77c68 --- /dev/null +++ b/tests/_support/subspace/module.py @@ -0,0 +1,6 @@ +from invoke import task + + +@task +def mytask(c): + pass diff --git a/tests/loader.py b/tests/loader.py index 9d6065c7..acef3fc9 100644 --- a/tests/loader.py +++ b/tests/loader.py @@ -2,6 +2,7 @@ import os import sys import types +from pathlib import Path from pytest import raises @@ -45,7 +46,7 @@ def adds_module_parent_dir_to_sys_path(self): # Crummy doesn't-explode test. _BasicLoader().load("namespacing") - def doesnt_dupliate_parent_dir_addition(self): + def doesnt_duplicate_parent_dir_addition(self): _BasicLoader().load("namespacing") _BasicLoader().load("namespacing") # If the bug is present, this will be 2 at least (and often more, since @@ -59,8 +60,11 @@ def closes_opened_file_object(self): def can_load_package(self): loader = _BasicLoader() - # make sure it doesn't explode - loader.load("package") + # Load itself doesn't explode (tests 'from . import xxx' internally) + mod, loc = loader.load("package") + # Properties of returned values look as expected + assert loc == support + assert mod.__file__ == str(Path(support) / "package" / "__init__.py") def load_name_defaults_to_config_tasks_collection_name(self): "load() name defaults to config.tasks.collection_name" From 609db6d151e2ddb1270ac4957bf54823eab04376 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 1 May 2023 22:09:43 -0400 Subject: [PATCH 8/9] Changelog closes #934 --- sites/www/changelog.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 7615a666..68bbc162 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -2,6 +2,10 @@ Changelog ========= +- :bug:`934` The `importlib` upgrade in 2.1 had a corner case bug (regarding + ``from . import `` functionality within package-like task trees) + which in turn exposed a false-pass in our test suite. Both have now been + fixed. Thanks to Greg Meyer and Robert J. Berger for the bug reports. - :release:`2.0.1 <2023-04-29>` - :bug:`910` Add more rigor around subprocess/runner shutdown to avoid spurious exceptions & also fix downstream issues in libraries like Fabric. Reported by From 0fa82cf83e97b0dbbc3ee4fbc3228bf4e206d09f Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 1 May 2023 22:11:05 -0400 Subject: [PATCH 9/9] Cut 2.1.1 --- invoke/_version.py | 2 +- sites/www/changelog.rst | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/invoke/_version.py b/invoke/_version.py index bb08fdbb..572fcf1e 100644 --- a/invoke/_version.py +++ b/invoke/_version.py @@ -1,2 +1,2 @@ -__version_info__ = (2, 1, 0) +__version_info__ = (2, 1, 1) __version__ = ".".join(map(str, __version_info__)) diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 68bbc162..1df2ec5c 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -2,6 +2,7 @@ Changelog ========= +- :release:`2.1.1 <2023-05-01>` - :bug:`934` The `importlib` upgrade in 2.1 had a corner case bug (regarding ``from . import `` functionality within package-like task trees) which in turn exposed a false-pass in our test suite. Both have now been