From c6ab1d31bbb27d4c2cfbcfd2f82523b6d0e668d8 Mon Sep 17 00:00:00 2001 From: matthewholman Date: Fri, 28 Aug 2026 20:17:09 -0400 Subject: [PATCH 1/3] Dispatch layup verbs from installed metadata, not PATH (#500) find_layup_verbs() walked every PATH directory for executables named layup-*, and the dispatcher ran `layup-` as a bare name, leaving the choice of program to PATH order. With two layup installations on one machine, `layup comet` from one environment could execute the other's layup-comet. Worse, any executable named layup- in a writable directory earlier on PATH would be run in preference to the real one, with the user's privileges -- an empty PATH entry means the working directory, and shared scientific systems often carry group-writable bin directories. PATH could also inject verbs into layup's own help output. Verbs now come from the installed distribution's entry points, and the verb runs in this process. Nothing is resolved by name, so the code that runs is always this installation's. Also pins the three CLI subprocess tests to this environment's console script rather than PATH's, which is how the problem surfaced. --- .../utilities/data_utilities_for_tests.py | 21 +++++++ src/layup_cmdline/main.py | 59 ++++++++++++------- tests/layup/test_comet.py | 4 +- tests/layup/test_predict.py | 14 ++--- 4 files changed, 67 insertions(+), 31 deletions(-) diff --git a/src/layup/utilities/data_utilities_for_tests.py b/src/layup/utilities/data_utilities_for_tests.py index 7afa481c..4f779fd2 100644 --- a/src/layup/utilities/data_utilities_for_tests.py +++ b/src/layup/utilities/data_utilities_for_tests.py @@ -44,3 +44,24 @@ def get_test_filepath(filename): # Returned path: `/tests/data/filename` return os.path.join(THIS_DIR, "tests/data", filename) + + +def layup_cli(*args): + """argv prefix that runs *this* environment's ``layup``, not ``PATH``'s. + + ``subprocess.run(["layup", ...])`` resolves the name against ``PATH``, so a + test exercises whichever installation comes first on the machine rather than + the one under test. That fails confusingly when another layup is installed + (a conda environment with a broken assist/rebound link, say) and, worse, + passes for the wrong reason when the stale installation happens to work + (issue #500). + + Console scripts are installed alongside the interpreter running the tests, + so resolving from ``sys.executable`` pins the invocation to this environment + while still exercising the real entry point and its subcommand dispatch. + """ + import sys + from pathlib import Path + + exe = Path(sys.executable).parent / "layup" + return [str(exe) if exe.exists() else "layup", *args] diff --git a/src/layup_cmdline/main.py b/src/layup_cmdline/main.py index 6bbb262a..c10c2cdb 100644 --- a/src/layup_cmdline/main.py +++ b/src/layup_cmdline/main.py @@ -1,23 +1,34 @@ import argparse -import subprocess import sys -import shutil -import os # # Generic verb dispatcher code # +def _verb_entry_points(): + """The ``layup-*`` console scripts *this* installation declares, by verb. + + Taken from the installed distribution's own metadata rather than by + searching ``PATH``. Searching ``PATH`` picked up any executable named + ``layup-`` anywhere on it, so with two layup installations on one + machine the dispatcher could run the other one's verb -- and an executable + dropped in any writable directory earlier on ``PATH`` (an empty ``PATH`` + entry means the working directory) would be run in preference to the real + one, with the user's privileges. + """ + from importlib.metadata import distribution + + verbs = {} + for ep in distribution("layup").entry_points: + if ep.group == "console_scripts" and ep.name.startswith("layup-"): + verbs[ep.name[len("layup-") :]] = ep + return verbs + + def find_layup_verbs(): - """Find available layup commands in the system's PATH.""" - layup_verbs = [] - for directory in os.environ.get("PATH", "").split(os.pathsep): - if os.path.isdir(directory): - for item in os.listdir(directory): - if item.startswith("layup-") and os.access(os.path.join(directory, item), os.X_OK): - layup_verbs.append(item[len("layup-") :]) - return sorted(set(layup_verbs)) + """Available layup verbs, from this installation's own metadata.""" + return sorted(_verb_entry_points()) def main(): @@ -75,21 +86,27 @@ def main(): parser.print_help() sys.exit(1) - # Construct the full command name utility = f"layup-{args.verb}" - - # Ensure the command is available - if not shutil.which(utility): + entry = _verb_entry_points().get(args.verb) + if entry is None: print(f"Error: '{utility}' is not available.") sys.exit(1) - # Execute the command with the remaining arguments + # Run the verb in this process. Nothing is resolved by name, so the verb + # that runs is always the one belonging to this installation. + verb_main = entry.load() + argv = sys.argv + sys.argv = [utility, *args.args] try: - result = subprocess.run([utility] + args.args, check=True) - sys.exit(result.returncode) - except subprocess.CalledProcessError as e: - print(f"Error: Command '{utility}' failed with exit code {e.returncode}.") - sys.exit(e.returncode) + code = verb_main() + except SystemExit as exc: # the verbs exit on their own error paths + code = exc.code + finally: + sys.argv = argv + if code not in (0, None): + print(f"Error: Command '{utility}' failed with exit code {code}.") + sys.exit(code) + sys.exit(0) if __name__ == "__main__": diff --git a/tests/layup/test_comet.py b/tests/layup/test_comet.py index 7b2c1b08..ce7ed6de 100644 --- a/tests/layup/test_comet.py +++ b/tests/layup/test_comet.py @@ -6,7 +6,7 @@ import numpy as np from numpy.testing import assert_allclose, assert_equal from layup.comet import _remove_spc, _assist_integrate, _direction_of_integration, _apply_comet, comet_cli -from layup.utilities.data_utilities_for_tests import get_test_filepath +from layup.utilities.data_utilities_for_tests import get_test_filepath, layup_cli from layup.utilities.file_io.CSVReader import CSVDataReader import pandas as pd import assist @@ -214,7 +214,7 @@ def test_comet_output(tmpdir): # The demo comet fixture is keyed by ObjID; comet's -pid now defaults to # provID (CLI-consistency), so pass -pid ObjID explicitly. result = subprocess.run( - ["layup", "comet", str(input_file), "-f", "-o", str(temp_out_file), "-pid", "ObjID"] + layup_cli("comet", str(input_file), "-f", "-o", str(temp_out_file), "-pid", "ObjID") ) assert result.returncode == 0 diff --git a/tests/layup/test_predict.py b/tests/layup/test_predict.py index c1e8a80c..f0bb583f 100644 --- a/tests/layup/test_predict.py +++ b/tests/layup/test_predict.py @@ -15,7 +15,7 @@ layup_get_residual_vectors, layup_calculate_rates_and_geometry, ) -from layup.utilities.data_utilities_for_tests import get_test_filepath +from layup.utilities.data_utilities_for_tests import get_test_filepath, layup_cli from layup.utilities.file_io.CSVReader import CSVDataReader @@ -221,7 +221,7 @@ def test_predict_output(tmpdir): temp_out_file = f"test_output_{input_file.stem}" result = subprocess.run( - ["layup", "predict", str(input_file), "-f", "-o", str(temp_out_file), "-s", start] + layup_cli("predict", str(input_file), "-f", "-o", str(temp_out_file), "-s", start) ) assert result.returncode == 0 @@ -281,8 +281,7 @@ def test_predict_output(tmpdir): # Testing the output of the sexagesimal conversion separately result = subprocess.run( - [ - "layup", + layup_cli( "predict", str(input_file), "-f", @@ -291,7 +290,7 @@ def test_predict_output(tmpdir): "-s", start, "-sg", - ] + ) ) assert result.returncode == 0 @@ -420,8 +419,7 @@ def test_get_onsky_data_output(tmpdir): temp_out_file = f"test_output_{input_file.stem}" result = subprocess.run( - [ - "layup", + layup_cli( "predict", str(input_file), "-f", @@ -430,7 +428,7 @@ def test_get_onsky_data_output(tmpdir): "-s", start, "-osd", - ] + ) ) assert result.returncode == 0 result_file = Path(f"{tmpdir}/{temp_out_file}.csv") From 796d87a70ffe6471ab995aa9f7cd9543fab879bb Mon Sep 17 00:00:00 2001 From: matthewholman Date: Wed, 2 Sep 2026 13:37:06 -0400 Subject: [PATCH 2/3] Fold the entry-point lookup into find_layup_verbs() Review feedback: the two-function form was a step to read through for no gain. find_layup_verbs() now returns the verb -> entry point mapping directly, main() reuses it instead of reading the distribution metadata a second time, and the argparse choices are sorted at the point of use so the help text keeps a stable order. Also corrects the layup_cli() docstring, which gave a broken assist/rebound link as the example failure. The failure actually seen is an older layup on PATH rejecting arguments the current code added. --- .../utilities/data_utilities_for_tests.py | 23 ++++++++-------- src/layup_cmdline/main.py | 27 +++++++------------ 2 files changed, 20 insertions(+), 30 deletions(-) diff --git a/src/layup/utilities/data_utilities_for_tests.py b/src/layup/utilities/data_utilities_for_tests.py index 4f779fd2..e12ede27 100644 --- a/src/layup/utilities/data_utilities_for_tests.py +++ b/src/layup/utilities/data_utilities_for_tests.py @@ -47,18 +47,17 @@ def get_test_filepath(filename): def layup_cli(*args): - """argv prefix that runs *this* environment's ``layup``, not ``PATH``'s. - - ``subprocess.run(["layup", ...])`` resolves the name against ``PATH``, so a - test exercises whichever installation comes first on the machine rather than - the one under test. That fails confusingly when another layup is installed - (a conda environment with a broken assist/rebound link, say) and, worse, - passes for the wrong reason when the stale installation happens to work - (issue #500). - - Console scripts are installed alongside the interpreter running the tests, - so resolving from ``sys.executable`` pins the invocation to this environment - while still exercising the real entry point and its subcommand dispatch. + """Build an argv that runs this environment's ``layup``, not ``PATH``'s. + + ``subprocess.run(["layup", ...])`` resolves the name against ``PATH``, so + the test runs whichever layup comes first on the machine instead of the one + being tested. On a machine with an older layup installed the test fails on + arguments the current code added, and -- worse -- when the older one happens + to accept them, it passes without testing anything (issue #500). + + Console scripts are installed next to the interpreter, so resolving from + ``sys.executable`` pins the call to this environment while still going + through the real entry point and its verb dispatch. """ import sys from pathlib import Path diff --git a/src/layup_cmdline/main.py b/src/layup_cmdline/main.py index c10c2cdb..7d1c133f 100644 --- a/src/layup_cmdline/main.py +++ b/src/layup_cmdline/main.py @@ -1,24 +1,20 @@ import argparse import sys +from importlib.metadata import distribution # # Generic verb dispatcher code # -def _verb_entry_points(): - """The ``layup-*`` console scripts *this* installation declares, by verb. +def find_layup_verbs(): + """The verbs this installation provides, as a dict of name -> entry point. - Taken from the installed distribution's own metadata rather than by - searching ``PATH``. Searching ``PATH`` picked up any executable named - ``layup-`` anywhere on it, so with two layup installations on one - machine the dispatcher could run the other one's verb -- and an executable - dropped in any writable directory earlier on ``PATH`` (an empty ``PATH`` - entry means the working directory) would be run in preference to the real - one, with the user's privileges. + Read from the installed package's own metadata. Do not go back to searching + PATH for executables named layup-: that ran whichever layup came first + on PATH, which on a machine with more than one installation was often not + the one the user meant. """ - from importlib.metadata import distribution - verbs = {} for ep in distribution("layup").entry_points: if ep.group == "console_scripts" and ep.name.startswith("layup-"): @@ -26,11 +22,6 @@ def _verb_entry_points(): return verbs -def find_layup_verbs(): - """Available layup verbs, from this installation's own metadata.""" - return sorted(_verb_entry_points()) - - def main(): # Discover available layup verbs available_verbs = find_layup_verbs() @@ -69,7 +60,7 @@ def main(): action="store_true", ) - parser.add_argument("verb", nargs="?", choices=available_verbs, help="Verb to execute") + parser.add_argument("verb", nargs="?", choices=sorted(available_verbs), help="Verb to execute") parser.add_argument("args", nargs=argparse.REMAINDER, help="Arguments for the verb") args = parser.parse_args() @@ -87,7 +78,7 @@ def main(): sys.exit(1) utility = f"layup-{args.verb}" - entry = _verb_entry_points().get(args.verb) + entry = available_verbs.get(args.verb) if entry is None: print(f"Error: '{utility}' is not available.") sys.exit(1) From c8b08bdd4278f0e81d656d52fad063459602b7b7 Mon Sep 17 00:00:00 2001 From: matthewholman Date: Thu, 3 Sep 2026 13:42:43 -0400 Subject: [PATCH 3/3] Rewrite find_layup_verbs's docstring for a future reader Per review: the previous text was a changelog rather than a description. Two of its three sentences explained what the change fixed and instructed a future editor not to undo it, which is commit-message material -- someone meeting this function in six years needs to know what it returns and why it reads metadata rather than scanning PATH, and nothing else. Wording reviewed and adopted by M. J. Holman. --- src/layup_cmdline/main.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/layup_cmdline/main.py b/src/layup_cmdline/main.py index 7d1c133f..958f2a88 100644 --- a/src/layup_cmdline/main.py +++ b/src/layup_cmdline/main.py @@ -8,12 +8,11 @@ def find_layup_verbs(): - """The verbs this installation provides, as a dict of name -> entry point. + """Return the verbs this installation provides, as a dict mapping verb name + to entry point. - Read from the installed package's own metadata. Do not go back to searching - PATH for executables named layup-: that ran whichever layup came first - on PATH, which on a machine with more than one installation was often not - the one the user meant. + The names come from the installed package's own metadata, so they are the + verbs belonging to this layup, not whichever ones happen to be first on PATH. """ verbs = {} for ep in distribution("layup").entry_points: