-
-
Notifications
You must be signed in to change notification settings - Fork 307
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
Conversation
…rass.script.setup.init
…is now in sync and contains macOS, so getting path to addons needs to deal just with the different dir name, not whole config path.
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.)
python/grass/app/runtime.py
Outdated
"""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.
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. |
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.)
…instead of a custom Popen wrapper.
…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.
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. |
Follow up in a separate issue #3781. |
This refactoring of the main executable ( This is to help the transition to FHS and ease the maintenance of the startup code. |
Travis still has a failure related to the app folder, to investigate |
…set up in the initialization functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@landam any objections?
This needs #3002 to work for Travis. |
Any last minute objections? I plan having it merged today. |
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.