embray / gappy

Python interface to GAP
GNU General Public License v3.0
13 stars 5 forks source link

workspace=None cannot be overwritten #16

Open dimpase opened 3 years ago

dimpase commented 3 years ago

in https://trac.sagemath.org/ticket/31404 the GAP workspace management is broken (I guess it never worked). Here is how I am trying to fix it:

--- a/src/sage/libs/gap/libgap.pyx
+++ b/src/sage/libs/gap/libgap.pyx
@@ -253,7 +253,10 @@ cdef class SageGap(Gap):
     """

     def __init__(self):
-        workspace, _ = get_workspace()
+        workspace, isitthere = get_workspace()
+        if not isitthere:
+            workspace = None
         Gap.__init__(self, gap_root=gap_root(), workspace=workspace,
                      autoload=True)

@@ -283,6 +287,10 @@ cdef class SageGap(Gap):
             from . import converters

             workspace, workspace_is_up_to_date = get_workspace()
+            if self.workspace is None:
+                # Create new workspace
+                self.workspace = os.path.normpath(workspace)
             if self.workspace == os.path.normpath(workspace):
                 # Save a new workspace if necessary
                 if not workspace_is_up_to_date:

it might be necessary to pass Gap.__init__() the value workspace=None, as only then no attempt is made to load the non-existent workspace, something that leads to a crash in gappy (see here). (This explains the first chunk of the diff).

However, then I cannot overwrite self.workspace later, when I'd like to save a new workspace, so the second chunk leads to the error

AttributeError: attribute 'workspace' of 'gappy.core.Gap' objects is not writable

An obvious way to handle this would be to change gappy code to have one more constructor parameter to tell it whether workspace is legit, rather than try to use workspace=None to test this.

Can you think of anything better (avoiding modifying gappy) ?

dimpase commented 3 years ago

Together with #17, the following solves the problem for Sage

--- a/src/sage/libs/gap/libgap.pyx
+++ b/src/sage/libs/gap/libgap.pyx
@@ -253,8 +253,8 @@ cdef class SageGap(Gap):
     """

     def __init__(self):
-        workspace, _ = get_workspace()
-        Gap.__init__(self, gap_root=gap_root(), workspace=workspace,
+        workspace, isitthere = get_workspace()
+        Gap.__init__(self, gap_root=gap_root(), workspace=workspace, workspace_valid=isitthere,
                      autoload=True)

     cpdef initialize(self):
embray commented 4 months ago

Related also to #1