Skip to content
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

Restoring saved workspaces broken in GAP 4.12? #5014

Closed
zickgraf opened this issue Aug 19, 2022 · 16 comments · Fixed by #5573
Closed

Restoring saved workspaces broken in GAP 4.12? #5014

zickgraf opened this issue Aug 19, 2022 · 16 comments · Fixed by #5573
Labels
kind: bug Issues describing general bugs, and PRs fixing them
Milestone

Comments

@zickgraf
Copy link
Contributor

When creating a workspace and trying to load it with GAP 4.12, the following error occurs:

Panic in src/saveload.c:618: This workspace is not compatible with GAP kernel (4.12.0, present: 4.12.0 with readline)

For me this looks like the version number is not handled consistently (once with "with readline" and once without). If you cannot reproduce, I will of course provide more details.

 ┌───────┐   GAP 4.12.0 of 2022-08-18
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-default64-kv8
 Configuration:  gmp 6.2.1, GASMAN, readline
 Loading the library and packages ...
 Packages:   AClib 1.3.2, Alnuth 3.2.1, AtlasRep 2.1.4, AutoDoc 2022.07.10, AutPGrp 1.11, Browse 1.8.14, 
             CaratInterface 2.3.4, CRISP 1.4.5, Cryst 4.1.25, CrystCat 1.1.10, CTblLib 1.3.4, FactInt 1.6.3, 
             FGA 1.4.0, Forms 1.2.8, GAPDoc 1.6.6, genss 1.6.7, IO 4.7.2, IRREDSOL 1.4.3, LAGUNA 3.9.5, orb 4.8.5, 
             Polenta 1.3.10, Polycyclic 2.16, PrimGrp 3.4.2, RadiRoot 2.9, recog 1.3.2, ResClasses 4.7.3, 
             SmallGrp 1.5, Sophus 1.27, SpinSym 1.5.2, TomLib 1.2.9, TransGrp 3.6.3, utils 0.76
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
@ChrisJefferson
Copy link
Contributor

Just for confirmation, I assume you downloaded the tarball from www.gap-system.org?

@zickgraf
Copy link
Contributor Author

Just for confirmation, I assume you downloaded the tarball from www.gap-system.org?

Yes, and this gave me https://github.com/gap-system/gap/releases/download/v4.12.0/gap-4.12.0.tar.gz

@ChrisJefferson
Copy link
Contributor

It would be good some more details -- I tried reproducing this but it seemed to work fine (I'm on Ubuntu in WSL). A full terminal cut, like this, would be helpful if yours is different:

gap-4.12.0/ $ ./gap
 ┌───────┐   GAP 4.12.0 of 2022-08-18
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-default64-kv8
 Configuration:  gmp 6.2.1, GASMAN, readline
 Loading the library and packages ...
 Packages:   AClib 1.3.2, Alnuth 3.2.1, AtlasRep 2.1.4, AutoDoc 2022.07.10, AutPGrp 1.11, CRISP 1.4.5, Cryst 4.1.25,
             CrystCat 1.1.10, CTblLib 1.3.4, FactInt 1.6.3, FGA 1.4.0, Forms 1.2.8, GAPDoc 1.6.6, genss 1.6.7,
             IO 4.7.2, IRREDSOL 1.4.3, LAGUNA 3.9.5, orb 4.8.5, Polenta 1.3.10, Polycyclic 2.16, PrimGrp 3.4.2,
             RadiRoot 2.9, recog 1.3.2, ResClasses 4.7.3, SmallGrp 1.5, Sophus 1.27, SpinSym 1.5.2, TomLib 1.2.9,
             TransGrp 3.6.3, utils 0.76
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> x := "test";
"test"
gap> SaveWorkspace("ws");
true
gap>
gap-4.12.0/ $ ./gap -L ws
 ┌───────┐   GAP 4.12.0 of 2022-08-18
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-default64-kv8
 Configuration:  gmp 6.2.1, GASMAN, readline
 Loaded workspace: ws
 Packages:   AClib 1.3.2, Alnuth 3.2.1, AtlasRep 2.1.4, AutoDoc 2022.07.10, AutPGrp 1.11, CRISP 1.4.5, Cryst 4.1.25,
             CrystCat 1.1.10, CTblLib 1.3.4, FactInt 1.6.3, FGA 1.4.0, Forms 1.2.8, GAPDoc 1.6.6, genss 1.6.7,
             IO 4.7.2, IRREDSOL 1.4.3, LAGUNA 3.9.5, orb 4.8.5, Polenta 1.3.10, Polycyclic 2.16, PrimGrp 3.4.2,
             RadiRoot 2.9, recog 1.3.2, ResClasses 4.7.3, SmallGrp 1.5, Sophus 1.27, SpinSym 1.5.2, TomLib 1.2.9,
             TransGrp 3.6.3, utils 0.76
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> x;
"test"

@zickgraf
Copy link
Contributor Author

Aaaah, the way I am calling GAP in a script seems to disable readline:

#!/bin/sh
#                                                           Frank L�beck
#
# Use this script inside the $GAPROOT directory (the top directory
# containing 'bin', 'lib', 'pkg' and so on as subdirectories) to 
# generate a workspace file:
#     bin/wsgap4

echo Creating new workspace file ... 

# the -r option avoids to store user specific settings in the workspace
current/bin/gap.sh -r <<EOF

# load here all packages you want to include in the standard workspace
LoadPackage("io");

# load help book infos with a dummy help query
??blablfdfhskhks

# a small trick to make everything sensible available to the TAB completion
function() local a; for a in NamesGVars() do if ISB_GVAR(a) then
VAL_GVAR(a); fi;od;end;
last();

# save the workspace
SaveWorkspace("current/bin/wsgap4");

quit;
EOF

I checked an old workspace, there readline support was not disabled by this way of calling GAP. Do you have a suggestion on how to call GAP with multiline input from a script and keep readline support enabled? (I tried the -E flag which does not seem to make a difference).

In any case, thanks for the super fast reply and sorry for the false alarm!

@ChrisJefferson
Copy link
Contributor

ChrisJefferson commented Aug 19, 2022

In terms of fixing your issue, you could replace running GAP with something like this:

current/bin/gap.sh -c 'LoadPackage("io");' -c '??blablfd' -c '(function() local a; for a in NamesGVars() do if ISB_GVAR(a) then VAL_GVAR(a); fi;od;end)();' -c 'SaveWorkspace("current/bin/wsgap4");' -c 'QuitGap();'

However, leave this issue open for now, as I'm interested why this used to work, and now doesn't, particularly if (based on @frankluebeck 's name), this is a script multiple people might be using.

@ChrisJefferson
Copy link
Contributor

Having looked through history, this is caused by system.c line 826, introduced by @fingolfin , who (quite reasonably) stopped us using readline when GAP is not run from a proper terminal. It's just unfortunate that interacts with workspaces, but I think might be something we don't want to change.

@frankluebeck
Copy link
Member

I will update the rsync distribution to GAP 4.12 and then investigate. Thanks for the hints already given.

@frankluebeck
Copy link
Member

I agree that it is sensible to disable readline by default when GAP's input is not a terminal. But why does GAP ignore an explicit -E argument and still does not load readline?

For the rsync distribution I have now changed the script that generates the workspace (the script does no longer redirect the standard input but just reads a file): see this commit

@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Sep 1, 2022
@fingolfin fingolfin added this to the GAP 4.12.1 milestone Sep 1, 2022
@ChrisJefferson
Copy link
Contributor

One minor point, actually -E disables readline (this happens in 4.11 and 4.12), as readline is enabled by default.

If people want this, and want something less confusing, we could easily add --readline and --no-readline, so make clear how to turn readline on and off (and make --readline force readline on even when reading from a file).

@fingolfin
Copy link
Member

This is a general concern I have with the GAP command line options: a lot of them toggle behavior, which can make it difficult to reliable ensure things are really turned on or off (think of gap being a shell script which already sets e.g. -E. So having longform options to more (all?) GAP options which normally toggle behavior would be nice. I'd tend to names like --enable-readline or --disable-readline. Of course they are not nice for interactive usage, but for scripts can be super handy for the reason I just gave

@frankluebeck
Copy link
Member

Indeed, toggling options are only useful when the default is fixed (which was the case in "old" GAP versions).

@fingolfin
Copy link
Member

So, how do we want to deal with this?

For saveload.c I think we really just want to store whether it is available ? The check was added in PR #2720 to fix two crash reports. But perhaps we can do this differently...

E.g. first we use a patch like this:

diff --git a/src/saveload.c b/src/saveload.c
index 606b24ee8..2d6df4507 100644
--- a/src/saveload.c
+++ b/src/saveload.c
@@ -499,9 +499,9 @@ static Char * GetKernelDescription(void)
 {
     static Char SyKernelDescription[256];
     strcpy(SyKernelDescription, SyKernelVersion);
-    if (SyUseReadline) {
-        strcat(SyKernelDescription, " with readline");
-    }
+#ifdef HAVE_LIBREADLINE
+    strcat(SyKernelDescription, " with readline");
+#endif
     return SyKernelDescription;
 }
 

Next, when restoring savespaces, we ignore the actual -E command line arguments, and instead initialize SyUseReadline to match the value of GAPInfo.UseReadline in the workspace being restored.

As an alternative to the second part, we could change the code in lib/cmdledit.g to use CallAndInstallPostRestore in order to setup GAPInfo.UseReadline and GAPInfo.CommandLineEditFunctions if necessary also after restoring a workspace?

@ChrisJefferson
Copy link
Contributor

My concern (and it's hard to know without tracing the code) is if this will reintroduce the bugs we were fixing by adding this -- there are some separate things, did we build with readline, and are we actually using it, and the question is will GAP misbehave if, when we load in the saved workspace, we start using readline when we weren't before?

@fingolfin fingolfin modified the milestones: GAP 4.12.1, GAP 4.12.2 Oct 20, 2022
@fingolfin
Copy link
Member

Punting to 4.12.2 as nobody worked on a fix.

mohamed-barakat added a commit to homalg-project/homalg_starter that referenced this issue Oct 21, 2022
@mohamed-barakat
Copy link
Member

Now that I managed to create the workspace using the workaround suggested by Frank the history saving does not work.

@fingolfin
Copy link
Member

Nobody has voiced intention to work on this, and so it won't be fixed in 4.12.2, and realistically not in any 4.12.x. I am going to punt it to the 4.13 milestone, but that won't fix it magically either, it'll just ensure it'll be revisited by me before that release.... We'll see what happens

@fingolfin fingolfin modified the milestones: GAP 4.12.2, GAP 4.13.0 Dec 15, 2022
fingolfin added a commit to fingolfin/gap that referenced this issue Jan 10, 2024
... by reverting a previous change that made us disable readline
when stdin is not connected to a terminal (see
gap-system#4495). While that still
seems useful, and avoids certain issues when piping GAP output
into files, it needs a better implication that avoids the issues
described in gap-system#5014

Also add a separate CI test suite target "testworkspace" for testing
for this issue, and potentially other workspace related issues.
fingolfin added a commit to fingolfin/gap that referenced this issue Jan 11, 2024
... by reverting a previous change that made us disable readline
when stdin is not connected to a terminal (see
gap-system#4495). While that still
seems useful, and avoids certain issues when piping GAP output
into files, it needs a better implication that avoids the issues
described in gap-system#5014

Also add a separate CI test suite target "testworkspace" for testing
for this issue, and potentially other workspace related issues.
fingolfin added a commit that referenced this issue Jan 12, 2024
... by reverting a previous change that made us disable readline
when stdin is not connected to a terminal (see
#4495). While that still
seems useful, and avoids certain issues when piping GAP output
into files, it needs a better implication that avoids the issues
described in #5014

Also add a separate CI test suite target "testworkspace" for testing
for this issue, and potentially other workspace related issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them
Projects
None yet
5 participants