[Pranav-error]

Fixes #7738.

set_executable_paths(), set_dynamic_library_path() and set_python_path_variable() added their entries unconditionally, so calling the setup repeatedly on one environment grew PATH, PYTHONPATH and the library path variable without bound. That happens in a pytest run and in a Jupyter kernel, where gs.setup.init() is called many times on os.environ.

set_executable_paths() and set_python_path_variable() now use prepend_paths(), which removes other occurrences of the paths and then prepends them (suggested in the review), so the paths keep the precedence they have on main and the variables don't grow. set_dynamic_library_path() appends the library path, so it skips the path when it is already there, which keeps the order of main. I did not route init() through ensure_runtime_env(), since runtime_env_is_active() only checks that GISBASE appears in PATH and would skip a re-setup that is actually needed.

set_dynamic_library_path() also assigned "" before appending when the variable was unset, leaving a leading separator. An empty entry in a library path means the current directory, so it now assigns the path directly in that case.

Tests

Tests in python/grass/app/tests/grass_app_runtime_test.py, covering, for each setter, that a second call changes nothing and that an existing value is preserved, the empty-entry case, switching back to an installation used before, and a path already present behind other paths.

Checked against the unfixed module first: the idempotency and empty-entry tests fail there and pass with the change. Locally 40 tests in the file pass; the other 6 need an installed GRASS (get_install_path() returns None here) and fail the same way on main.

I used an AI assistant while preparing this. I understand the change and can explain it.

[nilason]

Looks good to me, I have only a minor suggestion. I'll leave the final approval and merge to a colleague for another pair of eyes on this.

[nilason]

Suggested change

	# Assigning rather than appending to an empty value avoids a leading
	# separator, which would be an empty entry meaning the current directory.

Not worth the comment in code, but please add to commit description.

No need to explain every line of code, I think.

[Pranav-error]

Done — comment removed and the explanation moved into the commit message:

In set_dynamic_library_path() the value is assigned rather than appended when the variable is empty. Appending to an empty value would leave a leading separator, and an empty entry in a path list means the current directory.

Also rebased onto current main while I was in there.

Fair point on not explaining every line. That one was load-bearing enough that losing it would invite someone to "simplify" the assignment back into an append, which is why I had written it down — but the commit message is the better place for it, since that is where someone doing the simplifying would look.

[echoix]

That reads weird to me. Is there missing parentheses for me to understand well?

I read it like this:

        existing + os.pathsep + (library_path if existing else library_path)

Which is a useless if.

If it resolves to

        (existing + os.pathsep + library_path) if existing else library_path

Then it makes a bit more sense

[echoix]

Is the deduplication keeping the paths in the same order, keeping the left-most instance? And is it only affecting what we are adding, not pre-existing paths set by the user or software outside of us?

My comment might mean contradictory instructions though. But not keeping a path in front because it it found later on in the list might not be equivalent, if a binary of the same name exists in one of these other locations

[Pranav-error]

Both are worth answering precisely rather than from memory, so I went back to the code for each.

Line 357 (set_dynamic_library_path): the parens aren't missing — the code is already correct, though I agree it reads ambiguously. Python's conditional expression (A if cond else B) binds looser than +, so this:

env[variable_name] = (
    existing + os.pathsep + library_path if existing else library_path
)

parses as (existing + os.pathsep + library_path) if existing else library_path, not existing + os.pathsep + (library_path if existing else library_path). That's the intended behaviour — append with a separator when there's an existing value, otherwise just the bare path. Happy to add the explicit parens you suggested so it doesn't need a precedence check to read.

Line 291 (set_executable_paths dedup):

existing = env.get("PATH") or ""
present = existing.split(os.pathsep) if existing else []
paths = [path for path in paths if path not in present]

Yes to both parts of your question. paths starts as a deque built left-to-right by append_left_addon_paths / append_left_main_executable_paths — only the new entries, in that original order. The comprehension filters that deque against present without reordering it, so the left-most/original order among the new entries is preserved. It doesn't dedupe the new entries against each other, only against what's already in PATH — though two identical new entries isn't a case that can actually happen given how the deque is built.

And it's scoped to only what's being added: existing (the pre-existing PATH value) is never split, reordered, or deduped — it's appended back whole and rejoined at the end. So a duplicate that was already in the user's PATH before this function ran stays exactly as it was; only what GRASS itself is about to add gets checked against it.

Will push the explicit parens from the first point as a follow-up commit.

[wenzeslaus]

While this solves the duplication, it changes behavior when the session is in the middle of the path. It is not prepended anymore, which would matter if multiple installations or software packages conflict.

Details

These details and the inline suggestions are AI-generated.

Skipping paths which are already present keeps them wherever they are in the variable, so they can lose precedence. Before this PR, they were always put in front. This is the concern from the earlier review question about keeping paths in front. Results of the setters on plain dicts, main vs. this PR:

Scenario	main	This PR
Install A, then B, then A again	PATH=/A/bin:..., PYTHONPATH=/A/etc/python:...	PATH=/B/bin:/A/bin:..., PYTHONPATH=/B/etc/python:/A/etc/python (B wins while GISBASE is A)
PATH=/other/bin:/A/bin:/usr/bin, then install A	/A/bin first	/A/bin stays behind /other/bin
Install A, then B	/B/bin, addon paths, /A/bin	/B/bin, /A/bin, addon paths (A's tools now come before the addons)

An alternative which keeps the original precedence and still bounds the growth is to remove other occurrences of the paths being added and then prepend them, see the inline suggestions. Removing a later duplicate does not change which file is found, so the effective search order stays the same as on main. This was checked by calling set_executable_paths, set_dynamic_library_path, and set_python_path_variable on plain dicts for 90 combinations:

• starting PATH values: /usr/bin, /other/bin:/A/bin:/usr/bin, and /B/bin:/usr/bin,
• installation sequences: all 30 sequences of one to four setups with installations /A and /B (A; B; A, A; A, B; ...; B, B, B, B).

After removing duplicates (keeping the first occurrence), PATH, PYTHONPATH, and LD_LIBRARY_PATH from the suggested code are identical to main in all 90 combinations. For this PR, 67 of the 90 differ.

The library path variable is appended rather than prepended, and skipping it when present already gives the same order as main, so set_dynamic_library_path can stay as it is in this PR. (That also means the description's statement that another installation's paths are added in front does not hold for the library path; it did not on main either.)

The helper makes the explanation in the set_executable_paths docstring unnecessary, so the suggestion puts it on the helper only.

Tests:

• test_executable_paths_of_previous_installation_are_moved_to_front and test_executable_paths_already_present_are_moved_to_front (suggested) fail with the current code of this PR and pass with the suggested change.
• test_executable_paths_of_another_installation_are_added_in_front (current) passes with both, so the suggestion replaces it.
• With the suggested changes, all 46 tests in grass_app_runtime_test.py pass.

Cost: one setup (the three setters together) with a 19-entry PATH, measured with timeit on Linux, takes 13 µs on main, 15-16 µs with this PR, and 17 µs with the suggestion. For comparison, gs.setup.init() followed by finish() takes about 28 ms, so the difference between the versions (up to 4 µs) is about 0.015% of it. The new tests use plain dicts and each runs in under 5 ms.

[wenzeslaus]

One helper for both prepended variables, which removes other occurrences of the paths and then prepends them, so the paths keep precedence and the variable does not grow.

Suggested change

	def set_executable_paths(install_path, grass_config_dir, env):
	"""Add paths with executables to PATH in _env_"""
	"""Add paths with executables to PATH in _env_
	Paths already present are not added again, so calling this repeatedly on
	the same environment does not grow the variable. Paths from another
	installation are still added in front of the existing ones.
	"""
	paths = collections.deque()
	# Addons
	append_left_addon_paths(paths, grass_config_dir, env=env)
	# Standard installation
	append_left_main_executable_paths(paths, install_path=install_path)
	paths.append(env.get("PATH"))
	existing = env.get("PATH") or ""
	present = existing.split(os.pathsep) if existing else []
	paths = [path for path in paths if path not in present]
	if not paths:
	return
	if existing:
	paths.append(existing)
	env["PATH"] = os.pathsep.join(paths)
	def prepend_paths(variable_name, paths, env):
	"""Put paths in front of a path list variable in _env_
	Other occurrences of the paths are removed, so the paths take precedence
	and repeated calls on the same environment do not grow the variable.
	"""
	existing = env.get(variable_name)
	rest = existing.split(os.pathsep) if existing else []
	env[variable_name] = os.pathsep.join(
	paths + [path for path in rest if path not in paths]
	)
	def set_executable_paths(install_path, grass_config_dir, env):
	"""Add paths with executables to PATH in _env_"""
	paths = collections.deque()
	# Addons
	append_left_addon_paths(paths, grass_config_dir, env=env)
	# Standard installation
	append_left_main_executable_paths(paths, install_path=install_path)
	prepend_paths("PATH", list(paths), env=env)

[wenzeslaus]

The same helper for PYTHONPATH.

Suggested change

	def set_python_path_variable(install_path, env):
	"""Set PYTHONPATH to find GRASS Python package in subprocesses"""
	path = env.get("PYTHONPATH")
	etcpy = os.path.join(install_path, "etc", "python")
	if path and etcpy in path.split(os.pathsep):
	return
	path = etcpy + os.pathsep + path if path else etcpy
	env["PYTHONPATH"] = path
	def set_python_path_variable(install_path, env):
	"""Set PYTHONPATH to find GRASS Python package in subprocesses"""
	prepend_paths("PYTHONPATH", [os.path.join(install_path, "etc", "python")], env=env)

[wenzeslaus]

Tests for switching back to an installation used before, and for a path already present behind other paths. Both fail with the current code and pass with the suggested change.

Suggested change

	def test_executable_paths_of_another_installation_are_added_in_front(tmp_path):
	"""Switching to another installation still puts its paths first"""
	env = {"PATH": "/usr/bin"}
	first = tmp_path / "first"
	second = tmp_path / "second"
	set_executable_paths(str(first), str(tmp_path / "config"), env=env)
	set_executable_paths(str(second), str(tmp_path / "config"), env=env)
	entries = env["PATH"].split(os.pathsep)
	assert str(second / "bin") in entries
	assert entries.index(str(second / "bin")) < entries.index(str(first / "bin"))
	def test_executable_paths_of_previous_installation_are_moved_to_front(tmp_path):
	"""Switching back to an installation puts its paths first again"""
	env = {"PATH": "/usr/bin"}
	first = tmp_path / "first"
	second = tmp_path / "second"
	for install_path in (first, second, first):
	set_executable_paths(str(install_path), str(tmp_path / "config"), env=env)
	set_python_path_variable(str(install_path), env=env)
	assert env["PATH"].split(os.pathsep)[0] == str(first / "bin")
	assert env["PYTHONPATH"].split(os.pathsep) == [
	str(first / "etc" / "python"),
	str(second / "etc" / "python"),
	]
	def test_executable_paths_already_present_are_moved_to_front(tmp_path):
	"""A path already in PATH behind other paths is put first"""
	bin_path = str(tmp_path / "bin")
	env = {"PATH": os.pathsep.join(["/other/bin", bin_path, "/usr/bin"])}
	set_executable_paths(str(tmp_path), str(tmp_path / "config"), env=env)
	entries = env["PATH"].split(os.pathsep)
	assert entries[0] == bin_path
	assert entries.count(bin_path) == 1

[Pranav-error]

Thanks, you're right. I reran the comparison on plain dicts with my own script for your 90 combinations: this PR differs from main in 67 after removing duplicates, the suggestion in 0, and neither grows on repeated calls. I applied prepend_paths() for PATH and PYTHONPATH and your two tests, kept the library path as is, and fixed the description (the library path is appended). Locally 40 pass; the other 6 need an installed GRASS (get_install_path() returns None here) and fail the same way on main.

One more thing the comparison showed: on main, set_executable_paths() raises a TypeError when PATH isn't set at all (os.pathsep.join() gets None). This PR and the suggestion handle it.