From af8a6aab471551b30445b49720b52954d98517fd Mon Sep 17 00:00:00 2001 From: Vaclav Petras Date: Thu, 8 Aug 2024 17:11:50 -0400 Subject: [PATCH 1/8] grass.app: Move mapset locking to library This moves the lock_mapset function to the library. It introduces only small changes to the code, i.e., proper refactoring and deduplication in relation to the code elsewhere is still needed. However, this unifies the error handling to always raising a custom exception. It also fixes the reported user using the mapset (before always the current user, now, the owner of the lock file). It removes the debug message which I don't find particularly useful, for example it is currently misleading on windows and it is relatively easy to confirm the actual existence in process manager when debugging. Committing with --no-verify due to textproto issue with pre-commit. --- lib/init/grass.py | 65 ++++------------------------------------ python/grass/app/data.py | 64 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 60 deletions(-) diff --git a/lib/init/grass.py b/lib/init/grass.py index 2a9a585b92b..3f49b13e9dc 100755 --- a/lib/init/grass.py +++ b/lib/init/grass.py @@ -1301,64 +1301,6 @@ def set_language(grass_config_dir): gettext.install("grasslibs", gpath("locale")) -def lock_mapset(mapset_path, force_gislock_removal, user): - """Lock the mapset and return name of the lock file - - Behavior on error must be changed somehow; now it fatals but GUI case is - unresolved. - """ - if not os.path.exists(mapset_path): - fatal(_("Path '%s' doesn't exist") % mapset_path) - if not os.access(mapset_path, os.W_OK): - error = _("Path '%s' not accessible.") % mapset_path - stat_info = os.stat(mapset_path) - mapset_uid = stat_info.st_uid - if mapset_uid != os.getuid(): - # GTC %s is mapset's folder path - error = "%s\n%s" % ( - error, - _("You are not the owner of '%s'.") % mapset_path, - ) - fatal(error) - # Check for concurrent use - lockfile = os.path.join(mapset_path, ".gislock") - ret = call([gpath("etc", "lock"), lockfile, "%d" % os.getpid()]) - msg = None - if ret == 2: - if not force_gislock_removal: - msg = _( - "%(user)s is currently running GRASS in selected mapset" - " (file %(file)s found). Concurrent use not allowed.\n" - "You can force launching GRASS using -f flag" - " (note that you need permission for this operation)." - " Have another look in the processor " - "manager just to be sure..." % {"user": user, "file": lockfile} - ) - else: - try_remove(lockfile) - message( - _( - "%(user)s is currently running GRASS in selected mapset" - " (file %(file)s found). Forcing to launch GRASS..." - % {"user": user, "file": lockfile} - ) - ) - elif ret != 0: - msg = ( - _("Unable to properly access '%s'.\nPlease notify system personnel.") - % lockfile - ) - - if msg: - raise Exception(msg) - debug( - "Mapset <{mapset}> locked using '{lockfile}'".format( - mapset=mapset_path, lockfile=lockfile - ) - ) - return lockfile - - # TODO: the gisrcrc here does not make sense, remove it from load_gisrc def unlock_gisrc_mapset(gisrc, gisrcrc): """Unlock mapset from the gisrc file""" @@ -2440,14 +2382,17 @@ def main(): location = mapset_settings.full_mapset + from grass.app.data import lock_mapset, MapsetLockingException + try: # check and create .gislock file lock_mapset( - mapset_settings.full_mapset, + install_path=GISBASE, + mapset_path=mapset_settings.full_mapset, user=user, force_gislock_removal=params.force_gislock_removal, ) - except Exception as e: + except MapsetLockingException as e: fatal(e.args[0]) sys.exit(_("Exiting...")) diff --git a/python/grass/app/data.py b/python/grass/app/data.py index 439a6c3c4d1..c47ce584b3d 100644 --- a/python/grass/app/data.py +++ b/python/grass/app/data.py @@ -15,8 +15,10 @@ import os import tempfile import getpass +import subprocess import sys from shutil import copytree, ignore_patterns +from pathlib import Path import grass.grassdb.config as cfg from grass.grassdb.checks import is_location_valid @@ -162,3 +164,65 @@ def ensure_default_data_hierarchy(): mapset_path = os.path.join(gisdbase, location, mapset) return gisdbase, location, mapset, mapset_path + + +class MapsetLockingException(Exception): + pass + + +def lock_mapset(install_path, mapset_path, force_gislock_removal, user): + """Lock the mapset and return name of the lock file + + Behavior on error must be changed somehow; now it fatals but GUI case is + unresolved. + """ + if not os.path.exists(mapset_path): + raise MapsetLockingException(_("Path '%s' doesn't exist") % mapset_path) + if not os.access(mapset_path, os.W_OK): + error = _("Path '%s' not accessible.") % mapset_path + stat_info = os.stat(mapset_path) + mapset_uid = stat_info.st_uid + if mapset_uid != os.getuid(): + error = "%s\n%s" % ( + error, + _("You are not the owner of '%s'.") % mapset_path, + ) + raise MapsetLockingException(error) + # Check for concurrent use + lockfile = os.path.join(mapset_path, ".gislock") + install_path = Path(install_path) + ret = subprocess.run( + [install_path / "etc" / "lock", lockfile, "%d" % os.getpid()], check=False + ).returncode + msg = None + if ret == 2: + if not force_gislock_removal: + lockfile = Path(lockfile) + msg = _( + "{user} is currently running GRASS in selected mapset" + " (file {file} found). Concurrent use not allowed.\n" + "You can force launching GRASS using -f flag" + " (note that you need permission for this operation)." + " Have another look in the processor " + "manager just to be sure...".format( + user=lockfile.owner(), file=lockfile + ) + ) + else: + try_remove(lockfile) + message( + _( + "%(user)s is currently running GRASS in selected mapset" + " (file %(file)s found). Forcing to launch GRASS..." + % {"user": user, "file": lockfile} + ) + ) + elif ret != 0: + msg = ( + _("Unable to properly access '%s'.\nPlease notify system personnel.") + % lockfile + ) + + if msg: + raise MapsetLockingException(msg) + return lockfile From 76a51e4eb2d398e9f796d62e307dd26575f51bf3 Mon Sep 17 00:00:00 2001 From: Vaclav Petras Date: Fri, 9 Aug 2024 15:26:55 -0400 Subject: [PATCH 2/8] Change message wording to add clarity. Fix traslatable messages. Use format function everywhere. Add missing functions. --- lib/init/grass.py | 1 + python/grass/app/data.py | 46 ++++++++++++++++++++-------------------- 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/lib/init/grass.py b/lib/init/grass.py index 3f49b13e9dc..165b27f83f2 100755 --- a/lib/init/grass.py +++ b/lib/init/grass.py @@ -2391,6 +2391,7 @@ def main(): mapset_path=mapset_settings.full_mapset, user=user, force_gislock_removal=params.force_gislock_removal, + message_callback=message, ) except MapsetLockingException as e: fatal(e.args[0]) diff --git a/python/grass/app/data.py b/python/grass/app/data.py index c47ce584b3d..724ae35c7dd 100644 --- a/python/grass/app/data.py +++ b/python/grass/app/data.py @@ -19,6 +19,8 @@ import sys from shutil import copytree, ignore_patterns from pathlib import Path + +import grass.script as gs import grass.grassdb.config as cfg from grass.grassdb.checks import is_location_valid @@ -170,22 +172,24 @@ class MapsetLockingException(Exception): pass -def lock_mapset(install_path, mapset_path, force_gislock_removal, user): +def lock_mapset( + install_path, mapset_path, force_gislock_removal, user, message_callback +): """Lock the mapset and return name of the lock file Behavior on error must be changed somehow; now it fatals but GUI case is unresolved. """ if not os.path.exists(mapset_path): - raise MapsetLockingException(_("Path '%s' doesn't exist") % mapset_path) + raise MapsetLockingException(_("Path '{}' doesn't exist").format(mapset_path)) if not os.access(mapset_path, os.W_OK): - error = _("Path '%s' not accessible.") % mapset_path + error = _("Path '{}' not accessible.").format(mapset_path) stat_info = os.stat(mapset_path) mapset_uid = stat_info.st_uid if mapset_uid != os.getuid(): - error = "%s\n%s" % ( - error, - _("You are not the owner of '%s'.") % mapset_path, + error = "{error}\n{detail}".format( + error=error, + detail=_("You are not the owner of '{}'.").format(mapset_path), ) raise MapsetLockingException(error) # Check for concurrent use @@ -200,28 +204,24 @@ def lock_mapset(install_path, mapset_path, force_gislock_removal, user): lockfile = Path(lockfile) msg = _( "{user} is currently running GRASS in selected mapset" - " (file {file} found). Concurrent use not allowed.\n" + " (file {file} found). Concurrent use of one mapset not allowed.\n" "You can force launching GRASS using -f flag" - " (note that you need permission for this operation)." - " Have another look in the processor " - "manager just to be sure...".format( - user=lockfile.owner(), file=lockfile - ) - ) + " (assuming your have sufficient access permissions)." + " Confirm in a process manager " + "that there is no other process using the mapset." + ).format(user=lockfile.owner(), file=lockfile) else: - try_remove(lockfile) - message( + gs.try_remove(lockfile) + message_callback( _( - "%(user)s is currently running GRASS in selected mapset" - " (file %(file)s found). Forcing to launch GRASS..." - % {"user": user, "file": lockfile} - ) + "{user} is currently running GRASS in selected mapset" + " (file %(file)s found), but forcing to launch GRASS anyway..." + ).format(user=lockfile.owner(), file=lockfile) ) elif ret != 0: - msg = ( - _("Unable to properly access '%s'.\nPlease notify system personnel.") - % lockfile - ) + msg = _( + "Unable to properly access '{}'.\nPlease notify your system administrator." + ).format(lockfile) if msg: raise MapsetLockingException(msg) From 112373a083c6018311a513e98049023c1ebdd08c Mon Sep 17 00:00:00 2001 From: Vaclav Petras Date: Mon, 12 Aug 2024 15:45:36 -0400 Subject: [PATCH 3/8] Document the new function. Get install path (GISBASE) automatically (needs caller change). --- python/grass/app/data.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/python/grass/app/data.py b/python/grass/app/data.py index 724ae35c7dd..f47861debcb 100644 --- a/python/grass/app/data.py +++ b/python/grass/app/data.py @@ -172,13 +172,15 @@ class MapsetLockingException(Exception): pass -def lock_mapset( - install_path, mapset_path, force_gislock_removal, user, message_callback +def lock_mapset(mapset_path, force_gislock_removal, message_callback ): - """Lock the mapset and return name of the lock file + """Acquire a lock for a mapset and return name of new lock file - Behavior on error must be changed somehow; now it fatals but GUI case is - unresolved. + Raises MapsetLockingException when it is not possible to acquire a lock for the + given mapset either becuase of existing lock or due to insufficient permissions. + A corresponding localized message is given in the exception. + + Assumes that the runtime is setup (GISBASE). """ if not os.path.exists(mapset_path): raise MapsetLockingException(_("Path '{}' doesn't exist").format(mapset_path)) @@ -194,7 +196,7 @@ def lock_mapset( raise MapsetLockingException(error) # Check for concurrent use lockfile = os.path.join(mapset_path, ".gislock") - install_path = Path(install_path) + install_path = Path(os.environ["GISBASE"]) ret = subprocess.run( [install_path / "etc" / "lock", lockfile, "%d" % os.getpid()], check=False ).returncode @@ -215,7 +217,7 @@ def lock_mapset( message_callback( _( "{user} is currently running GRASS in selected mapset" - " (file %(file)s found), but forcing to launch GRASS anyway..." + " (file {file} found), but forcing to launch GRASS anyway..." ).format(user=lockfile.owner(), file=lockfile) ) elif ret != 0: From 7219e999e08a7347d9930f353a7d61cc1e8f4dae Mon Sep 17 00:00:00 2001 From: Vaclav Petras Date: Mon, 12 Aug 2024 15:52:38 -0400 Subject: [PATCH 4/8] Remove old parameters --- lib/init/grass.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/init/grass.py b/lib/init/grass.py index 165b27f83f2..61519288c70 100755 --- a/lib/init/grass.py +++ b/lib/init/grass.py @@ -2387,9 +2387,7 @@ def main(): try: # check and create .gislock file lock_mapset( - install_path=GISBASE, mapset_path=mapset_settings.full_mapset, - user=user, force_gislock_removal=params.force_gislock_removal, message_callback=message, ) From c813457aa86095eb044e42f0c65ffe7cf3d03175 Mon Sep 17 00:00:00 2001 From: Vaclav Petras Date: Mon, 12 Aug 2024 15:53:27 -0400 Subject: [PATCH 5/8] Simplify formatting --- python/grass/app/data.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/python/grass/app/data.py b/python/grass/app/data.py index f47861debcb..2efd705eba2 100644 --- a/python/grass/app/data.py +++ b/python/grass/app/data.py @@ -172,8 +172,7 @@ class MapsetLockingException(Exception): pass -def lock_mapset(mapset_path, force_gislock_removal, message_callback -): +def lock_mapset(mapset_path, force_gislock_removal, message_callback): """Acquire a lock for a mapset and return name of new lock file Raises MapsetLockingException when it is not possible to acquire a lock for the From 260ecde7fce3d9da9f682e172c50b602eae48048 Mon Sep 17 00:00:00 2001 From: Vaclav Petras Date: Mon, 12 Aug 2024 16:12:05 -0400 Subject: [PATCH 6/8] Report owner, then delete the file. Clean up paths. --- lib/init/grass.py | 2 +- python/grass/app/data.py | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/init/grass.py b/lib/init/grass.py index 61519288c70..e59eebacd06 100755 --- a/lib/init/grass.py +++ b/lib/init/grass.py @@ -2388,7 +2388,7 @@ def main(): # check and create .gislock file lock_mapset( mapset_path=mapset_settings.full_mapset, - force_gislock_removal=params.force_gislock_removal, + force_lock_removal=params.force_gislock_removal, message_callback=message, ) except MapsetLockingException as e: diff --git a/python/grass/app/data.py b/python/grass/app/data.py index 2efd705eba2..92102b163ed 100644 --- a/python/grass/app/data.py +++ b/python/grass/app/data.py @@ -172,13 +172,17 @@ class MapsetLockingException(Exception): pass -def lock_mapset(mapset_path, force_gislock_removal, message_callback): +def lock_mapset(mapset_path, force_lock_removal, message_callback): """Acquire a lock for a mapset and return name of new lock file Raises MapsetLockingException when it is not possible to acquire a lock for the given mapset either becuase of existing lock or due to insufficient permissions. A corresponding localized message is given in the exception. + A *message_callback* is a function which will be called to report messages about + certain states. Specifically, the funtion is called when forcibly unlocking the + mapset. + Assumes that the runtime is setup (GISBASE). """ if not os.path.exists(mapset_path): @@ -195,14 +199,13 @@ def lock_mapset(mapset_path, force_gislock_removal, message_callback): raise MapsetLockingException(error) # Check for concurrent use lockfile = os.path.join(mapset_path, ".gislock") - install_path = Path(os.environ["GISBASE"]) + locker_path = os.path.join(os.environ["GISBASE"], "etc", "lock") ret = subprocess.run( - [install_path / "etc" / "lock", lockfile, "%d" % os.getpid()], check=False + [locker_path, lockfile, "%d" % os.getpid()], check=False ).returncode msg = None if ret == 2: - if not force_gislock_removal: - lockfile = Path(lockfile) + if not force_lock_removal: msg = _( "{user} is currently running GRASS in selected mapset" " (file {file} found). Concurrent use of one mapset not allowed.\n" @@ -210,15 +213,15 @@ def lock_mapset(mapset_path, force_gislock_removal, message_callback): " (assuming your have sufficient access permissions)." " Confirm in a process manager " "that there is no other process using the mapset." - ).format(user=lockfile.owner(), file=lockfile) + ).format(user=Path(lockfile).owner(), file=lockfile) else: - gs.try_remove(lockfile) message_callback( _( "{user} is currently running GRASS in selected mapset" " (file {file} found), but forcing to launch GRASS anyway..." - ).format(user=lockfile.owner(), file=lockfile) + ).format(user=Path(lockfile).owner(), file=lockfile) ) + gs.try_remove(lockfile) elif ret != 0: msg = _( "Unable to properly access '{}'.\nPlease notify your system administrator." From 0a9d2fa2905d343add946414809c39de718936b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edouard=20Choini=C3=A8re?= <27212526+echoix@users.noreply.github.com> Date: Thu, 5 Sep 2024 18:50:10 -0400 Subject: [PATCH 7/8] Apply suggestions from code review --- python/grass/app/data.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/grass/app/data.py b/python/grass/app/data.py index 92102b163ed..ec9ea9ff2f6 100644 --- a/python/grass/app/data.py +++ b/python/grass/app/data.py @@ -180,10 +180,10 @@ def lock_mapset(mapset_path, force_lock_removal, message_callback): A corresponding localized message is given in the exception. A *message_callback* is a function which will be called to report messages about - certain states. Specifically, the funtion is called when forcibly unlocking the + certain states. Specifically, the function is called when forcibly unlocking the mapset. - Assumes that the runtime is setup (GISBASE). + Assumes that the runtime is set up (GISBASE). """ if not os.path.exists(mapset_path): raise MapsetLockingException(_("Path '{}' doesn't exist").format(mapset_path)) From 7d9d4bb8bcba5175a225a70dceadf1d9cb81fe3d Mon Sep 17 00:00:00 2001 From: Vaclav Petras Date: Tue, 24 Sep 2024 15:26:01 -0400 Subject: [PATCH 8/8] Add more details to lock not accessible message --- python/grass/app/data.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/python/grass/app/data.py b/python/grass/app/data.py index 92102b163ed..13f4256fe4b 100644 --- a/python/grass/app/data.py +++ b/python/grass/app/data.py @@ -176,14 +176,14 @@ def lock_mapset(mapset_path, force_lock_removal, message_callback): """Acquire a lock for a mapset and return name of new lock file Raises MapsetLockingException when it is not possible to acquire a lock for the - given mapset either becuase of existing lock or due to insufficient permissions. + given mapset either because of existing lock or due to insufficient permissions. A corresponding localized message is given in the exception. A *message_callback* is a function which will be called to report messages about - certain states. Specifically, the funtion is called when forcibly unlocking the + certain states. Specifically, the function is called when forcibly unlocking the mapset. - Assumes that the runtime is setup (GISBASE). + Assumes that the runtime is setup (specifically that GISBASE is in the environment). """ if not os.path.exists(mapset_path): raise MapsetLockingException(_("Path '{}' doesn't exist").format(mapset_path)) @@ -224,8 +224,9 @@ def lock_mapset(mapset_path, force_lock_removal, message_callback): gs.try_remove(lockfile) elif ret != 0: msg = _( - "Unable to properly access '{}'.\nPlease notify your system administrator." - ).format(lockfile) + "Unable to properly access lock file '{name}'.\n" + "Please resolve this with your system administrator." + ).format(name=lockfile) if msg: raise MapsetLockingException(msg)