Skip to content

grass.script: Always use env for shutil.which #5717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

wenzeslaus
Copy link
Member

@wenzeslaus wenzeslaus commented May 21, 2025

Currently, shutil.which is called on Windows to find the executable. However, it does not use the provided env (if any) which leads to the executable not being find if only the env parameter for run_command (and family) contains the runtime setup. This is not a problem on Linux (and probably unix/posix in general), because the Popen call eventually uses the system to find the executable and uses the provided env. However, on Windows, env is not used by the subprocess call. The shutil.which uses the os.environ process, so when the global environment is used, everything still works even on Windows.

This adds the PATH from env to the shutil.which call if it is there, so that it is used to find the exectuable. Additionally, this adds the shutil.which call for all platforms which is considered a best practice, avoiding Bandit's B607: Test for starting a process with a partial path.

https://bandit.readthedocs.io/en/latest/plugins/b607_start_process_with_partial_path.html

Currently, shutil.which is called on Windows to find the executable. However, it does not use the provided env (if any) which leads to the executable not being find if only the env parameter for run_command (and family) contains the runtime setup. This is not a problem on Linux (and probably unix/posix in general), because the Popen call eventually uses the system to find the executable and uses the provided env. However, on Windows, env is not used by the subprocess call. The shutil.which uses the os.environ process, so when the global environment is used, everything still works even on Windows.

This adds the PATH from env to the shutil.which call if it is there, so that it is used to find the exectuable. Additionally, this adds the shutil.which call for all platforms which is considered a best practice, avoiding Bandit's B607: Test for starting a process with a partial path.
@wenzeslaus wenzeslaus requested review from landam, echoix and pesekon2 May 21, 2025 13:48
@pesekon2 pesekon2 removed their request for review May 21, 2025 15:02
@landam landam added the Python Related code is in Python label May 21, 2025
@landam landam added this to the 8.5.0 milestone May 21, 2025
@echoix
Copy link
Member

echoix commented May 21, 2025

Do you have an idea on what to use to test it? I'll try it locally, but I don't have an idea in mind

@wenzeslaus
Copy link
Member Author

On Windows, calling create_project before grass.script.setup.init presumably fails on main and in 8.4 with g.proj not found while on Linux it works.

@landam
Copy link
Member

landam commented May 24, 2025

@wenzeslaus I run the script below from OSGeo4W shell:

import os
import sys
import subprocess

# Append GRASS to the python system path
sys.path.append(
    subprocess.check_output(["grass84.bat", "--config", "python_path"], text=True).strip()
)

import grass.script as gs

# Create a new project
project_path = os.path.join(os.environ["HOMEPATH"], "Documents", "grassdata", "pytest")
gs.create_project(path=project_path, epsg="3358")

# Initialize the GRASS session
with gs.setup.init(project_path) as session:
    # Run GRASS tools
    gs.run_command("g.region", flags="p")

It still fails with an error:

Traceback (most recent call last):
  File "c:\Users\martin\Downloads\grass_create_project.py", line 15, in <module>
    gs.create_project(path=project_path, epsg="3358")
  File "C:\OSGeo4W\apps\grass\grass84\etc\python\grass\script\core.py", line 1820, in create_project
    ps = pipe_command(
         ^^^^^^^^^^^^^
  File "C:\OSGeo4W\apps\grass\grass84\etc\python\grass\script\core.py", line 513, in pipe_command
    return start_command(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\OSGeo4W\apps\grass\grass84\etc\python\grass\script\core.py", line 434, in start_command
    return Popen(args, **popts)
           ^^^^^^^^^^^^^^^^^^^^
  File "C:\OSGeo4W\apps\grass\grass84\etc\python\grass\script\core.py", line 56, in __init__
    raise OSError(_("Cannot find the executable {0}").format(args[0]))
OSError: Cannot find the executable g.proj

because the path doesn't contain in my case C:\OSGeo4W\apps\grass84\bin:

C:\OSGeo4W\apps\qt5\bin;C:\OSGeo4W\apps\Python312\Scripts;C:\OSGeo4W\bin;C:\Windows\system32;C:\Windows;C:\Windows\system32\WBem

@landam
Copy link
Member

landam commented May 25, 2025

@wenzeslaus I tested it on GRASS 8.5 (a6b8391) with the same result.

@wenzeslaus
Copy link
Member Author

Does the OSGeo shell set GISBASE? If yes, that would skip the runtime env setup, and thus fail later.

@wenzeslaus
Copy link
Member Author

Is there a way you could transfer your local test to CI? ie it would fail on current main, but would be disabled, or edit this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants