-
-
Notifications
You must be signed in to change notification settings - Fork 361
build: refactor grass.py in preparation for FHS #5933
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
Conversation
How about to move the content of my added "resource_paths.py" file to the existing "runtime.py" and move all other configuration variables like from grass.app import Runtime
startup_location = os.path.join(Runtime().share_dir, "demolocation")
color_dir = Runtime().color_dir |
My idea for runtime.py was to provide interface and functionality for all things runtime, so yes, the runtime.py should hide what is in the background and should make it easy to use. On the other hand, maybe there is a value in having all the generated stuff at one place, so interfacing through runtime.py, but having the # resource_paths.py
GRASS_VERSION_NUMBER = "@GRASS_VERSION_NUMBER@"
GRASS_COLOR_DIR = "@GRASS_COLOR_DIR@" # runtime.py
# Actually deal with paths and partial paths, e.g.,
def get_color_dir():
return os.path.normpath(os.path.join(get_prefix(), "@GRASS_COLOR_DIR@")
I'm not sure what you refer to. Remain where? In grass.py? Yes. My question in the review was about having the environmental variable.
+1
Singleton may not be necessary, at least not on the level where a user needs to see it. I recently used module-level # runtime.py
def __getattr__(name):
if name == "GRASS_VERSION":
return "@GRASS_VERSION_NUMBER@"
if name == "GRASS_COLOR_DIR":
# Runs only when actually asked for
return os.path.normpath(os.path.join(GRASS_PREFIX, "@GRASS_COLOR_DIR@") Or, if we want snake case names on module level and no logic is need for some: # runtime.py
version = "@GRASS_VERSION_NUMBER@"
def __getattr__(name):
if name == "color_dir":
# Runs only when actually asked for
return os.path.normpath(os.path.join(prefix, "@GRASS_COLOR_DIR@") Either way: from grass.app.runtime import version
import grass.app.runtime as runtime
runtime.color_dir However, a function returning an object which may or may not be singleton is possible too. # runtime.py
class RuntimePaths:
@property
def color_dir(self):
if self._color_dir is None:
self._color_dir = os.path.normpath(os.path.join(self.prefix, "@GRASS_COLOR_DIR@")
return self._color_dir
_runtime_paths = RuntimePaths()
def runtime_paths():
return _runtime_paths User code: from grass.app import runtime_paths
startup_location = os.path.join(runtime_paths().share_dir, "demolocation")
color_dir = runtime_paths().color_dir Or even through an attribute rather than a function call: _runtime_paths = None
def __getattr__(name):
if name == "runtime_paths":
if _runtime_paths is None:
_runtime_paths = RuntimePaths()
return _runtime_paths from grass.app import runtime_paths
startup_location = os.path.join(runtime_paths.share_dir, "demolocation")
color_dir = runtime_paths.color_dir |
Thanks for the input! I'll see what I can come up with. |
Currently GISBASE is initialised in grass.py in this way: if "GISBASE" in os.environ and len(os.getenv("GISBASE")) > 0:
GISBASE = os.path.normpath(os.environ["GISBASE"])
else:
GISBASE = os.path.normpath("@GISBASE_INSTALL_PATH@")
os.environ["GISBASE"] = GISBASE The new path env. variables should also be set up in similar way, if we are to keep current behaviour. E.g., if the env. variable is set, then return the value of that, or set it with a configured value. The functions in runtime.py and setup.py also reflects the factor of environment. To achieve that using a simple class may perhaps be an approach: # runtime.py
class RuntimePaths:
def __init__(self, env=os.environ):
self.env = env
def __getattr__(self, name):
if name == "grass_version":
return runtime_paths.GRASS_VERSION_NUMBER
# user code
from runtime import RuntimePaths
gisbase = RuntimePaths(env=my_env).gisbase
# or:
runtime_paths = RuntimePaths() # using default "os.environ"
version = runtime_paths.grass_version
share_dir = runtime_paths.share_dir |
Just pushed update with the alternative approach I mentioned before. It is not completely ready yet, especially the cmake build needs some more work. But you can see if this could be an acceptable and/or promising solution. |
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.
Good idea, when the environment is involved, even this should accept env parameter.
(This is extracted from OSGeo#5630 to simplify review.) The transitional period from the traditional GRASS installation structure to a Filesystem Hierarchy Standard (FHS) complying structure (enabled by CMake build) requires an alternative way to find resources. Currently all resources are located in relation to GISBASE, that is no longer the case with a FHS installation. In addition, and instead of GISBASE, the following environment variables will be needed (not included in this update): - GRASS_SHAREDIR - GRASS_LOCALEDIR - GRASS_PYDIR - GRASS_GUIWXDIR - GRASS_GUISCRIPTDIR - GRASS_GUIRESDIR - GRASS_FONTSDIR - GRASS_ETCDIR Setting up all these variables would bloat the grass.py file, therefore this update suggests to move this to the grass.app Python module. Summary of changes: - grass.py: to simplify Conda adoption, the variable GRASS_PREFIX is added, which is essentially equal to installation prefix, but easily changeable if needed. - grass.py need to find the Python module to be able to set necessary variables, a new variable GRASS_PYDIR is configured into the file to do just this. - python/grass/app/resource_paths.py is a configurable file (like grass.py) - update copy_python_files_in_subdir.cmake to be able to exclude files
Any more suggestions, objections. #5630 builds upon this and that one is starting to shape up, so it would be good to move on. |
Are some changes in the mswindows/ folder needed, regarding the changes of |
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.
I like the Python part and the CMake changes look reasonable. We can merge this as is. I have just some minor comments which you can ignore.
I'm merging this now. There are a number of adaptations to be made in "grass/app/runtime.py" and "grass/script/setup.py" following the changes this brings. The first step will be #5630. |
(This is extracted from #5630 to simplify review.)
The transitional period from the traditional GRASS installation structure to a Filesystem Hierarchy Standard (FHS) complying structure (enabled by CMake build) requires an alternative way to find resources. Currently all resources are located in relation to GISBASE, that is no longer the case with a FHS installation.
In addition, and instead of GISBASE, the following environment variables will be needed (not included in this update):
Setting up all these variables would bloat the grass.py file, therefore this update suggests to move this to the grass.app Python module.
Summary of changes: