lib/init: add GRASS_CONFIG_DIR environment variable #3899
Conversation
imincik
force-pushed
the
config-dir-env-variable
branch
from
eb22446
to
e15d5da
Compare
GRASS_CONFIG_DIR variable allows to set alternative root path for `.grass<VERSION>` configuration directory. If not set, $HOME directory is used.
imincik
force-pushed
the
config-dir-env-variable
branch
from
e15d5da
to
8781e89
Compare
|
Related #3153 |
|
Would it be possible to, instead of introducing another GRASS env var, adopt our code to respect (if set) $XDG_CONFIG_HOME (see https://specifications.freedesktop.org/basedir-spec/basedir-spec-0.6.html)? |
The main use case of this PR is to be able to run multiple isolated instances of grass. Using $HOME or $XDG_CONFIG_HOME variables to force grass to store configuration in alternative directory is not a good idea, because this will also have an impact on other tools relying on those variables. That's why I introduced a new grass specific environment variable. |
But I agree, that usage of XDG_CONFIG_HOME instead of HOME is a good fix for #3153 . |
I see. |
Co-authored-by: Nicklas Larsson <n_larsson@yahoo.com>
github-actions
bot
added
Python
![github-actions[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
- create grass.utils.config to avoid code duplication in grass.py and core.utils - change grass.py in order to use grass.utils.config.get_grass_config_path()
|
I will not be able to look at and test this this week, but please hold on, I see there are Mac specific changes now that need hands-on testing/review. |
Sure. This PR shouldn't change behaviour compared to #3694. |
There was a problem hiding this comment.
This seems like a reasonable feature, although I'm wondering whether other (all?) software allows the same, i.e., having its own variable to set config dir just for that software.
As for the implementation, there is couple of other places which may need fixing. g.extension needs to do something, but I didn't examine what. lib/gis/home.c worries me a little bit as it seems to figure out the path as well, at this point duplicating what is in Python which is more complex code with this PR and the anticipated XDG change.
| if env.get("GRASS_CONFIG_DIR"): | ||
| # override config_path if GRASS_CONFIG_DIR environmental | ||
| # variable is defined | ||
| config_path = os.path.join(env.get("GRASS_CONFIG_DIR"), config_dirname) |
There was a problem hiding this comment.
This overrides, so it can be first and no tests of base env vars or existence of paths from them is needed. (Current code would raise if HOME does not exist even when GRASS_CONFIG_DIR is set.)
| " system support" | ||
| ) | ||
| if not os.path.exists(env.get(env_dirname)): | ||
| raise IsADirectoryError( |
There was a problem hiding this comment.
Raising IsADirectoryError when it is confusing:
exception IsADirectoryError
Raised when a file operation (such as os.remove()) is requested on a directory. Corresponds to errno EISDIR.
NotADirectoryError seems to be appropriate, but in that case, the test should be isdir not exists.
| @@ -29,15 +29,34 @@ def get_grass_config_dir(major_version, minor_version, env): | |||
| Determines path of GRASS GIS user configuration directory. | |||
| """ | |||
| # The code is in sync with grass.app.runtime (but not the same). | |||
| env_dirname = "APPDATA" if WINDOWS else "HOME" | |||
| if env.get(env_dirname) is None: | |||
| raise KeyError( | |||
There was a problem hiding this comment.
I can't quite see what exception would make sense here. KeyError is more related to the underlying dictionary rather than the state the error message is trying to describe. Consider RuntimeError.
| def get_grass_config_dir(): | ||
| """Get configuration directory | ||
| def create_grass_config_dir(): | ||
| """Create configuration directory |
There was a problem hiding this comment.
Without examining the code more closely, I don't see where the creation happens, but it is not here. I think "creation" is expected to refer to creation of the actual directory rather than construction of the path which is referred to by the simple "get".
GRASS_CONFIG_DIR variable allows to set alternative root path for
.grass<VERSION>configuration directory. If not set, $HOME directoryis used.
The main use case of this PR is to be able to run multiple isolated instances of grass. Using $HOME or $XDG_CONFIG_HOME variables to force grass to store configuration in alternative directory is not a good idea, because this will also have an impact on other tools relying on those variables. That's why I introduced a new grass specific environment variable.
/cc @landam