From b6d308b35aea04e622f21098d15845438915ec63 Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Wed, 13 Nov 2024 00:30:39 -0500 Subject: [PATCH 01/19] Make default stderr sys.stderr, not passthrough --- choreographer/browser.py | 2 +- choreographer/chrome_wrapper.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/choreographer/browser.py b/choreographer/browser.py index 5be0e68..12064bd 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -71,7 +71,7 @@ def __init__( if not debug_browser: # false o None stderr = subprocess.DEVNULL elif debug_browser is True: - stderr = None + stderr = sys.stderr else: stderr = debug_browser self._stderr = stderr diff --git a/choreographer/chrome_wrapper.py b/choreographer/chrome_wrapper.py index 4d09e50..25e5e8f 100644 --- a/choreographer/chrome_wrapper.py +++ b/choreographer/chrome_wrapper.py @@ -3,7 +3,6 @@ # importing modules has side effects, so we do this before imports # chromium reads on 3, writes on 4 # linter complains. -env = None # really this means windows only, but a more readable platform.system() # needs to come later for the above reasons @@ -18,6 +17,7 @@ import signal # noqa import platform # noqa import asyncio #noqa +import sys #noqa system = platform.system() if system == "Windows": @@ -26,7 +26,7 @@ os.set_inheritable(4, True) os.set_inheritable(3, True) -def open_browser(to_chromium, from_chromium, stderr=None, env=None, loop=None, loop_hack=False): +def open_browser(to_chromium, from_chromium, stderr=sys.stderr, env=None, loop=None, loop_hack=False): path = env.get("BROWSER_PATH") if not path: raise RuntimeError("No browser path was passed to run") From e41edce73fe58b9e23e8c7c0851179ebd05244ae Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Wed, 13 Nov 2024 00:40:25 -0500 Subject: [PATCH 02/19] Readd random env = None --- choreographer/chrome_wrapper.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/choreographer/chrome_wrapper.py b/choreographer/chrome_wrapper.py index 25e5e8f..1209595 100644 --- a/choreographer/chrome_wrapper.py +++ b/choreographer/chrome_wrapper.py @@ -1,18 +1,17 @@ import os # importing modules has side effects, so we do this before imports -# chromium reads on 3, writes on 4 # linter complains. +env = None + +# chromium reads on 3, writes on 4 # really this means windows only, but a more readable platform.system() -# needs to come later for the above reasons -# windows will import, not run as separate process if __name__ == "__main__": os.dup2(0, 3) # make our stdin their input os.dup2(1, 4) # make our stdout their output - import subprocess # noqa import signal # noqa import platform # noqa From 2153947bac9402b023db4e38c6c52a5f0efe78ab Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Wed, 13 Nov 2024 16:07:10 -0500 Subject: [PATCH 03/19] Add notice about Popen-stderr compatability --- choreographer/browser.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/choreographer/browser.py b/choreographer/browser.py index 12064bd..96e6a89 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -2,6 +2,7 @@ import os from pathlib import Path import sys +import io import subprocess import time import tempfile @@ -74,7 +75,17 @@ def __init__( stderr = sys.stderr else: stderr = debug_browser + + # awful + if stderr and stderr != subprocess.PIPE and stderr != subprocess.STDOUT and stderr != subprocess.DEVNULL and not isinstance(stderr, int): + try: stderr.fileno() + except io.UnsupportedOperation: + warnings.warn("A value has been passed to debug_browser which is not compatible with python. The default value if deug_browser is True is whatever the value of sys.stderr is. sys.stderr may be many things but debug_browser must be a value Popen accepts for stderr, or True.") + + + self._stderr = stderr + if debug: print(f"STDERR: {stderr}", file=sys.stderr) From 4b73896d6b4f7ce57c3af5f7422bc9f95dd15df8 Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Wed, 13 Nov 2024 16:07:53 -0500 Subject: [PATCH 04/19] Narrow default_browser default to False --- choreographer/browser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/choreographer/browser.py b/choreographer/browser.py index 96e6a89..3070a63 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -56,7 +56,7 @@ def __init__( loop=None, executor=None, debug=False, - debug_browser=None, + debug_browser=False, **kwargs ): # Configuration @@ -69,7 +69,7 @@ def __init__( self.loop_hack = False # subprocess needs weird stuff w/ SelectorEventLoop # Set up stderr - if not debug_browser: # false o None + if debug_browser is False: # false o None stderr = subprocess.DEVNULL elif debug_browser is True: stderr = sys.stderr From 6ecb6a192b6a4c8a09d1ddb9112a7ebbc423a189 Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Wed, 13 Nov 2024 16:11:27 -0500 Subject: [PATCH 05/19] Fix tests to override default stderr --- tests/conftest.py | 5 +++-- tests/test_process.py | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 11449f4..c72189c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -44,8 +44,9 @@ async def browser(request): # this needs also to be set by command line TODO headless = request.config.getoption("--headless") debug = request.config.get_verbosity() > 2 + debug_browser = None if debug else False browser = await choreo.Browser( - headless=headless, debug=debug, debug_browser=debug + headless=headless, debug=debug, debug_browser=debug_browser ) yield browser try: @@ -57,7 +58,7 @@ async def browser(request): @pytest_asyncio.fixture(scope="function", loop_scope="function") async def browser_verbose(): - browser = await choreo.Browser(debug=True, debug_browser=True) + browser = await choreo.Browser(debug=True, debug_browser=None) yield browser await browser.close() diff --git a/tests/test_process.py b/tests/test_process.py index 16a6ed6..66540e3 100644 --- a/tests/test_process.py +++ b/tests/test_process.py @@ -14,7 +14,7 @@ async def test_context(capteesys, headless, debug, debug_browser, sandbox, gpu): async with choreo.Browser( headless=headless, debug=debug, - debug_browser=debug_browser, + debug_browser=None if debug_browser else False, enable_sandbox=sandbox, enable_gpu=gpu ) as browser, timeout(pytest.default_timeout): @@ -35,7 +35,7 @@ async def test_no_context(capteesys, headless, debug, debug_browser, sandbox, gp browser = await choreo.Browser( headless=headless, debug=debug, - debug_browser=debug_browser, + debug_browser=None if debug_browser else False, enable_sandbox=sandbox, enable_gpu=gpu ) @@ -59,7 +59,7 @@ async def test_watchdog(capteesys, headless, debug, debug_browser): browser = await choreo.Browser( headless=headless, debug=debug, - debug_browser=debug_browser, + debug_browser=None if debug_browser else False, ) #async with timeout(pytest.default_timeout): From 3468d1a4213130645ed22c10746c6e5e7c000d69 Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 10:57:45 -0500 Subject: [PATCH 06/19] Add blank lines to provoke CI tests --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 74c1460..2b62421 100644 --- a/README.md +++ b/README.md @@ -148,3 +148,5 @@ We will document these as the API stabilizes. [devtools-ref]: https://chromedevtools.github.io/devtools-protocol/ [kaleido]: https://pypi.org/project/kaleido/ [puppeteer]: https://pptr.dev/ + + From 5d127780b11ec12a8fac5d42f4692d599817016d Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 11:06:51 -0500 Subject: [PATCH 07/19] Check in tests for tempdir deletion --- tests/conftest.py | 7 +++++++ tests/test_process.py | 13 ++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 11449f4..0fe6c68 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,5 @@ import asyncio +import os import pytest import pytest_asyncio @@ -47,19 +48,25 @@ async def browser(request): browser = await choreo.Browser( headless=headless, debug=debug, debug_browser=debug ) + temp_dir = browser.temp_dir.name yield browser try: await browser.close() except choreo.browser.BrowserClosedError: pass + if os.path.exists(temp_dir): + raise RuntimeError(f"Temporary directory not deleted successfully: {temp_dir}") @pytest_asyncio.fixture(scope="function", loop_scope="function") async def browser_verbose(): browser = await choreo.Browser(debug=True, debug_browser=True) + temp_dir = browser.temp_dir.name yield browser await browser.close() + if os.path.exists(temp_dir): + raise RuntimeError(f"Temporary directory not deleted successfully: {temp_dir}") # add a 2 second timeout if there is a browser fixture # we do timeouts manually in test_process because it diff --git a/tests/test_process.py b/tests/test_process.py index 16a6ed6..9383eab 100644 --- a/tests/test_process.py +++ b/tests/test_process.py @@ -18,6 +18,7 @@ async def test_context(capteesys, headless, debug, debug_browser, sandbox, gpu): enable_sandbox=sandbox, enable_gpu=gpu ) as browser, timeout(pytest.default_timeout): + temp_dir = browser.temp_dir.name response = await browser.send_command(command="Target.getTargets") assert "result" in response and "targetInfos" in response["result"] assert (len(response["result"]["targetInfos"]) != 0) != headless @@ -29,6 +30,7 @@ async def test_context(capteesys, headless, debug, debug_browser, sandbox, gpu): assert capteesys.readouterr().out == "\n", "stdout should be silent!" # let asyncio do some cleaning up if it wants, may prevent warnings await asyncio.sleep(0) + assert not os.path.exists(temp_dir) @pytest.mark.asyncio(loop_scope="function") async def test_no_context(capteesys, headless, debug, debug_browser, sandbox, gpu): @@ -39,6 +41,7 @@ async def test_no_context(capteesys, headless, debug, debug_browser, sandbox, gp enable_sandbox=sandbox, enable_gpu=gpu ) + temp_dir = browser.temp_dir.name try: async with timeout(pytest.default_timeout): response = await browser.send_command(command="Target.getTargets") @@ -49,9 +52,10 @@ async def test_no_context(capteesys, headless, debug, debug_browser, sandbox, gp assert len(browser.get_tab().sessions) == 1 finally: await browser.close() - print("") # this make sure that capturing is working - assert capteesys.readouterr().out == "\n", "stdout should be silent!" - await asyncio.sleep(0) + print("") # this make sure that capturing is working + assert capteesys.readouterr().out == "\n", "stdout should be silent!" + await asyncio.sleep(0) + assert not os.path.exists(temp_dir) # we're basically just harrassing choreographer with a kill in this test to see if it behaves well @pytest.mark.asyncio(loop_scope="function") @@ -61,6 +65,7 @@ async def test_watchdog(capteesys, headless, debug, debug_browser): debug=debug, debug_browser=debug_browser, ) + temp_dir = browser.temp_dir.name #async with timeout(pytest.default_timeout): if platform.system() == "Windows": @@ -77,3 +82,5 @@ async def test_watchdog(capteesys, headless, debug, debug_browser): # interestingly, try/except doesn't work here? maybe problem with pytest await browser.close() + await asyncio.sleep(0) + assert not os.path.exists(temp_dir) From 8faff8279c63dd136bace2b6832f92a5c6d3b763 Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 11:21:00 -0500 Subject: [PATCH 08/19] Add manual delete to watchdog --- choreographer/browser.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/choreographer/browser.py b/choreographer/browser.py index 5be0e68..feff77d 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -203,11 +203,17 @@ def _open(self): async def _watchdog(self): + self._watchdog_healthy = True if self.debug: print("Starting watchdog", file=sys.stderr) await self.subprocess.wait() + if self.lock.locked(): return # it was locked and closed + self._watchdog_healthy = False if self.debug: print("Browser is being closed because chrom* closed", file=sys.stderr) await self.close() + await asyncio.sleep(1) + self._retry_delete_manual(self.temp_dir.name, delete=True) + async def _open_async(self): From 9e2b4fb4c8a8af303f053f3d0f7aa83f9dd98740 Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 11:21:13 -0500 Subject: [PATCH 09/19] Add manual tempdir deletes to all platforms: Targeted windows before but now mac is acting up, so just do it everywhere --- choreographer/browser.py | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/choreographer/browser.py b/choreographer/browser.py index feff77d..a5e6408 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -287,16 +287,14 @@ def _clean_temp(self): name = self.temp_dir.name clean = False try: + # no faith in this python implementation, always fails with windows + # very unstable recently as well, lots new arguments in tempfile package self.temp_dir.cleanup() clean=True except BaseException as e: - if platform.system() == "Windows": - if self.debug: - print(f"TempDirWarning: {str(e)}", file=sys.stderr) - else: - warnings.warn(str(e), TempDirWarning) + if self.debug: + print(f"First tempdir deletion failed: TempDirWarning: {str(e)}", file=sys.stderr) - # windows+old vers doesn't like python's default cleanup def remove_readonly(func, path, excinfo): os.chmod(path, stat.S_IWUSR) @@ -313,17 +311,14 @@ def remove_readonly(func, path, excinfo): except FileNotFoundError: pass # it worked! except BaseException as e: + if self.debug: + print(f"Second tmpdir deletion failed (shutil.rmtree): {str(e)}", file=sys.stderr) if not clean: - if platform.system() == "Windows": - def extra_clean(): - time.sleep(5) - self._retry_delete_manual(name, delete=True) - t = Thread(target=extra_clean) - t.run() - else: - warnings.warn( - f"The temporary directory could not be deleted, execution will continue. {type(e)}: {e}", TempDirWarning - ) + def extra_clean(): + time.sleep(2) + self._retry_delete_manual(name, delete=True) + t = Thread(target=extra_clean) + t.run() if self.debug: print(f"Tempfile still exists?: {bool(os.path.exists(str(name)))}", file=sys.stderr) From 8280aef140187b71b66b0aab6875f1075c7f0b0a Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 11:37:39 -0500 Subject: [PATCH 10/19] Store temporary directory name separately: This allows to check for deletion even after python object is `del`ed --- choreographer/browser.py | 13 +++++++------ tests/conftest.py | 4 ++-- tests/test_process.py | 6 +++--- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/choreographer/browser.py b/choreographer/browser.py index a5e6408..26c248e 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -97,6 +97,7 @@ def __init__( ) else: self.temp_dir = tempfile.TemporaryDirectory(**temp_args) + self._temp_dir_name = self.temp_dir.name # Set up process env new_env = os.environ.copy() @@ -117,7 +118,7 @@ def __init__( if self.enable_sandbox: new_env["SANDBOX_ENABLED"] = "true" - new_env["USER_DATA_DIR"] = str(self.temp_dir.name) + new_env["USER_DATA_DIR"] = str(self._temp_dir_name) if headless: new_env["HEADLESS"] = "--headless" # unset if false @@ -211,8 +212,8 @@ async def _watchdog(self): if self.debug: print("Browser is being closed because chrom* closed", file=sys.stderr) await self.close() - await asyncio.sleep(1) - self._retry_delete_manual(self.temp_dir.name, delete=True) + await asyncio.sleep(.5) + self._retry_delete_manual(self._temp_dir_name, delete=True) @@ -284,7 +285,7 @@ def _retry_delete_manual(self, path, delete=False): return n_dirs, n_files, errors def _clean_temp(self): - name = self.temp_dir.name + name = self._temp_dir_name clean = False try: # no faith in this python implementation, always fails with windows @@ -302,10 +303,10 @@ def remove_readonly(func, path, excinfo): try: if with_onexc: - shutil.rmtree(self.temp_dir.name, onexc=remove_readonly) + shutil.rmtree(self._temp_dir_name, onexc=remove_readonly) clean=True else: - shutil.rmtree(self.temp_dir.name, onerror=remove_readonly) + shutil.rmtree(self._temp_dir_name, onerror=remove_readonly) clean=True del self.temp_dir except FileNotFoundError: diff --git a/tests/conftest.py b/tests/conftest.py index 0fe6c68..790d994 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -48,7 +48,7 @@ async def browser(request): browser = await choreo.Browser( headless=headless, debug=debug, debug_browser=debug ) - temp_dir = browser.temp_dir.name + temp_dir = browser._temp_dir_name yield browser try: await browser.close() @@ -62,7 +62,7 @@ async def browser(request): @pytest_asyncio.fixture(scope="function", loop_scope="function") async def browser_verbose(): browser = await choreo.Browser(debug=True, debug_browser=True) - temp_dir = browser.temp_dir.name + temp_dir = browser._temp_dir_name yield browser await browser.close() if os.path.exists(temp_dir): diff --git a/tests/test_process.py b/tests/test_process.py index 9383eab..4e20d02 100644 --- a/tests/test_process.py +++ b/tests/test_process.py @@ -18,7 +18,7 @@ async def test_context(capteesys, headless, debug, debug_browser, sandbox, gpu): enable_sandbox=sandbox, enable_gpu=gpu ) as browser, timeout(pytest.default_timeout): - temp_dir = browser.temp_dir.name + temp_dir = browser._temp_dir_name response = await browser.send_command(command="Target.getTargets") assert "result" in response and "targetInfos" in response["result"] assert (len(response["result"]["targetInfos"]) != 0) != headless @@ -41,7 +41,7 @@ async def test_no_context(capteesys, headless, debug, debug_browser, sandbox, gp enable_sandbox=sandbox, enable_gpu=gpu ) - temp_dir = browser.temp_dir.name + temp_dir = browser._temp_dir_name try: async with timeout(pytest.default_timeout): response = await browser.send_command(command="Target.getTargets") @@ -65,7 +65,7 @@ async def test_watchdog(capteesys, headless, debug, debug_browser): debug=debug, debug_browser=debug_browser, ) - temp_dir = browser.temp_dir.name + temp_dir = browser._temp_dir_name #async with timeout(pytest.default_timeout): if platform.system() == "Windows": From 2cef2c274b496b109fa83ec2d6cf44674baeaf0d Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 11:47:59 -0500 Subject: [PATCH 11/19] Tweak timing and debug on delete tmpdir --- choreographer/browser.py | 6 +++++- tests/test_process.py | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/choreographer/browser.py b/choreographer/browser.py index 26c248e..23a8220 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -212,7 +212,7 @@ async def _watchdog(self): if self.debug: print("Browser is being closed because chrom* closed", file=sys.stderr) await self.close() - await asyncio.sleep(.5) + await asyncio.sleep(.75) self._retry_delete_manual(self._temp_dir_name, delete=True) @@ -258,6 +258,8 @@ def _retry_delete_manual(self, path, delete=False): n_files += len(files) if delete: for f in files: + if self.debug: + print(f"Removing file: {f}", file=sys.stderr) fp = os.path.join(root, f) try: os.chmod(fp, stat.S_IWUSR) @@ -265,6 +267,8 @@ def _retry_delete_manual(self, path, delete=False): except BaseException as e: errors.append((fp, e)) for d in dirs: + if self.debug: + print(f"Removing dir: {d}", file=sys.stderr) fp = os.path.join(root, d) try: os.chmod(fp, stat.S_IWUSR) diff --git a/tests/test_process.py b/tests/test_process.py index 4e20d02..0ee090c 100644 --- a/tests/test_process.py +++ b/tests/test_process.py @@ -76,7 +76,7 @@ async def test_watchdog(capteesys, headless, debug, debug_browser): ) else: os.kill(browser.subprocess.pid, signal.SIGKILL) - await asyncio.sleep(1) + await asyncio.sleep(1.5) with pytest.raises((choreo.browser.PipeClosedError, choreo.browser.BrowserClosedError)): await browser.send_command(command="Target.getTargets") # could check for error, for close # interestingly, try/except doesn't work here? maybe problem with pytest From 904bdb734b7e61f58ca6102fd9ab4b6910466108 Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 11:58:33 -0500 Subject: [PATCH 12/19] Improve debugging --- choreographer/browser.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/choreographer/browser.py b/choreographer/browser.py index 23a8220..531635e 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -98,6 +98,8 @@ def __init__( else: self.temp_dir = tempfile.TemporaryDirectory(**temp_args) self._temp_dir_name = self.temp_dir.name + if self.debug: + print(f"TEMP DIR NAME: {self.temp_dir_name}", file=sys.stderr) # Set up process env new_env = os.environ.copy() @@ -249,7 +251,9 @@ async def _open_async(self): def _retry_delete_manual(self, path, delete=False): if not os.path.exists(path): - return 0, 0, [(path, FileNotFoundError("Supplied path doesn't exist"))] + if self.debug: + print("No retry delete manual necessary, path doesn't exist", file=sys.stderr) + return 0, 0, [] n_dirs = 0 n_files = 0 errors = [] @@ -258,21 +262,23 @@ def _retry_delete_manual(self, path, delete=False): n_files += len(files) if delete: for f in files: - if self.debug: - print(f"Removing file: {f}", file=sys.stderr) fp = os.path.join(root, f) + if self.debug: + print(f"Removing file: {fp}", file=sys.stderr) try: os.chmod(fp, stat.S_IWUSR) os.remove(fp) + if self.debug: print("Success", file=sys.stderr) except BaseException as e: errors.append((fp, e)) for d in dirs: - if self.debug: - print(f"Removing dir: {d}", file=sys.stderr) fp = os.path.join(root, d) + if self.debug: + print(f"Removing dir: {fp}", file=sys.stderr) try: os.chmod(fp, stat.S_IWUSR) os.rmdir(fp) + if self.debug: print("Success", file=sys.stderr) except BaseException as e: errors.append((fp, e)) # clean up directory From af00e68a43fd4564c59776ecaf0925b7a202fabd Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 12:01:44 -0500 Subject: [PATCH 13/19] Fix bad variable name --- choreographer/browser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/choreographer/browser.py b/choreographer/browser.py index 531635e..a43abd4 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -99,7 +99,7 @@ def __init__( self.temp_dir = tempfile.TemporaryDirectory(**temp_args) self._temp_dir_name = self.temp_dir.name if self.debug: - print(f"TEMP DIR NAME: {self.temp_dir_name}", file=sys.stderr) + print(f"TEMP DIR NAME: {self._temp_dir_name}", file=sys.stderr) # Set up process env new_env = os.environ.copy() From 285a7e3c58391a840569db18e195875b8d2c841c Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 12:06:45 -0500 Subject: [PATCH 14/19] Silence warnings on watchdog --- choreographer/browser.py | 9 +++++++-- tests/test_process.py | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/choreographer/browser.py b/choreographer/browser.py index a43abd4..c5e6d70 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -214,8 +214,13 @@ async def _watchdog(self): if self.debug: print("Browser is being closed because chrom* closed", file=sys.stderr) await self.close() - await asyncio.sleep(.75) - self._retry_delete_manual(self._temp_dir_name, delete=True) + await asyncio.sleep(1) + with warnings.catch_warnings(): + # we'll ignore warnings here because + # if the user sloppy-closes the browsers + # they may leave processes up still trying to create temporary files + warnings.filterwarnings("ignore", category=TempDirWarning) + self._retry_delete_manual(self._temp_dir_name, delete=True) diff --git a/tests/test_process.py b/tests/test_process.py index 0ee090c..589208b 100644 --- a/tests/test_process.py +++ b/tests/test_process.py @@ -65,7 +65,7 @@ async def test_watchdog(capteesys, headless, debug, debug_browser): debug=debug, debug_browser=debug_browser, ) - temp_dir = browser._temp_dir_name + #temp_dir = browser._temp_dir_name #async with timeout(pytest.default_timeout): if platform.system() == "Windows": @@ -83,4 +83,4 @@ async def test_watchdog(capteesys, headless, debug, debug_browser): await browser.close() await asyncio.sleep(0) - assert not os.path.exists(temp_dir) + # assert not os.path.exists(temp_dir) # since we slopily kill the browser, we have no idea whats going to happen From 9d43804d74dee8a06a7c3501b37b1140e940c2a6 Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 12:11:00 -0500 Subject: [PATCH 15/19] Increase timeout on mac --- .github/workflows/test.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index e7830b2..a5d0420 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -83,7 +83,7 @@ jobs: - name: Test Process Control if: ${{ ! runner.debug }} run: pytest -W error -n auto -v -rFe --capture=fd tests/test_process.py - timeout-minutes: 3 + timeout-minutes: 4 - name: Test The Rest DEBUG if: runner.debug run: pytest -W error -vvv -rA --capture=tee-sys --ignore=tests/test_process.py @@ -91,5 +91,5 @@ jobs: - name: Test The Rest if: ${{ ! runner.debug }} run: pytest -W error -n auto -v -rfE --ignore=tests/test_process.py - timeout-minutes: 2 + timeout-minutes: 3 From 786439a07e4162b7746c6333f2afbbb51878efa3 Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 12:24:26 -0500 Subject: [PATCH 16/19] Try to detect snap for changing for tmp dir --- choreographer/browser.py | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/choreographer/browser.py b/choreographer/browser.py index c5e6d70..4697b72 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -78,10 +78,27 @@ def __init__( if debug: print(f"STDERR: {stderr}", file=sys.stderr) - # Set up temp dir - if platform.system() == "Linux": + + # Set up process env + new_env = os.environ.copy() + + if not path: # use argument first + path = os.environ.get("BROWSER_PATH", None) + if not path: + path = default_path + if path: + new_env["BROWSER_PATH"] = str(path) + else: + raise BrowserFailedError( + "Could not find an acceptable browser. Please set environmental variable BROWSER_PATH or pass `path=/path/to/browser` into the Browser() constructor." + ) + if path.contains("snap"): + self._snap = True + if self.debug: + print("Snap detected, moving tmp directory to home", file=sys.stderr) temp_args = dict(prefix=".choreographer-", dir=Path.home()) else: + self._snap = False temp_args = {} if platform.system() != "Windows": self.temp_dir = tempfile.TemporaryDirectory(**temp_args) @@ -101,20 +118,6 @@ def __init__( if self.debug: print(f"TEMP DIR NAME: {self._temp_dir_name}", file=sys.stderr) - # Set up process env - new_env = os.environ.copy() - - if not path: # use argument first - path = os.environ.get("BROWSER_PATH", None) - if not path: - path = default_path - if path: - new_env["BROWSER_PATH"] = str(path) - else: - raise BrowserFailedError( - "Could not find an acceptable browser. Please set environmental variable BROWSER_PATH or pass `path=/path/to/browser` into the Browser() constructor." - ) - if self.enable_gpu: new_env["GPU_ENABLED"] = "true" if self.enable_sandbox: From 0411028a88e49e4b8e7f14d2b6f64fbc38d1bc9a Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 12:27:02 -0500 Subject: [PATCH 17/19] Add override for temp directory path --- choreographer/browser.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/choreographer/browser.py b/choreographer/browser.py index 4697b72..b2c92a0 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -61,6 +61,7 @@ def __init__( # Configuration self.enable_gpu = kwargs.pop("enable_gpu", False) self.enable_sandbox = kwargs.pop("enable_sandbox", False) + self._tmpdir_path = kwargs.pop("tmp_path", None) if len(kwargs): raise ValueError(f"Unknown keyword arguments: {kwargs}") self.headless = headless @@ -92,8 +93,11 @@ def __init__( raise BrowserFailedError( "Could not find an acceptable browser. Please set environmental variable BROWSER_PATH or pass `path=/path/to/browser` into the Browser() constructor." ) - if path.contains("snap"): - self._snap = True + + if self._tmpdir_path: + temp_args = dict(dir=self._tmpdir_path) + elif path.contains("snap"): + self._tmpdir_path = Path.home() if self.debug: print("Snap detected, moving tmp directory to home", file=sys.stderr) temp_args = dict(prefix=".choreographer-", dir=Path.home()) From 315e657bb005e17549c9996421a68dbb97c87c78 Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 12:28:57 -0500 Subject: [PATCH 18/19] Fix bad api syntax --- choreographer/browser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/choreographer/browser.py b/choreographer/browser.py index b2c92a0..345e027 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -96,7 +96,7 @@ def __init__( if self._tmpdir_path: temp_args = dict(dir=self._tmpdir_path) - elif path.contains("snap"): + elif "snap" in path: self._tmpdir_path = Path.home() if self.debug: print("Snap detected, moving tmp directory to home", file=sys.stderr) From b745e2bdbfdf96f2870d3cd7a15546f34fe23480 Mon Sep 17 00:00:00 2001 From: Andrew Pikul Date: Thu, 14 Nov 2024 15:48:14 -0500 Subject: [PATCH 19/19] Fix up conditional --- choreographer/browser.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/choreographer/browser.py b/choreographer/browser.py index d950b07..2bb27be 100644 --- a/choreographer/browser.py +++ b/choreographer/browser.py @@ -77,8 +77,9 @@ def __init__( else: stderr = debug_browser - # awful - if stderr and stderr != subprocess.PIPE and stderr != subprocess.STDOUT and stderr != subprocess.DEVNULL and not isinstance(stderr, int): + if ( stderr + and stderr not in ( subprocess.PIPE, subprocess.STDOUT, subprocess.DEVNULL ) + and not isinstance(stderr, int) ): try: stderr.fileno() except io.UnsupportedOperation: warnings.warn("A value has been passed to debug_browser which is not compatible with python. The default value if deug_browser is True is whatever the value of sys.stderr is. sys.stderr may be many things but debug_browser must be a value Popen accepts for stderr, or True.")