Skip to content
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

grass.app: Refactor PATH setup in grass init script and grass.script.setup #3694

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

Conversation

wenzeslaus
Copy link
Member

This moves setting of the PATH variable and other environment variables to from the grass init script (lib/init/grass.py) and the grass.script subpackage to grass.app subpackage.

I'm not set on where the code should live, but I'm trying to have only one code without duplication. This should ease the changes required for Filesystem Hierarchy Standard (FHS), see #3661.

@wenzeslaus wenzeslaus added this to the 8.5.0 milestone May 8, 2024
@wenzeslaus wenzeslaus self-assigned this May 8, 2024
@github-actions github-actions bot added Python Related code is in Python libraries labels May 8, 2024
python/grass/app/runtime.py Fixed Show fixed Hide fixed
wenzeslaus added a commit to wenzeslaus/grass that referenced this pull request May 9, 2024
With an error in the code which prevents the simple test of the main executable to run, the Pylint check does not run. While it requires some functionality from the grass executable, it does not require everything to work. This removes the step with the explicit test of the grass executable and relies on other workflows to do that test and provide a reasonable message there. This workflow now hopes to get to Pylint and report an issue with Pylint.

The motivation is OSGeo#3694 where I would expect to see a Pylint error, but get a traceback from the grass executable in the simple test. (The executable partially works, but not enough for the simple test.)
"""Wrapper for subprocess.Popen to deal with platform-specific issues"""
if WINDOWS:
kwargs["shell"] = True
return subprocess.Popen(cmd, **kwargs)

Check notice

Code scanning / Bandit

subprocess call - check for execution of untrusted input.

subprocess call - check for execution of untrusted input.
@wenzeslaus
Copy link
Member Author

Just to be clear, the idea here is to prepare it for the FHS changes, not to actually do them. As for the state of this PR, this is close to reviewable state, but it is not there yet.

wenzeslaus added a commit that referenced this pull request May 9, 2024
With an error in the code which prevents the simple test of the main executable to run, the Pylint check does not run. While it requires some functionality from the grass executable, it does not require everything to work. This removes the step with the explicit test of the grass executable and relies on other workflows to do that test and provide a reasonable message there. This workflow now hopes to get to Pylint and report an issue with Pylint.

The motivation was #3694 where I expected to see a Pylint error, but got a traceback from the main executable in the simple test. (The executable partially worked, enough for config, but not enough for the simple test.)
…ill be likely right even with Python's UTF-8 mode because it is non-Windows code. This allows us to remove all encode and decode wrappers for lib/init. Using subprocess.run. Clean up man path code and GISBASE usage.
lib/init/grass.py Fixed Show fixed Hide fixed
python/grass/app/runtime.py Fixed Show resolved Hide resolved
lib/init/grass.py Outdated Show resolved Hide resolved
lib/init/grass.py Show resolved Hide resolved
python/grass/app/runtime.py Outdated Show resolved Hide resolved
python/grass/app/runtime.py Show resolved Hide resolved
@wenzeslaus
Copy link
Member Author

Re: legacy code

There is also some messy code around web browser on macOS, but that spans even outside code in this PR, so that would be already a 3rd PR to create after this one is merged. I know all the strange code makes it harder to review, but I'm also thinking that removing code as part of this process will make it harder to review whether all the code made it through the refactoring. However, I'm changing and removing ton of code anyway. I can go either way. Just let me know.

@wenzeslaus
Copy link
Member Author

There is also some messy code around web browser on macOS, but that spans even outside code in this PR...

Follow up in a separate issue #3781.

@wenzeslaus wenzeslaus marked this pull request as ready for review June 15, 2024 20:17
@wenzeslaus
Copy link
Member Author

This refactoring of the main executable (grass.py) and grass.script.setup.init is now ready for review. It is not aiming at refactoring whole (grass.py), just the part where paths are setup and thus functionality is shared with grass.script.setup.init.

This is to help the transition to FHS and ease the maintenance of the startup code.

@echoix
Copy link
Member

echoix commented Jun 16, 2024

Travis still has a failure related to the app folder, to investigate

Copy link
Member

@hellik hellik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@landam any objections?

@github-actions github-actions bot added the Windows Microsoft Windows specific label Jun 16, 2024
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 Windows Microsoft Windows specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants