grass.app: Move mapset locking to library

[wenzeslaus]

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; for that, lock file is removed only after getting its owner). 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.

[wenzeslaus]

This is now ready for review. Description updated accordingly.

[echoix]

What's the testing strategy you'd want here? For now, these code paths don't seem used by any pytest test, so we don't have code coverage to show that the new code is used (and the old one was too). I have Python code coverage working properly for gunittest on a PR in my fork, but it almost doubles the testing time, so I'm not pushing for it now (Ubuntu and macOS).
Would it make sense to take some time to create a special branch that would allow to collect complete code coverage for this PR, or you are confident enough that the current tests used the old code paths and now use all the new code paths?

[wenzeslaus]

Basic tests are done by just running the grass command in the CI, but this is not covered and I did not add any new tests as it requires knowing the executable in the tests and we don't have that resolved. However, I did a lot of local testing and this is tested by any usage of GRASS GIS from CLI or with GUI, so I rely here on old-time user testing (people using main are a good group for this PR).

The situation is not ideal. There is at least one bug in the original code which I carried over. My hope is that with more refactoring this will be easier to fit and easier to test.

I welcome suggestions on getting the executable path or writing the tests for these in general, but it seems to me that in this phase, we need to let these PRs pass without much automated testing.

[echoix]

I welcome suggestions on getting the executable path or writing the tests for these in general, but it seems to me that in this phase, we need to let these PRs pass without much automated testing.

I'm fine with this approach, we need to incrementally do better and not stick too long for PRs that improves and makes the repo in better shape than without.

So what remains to review then is the actual Python. In my quick screening yesterday I didn't find anything that popped to my eyes. I invite another Python-only review.

[echoix]

Merging main to have it check on the pytest on macOS to see if the importing still works (it should, but if it fails there because of spawn it would have failed on windows too)

[echoix]

Merging main to have it check on the pytest on macOS to see if the importing still works (it should, but if it fails there because of spawn it would have failed on windows too)

It didn't fail :)

[echoix]

Do you know if some changes to the deleted lines were made since the creation of the PR, and that should be part of the new lines?

[wenzeslaus]

According to blame, the internationalization fixes are the only ones which touched these lines. I already had these fixes in my new/moved code before.

[wenzeslaus]

As already mentioned above, automated testing would be a major undertaking which is itself dependent on more refactoring, so while this misses dedicated tests, this is ready to be merged.

Here is a diff of the old and new lock_mapset function:

--- grass.py	2024-09-24 20:36:05.684824023 -0400
+++ data.py	2024-09-24 20:36:02.948946546 -0400
@@ -1,57 +1,60 @@
-def lock_mapset(mapset_path, force_gislock_removal, user):
-    """Lock the mapset and return name of the lock file
+def lock_mapset(mapset_path, force_lock_removal, message_callback):
+    """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 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 function is called when forcibly unlocking the
+    mapset.
+
+    Assumes that the runtime is set up (specifically that GISBASE is in
+    the environment).
     """
     if not os.path.exists(mapset_path):
-        fatal(_("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():
-            # GTC %s is mapset's folder path
-            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),
             )
-        fatal(error)
+        raise MapsetLockingException(error)
     # Check for concurrent use
     lockfile = os.path.join(mapset_path, ".gislock")
-    ret = call([gpath("etc", "lock"), lockfile, "%d" % os.getpid()])
+    locker_path = os.path.join(os.environ["GISBASE"], "etc", "lock")
+    ret = subprocess.run(
+        [locker_path, lockfile, "%d" % os.getpid()], check=False
+    ).returncode
     msg = None
     if ret == 2:
-        if not force_gislock_removal:
+        if not force_lock_removal:
             msg = _(
-                "%(user)s is currently running GRASS in selected mapset"
-                " (file %(file)s found). Concurrent use not allowed.\n"
+                "{user} is currently running GRASS in selected mapset"
+                " (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..."
-            ) % {"user": user, "file": lockfile}
-
+                " (assuming your have sufficient access permissions)."
+                " Confirm in a process manager "
+                "that there is no other process using the mapset."
+            ).format(user=Path(lockfile).owner(), file=lockfile)
         else:
-            try_remove(lockfile)
-            message(
+            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} found), but forcing to launch GRASS anyway..."
+                ).format(user=Path(lockfile).owner(), file=lockfile)
             )
+            gs.try_remove(lockfile)
     elif ret != 0:
-        msg = (
-            _("Unable to properly access '%s'.\nPlease notify system personnel.")
-            % lockfile
-        )
+        msg = _(
+            "Unable to properly access lock file '{name}'.\n"
+            "Please resolve this with your system administrator."
+        ).format(name=lockfile)
 
     if msg:
-        raise Exception(msg)
-    debug(
-        "Mapset <{mapset}> locked using '{lockfile}'".format(
-            mapset=mapset_path, lockfile=lockfile
-        )
-    )
+        raise MapsetLockingException(msg)
     return lockfile

[echoix]

Thanks for the manual diff, it's so easy to follow!