Added IsLIBGAP constant #2528

sebasguts · 2018-06-07T11:10:59Z

This constant can be set to true in the initialize_libgap function (future work)
to stop GAP from starting an interactive session

This replaces #2527

fingolfin · 2018-06-07T13:53:13Z

lib/init.g

  SESSION();
+else
+  BIND_GLOBAL( "IsLIBGAP", true );


Let's use BIND_CONSTANT instead

fingolfin

Looks good to me, but let's wait to see if tests pass.

markuspf

LGTM

fingolfin · 2018-06-07T18:50:27Z

lib/init.g

@@ -1019,8 +1019,11 @@ end );
 if IsHPCGAP and THREAD_UI() then
  ReadLib("hpc/consoleui.g");
  MULTI_SESSION();
-else
+elif not IsBound( IsLIBGAP ) or IsLIBGAP = false then
+  BIND_CONSTANT( "IsLIBGAP", false );


Test fail, partially because of your .tst file... e.g. in HPC-GAP, we will not define IsLIBGAP. Better is to do this before line 1019:

if not IsBound(IsLIBGAP) then BIND_CONSTANT( "IsLIBGAP", false ); fi;

and then change this check here back to elif not IsLIBGAP, and drop the else part.

fingolfin · 2018-06-07T18:51:53Z

tst/testinstall/IsLIBGAP.tst

+##
+gap> START_TEST("IsLIBGAP.tst");
+gap> IsBound( IsLIBGAP );
+true


You may want to run ./gap tst/testinstall.g before submitting an updated version of this PR, to make sure it really works.

codecov · 2018-06-07T22:53:34Z

Codecov Report

Merging #2528 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2528   +/-   ##
=======================================
  Coverage    74.4%    74.4%           
=======================================
  Files         481      481           
  Lines      243488   243488           
=======================================
  Hits       181157   181157           
  Misses      62331    62331

Impacted Files	Coverage Δ
lib/init.g	`71.54% <ø> (ø)`	⬆️

sebasguts · 2018-06-07T22:54:24Z

I updated the PR.

I now initialize the IsLIBGAP variable before the systems gvar list is created. Otherwise the test will not work (and I think IsLIBGAP should be a system gvar).

fingolfin · 2018-06-10T23:13:01Z

lib/init.g

+
+#############################################################################
+##
+## Initialize the IsLIBGAP variable (if not done before). If this variable is false, an interactive


Please line-wrap this (I think at 78 chars... anyway, at the same width as the ####... line above.

fingolfin · 2018-06-10T23:13:17Z

lib/init.g

@@ -936,7 +947,6 @@ end );
 #T as system variables!
 ##

-


Please don't remove this line.

fingolfin · 2018-06-11T12:17:41Z

lib/init.g

+## Initialize the IsLIBGAP variable (if not done before). If this variable
+## is false, an interactive session will be started.
+## Otherwise no interactive session is started.
+## This should be done before the system GVAR list is created.


Change "should" -> "must" Or rather, just drop that line?

Anyway, let's not worry about this and merge as-is; I only mention it in case we need to update this again, say due to a build issue.

"Should" is the right word here. It works if you do it afterwards (however that should be done), but it will not be marked as a system gvar.

My point is more like this: There are countless variables which "should" or "must" be system vars, and none of them has a comment like this. It feels odd to point this out for just this variable. While I understand that this was a stumbling block during development of this PR, I don't quite see what it adds -- OK, hypothetically it might stop somebody from "moving" it around, but then, why would somebody do that? OTOH, if I just want to understand what it is, then reading this adds cognitive burden.

That said, I am not going to get stuck on that -- I merely point it out.

fingolfin · 2018-06-11T12:18:20Z

lib/init.g

+    MULTI_SESSION();
+  fi;
+elif IsLIBGAP = false then
+  BIND_CONSTANT( "IsLIBGAP", false );


Actually, this call to BIND_CONSTANT is redundant, isn't it? After all, you just checked that value.

It was supposed to make IsLIBGAP constant, which does not work in this case anyway, so I will remove it.

fingolfin · 2018-06-11T12:20:17Z

lib/init.g

@@ -1018,8 +1030,11 @@ end );

 if IsHPCGAP and THREAD_UI() then
  ReadLib("hpc/consoleui.g");
-  MULTI_SESSION();
-else
+  if IsLIBGAP = false then


Hmm... I was confused by the new logic for a moment (sorry for missing it in the previous version)... How about this

if IsLIBGAP then # GAP is used as a library, so don't start a session elif IsHPCGAP and THREAD_UI() then MULTI_SESSION(); else SESSION(); fi;

I am not sure actually. If we ever decide to have a HPC-GAP libGAP, the way it is now we could leave this as it is.

On the other hand, nevermind. I will change it to do have this logic.

This constant can be set to true in the initialize_libgap function (future work) to stop GAP from starting an interactive session

sebasguts · 2018-06-11T15:59:26Z

I removed the questionable sentence :).

sebasguts mentioned this pull request Jun 7, 2018

Added IsLIBGAP constant #2527

Closed

sebasguts requested review from fingolfin and markuspf June 7, 2018 11:11

fingolfin reviewed Jun 7, 2018

View reviewed changes

lib/init.g Outdated

SESSION();

else

BIND_GLOBAL( "IsLIBGAP", true );

Copy link

Member

fingolfin Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use BIND_CONSTANT instead

fingolfin mentioned this pull request Jun 7, 2018

Add --enable-libgap to build system #2526

Closed

sebasguts force-pushed the IsLIBGAP_const branch from 68add25 to bb2a824 Compare June 7, 2018 14:05

fingolfin approved these changes Jun 7, 2018

View reviewed changes

markuspf approved these changes Jun 7, 2018

View reviewed changes

fingolfin reviewed Jun 7, 2018

View reviewed changes

sebasguts force-pushed the IsLIBGAP_const branch from bb2a824 to ec2adae Compare June 7, 2018 22:53

olexandr-konovalov added the gapdays2018-spring Issues and PRs that arose at http://www.gapdays.de/gap-jupyter-days2018 label Jun 8, 2018

fingolfin reviewed Jun 10, 2018

View reviewed changes

fingolfin requested changes Jun 10, 2018

View reviewed changes

lib/init.g Outdated

@@ -936,7 +947,6 @@ end );

#T as system variables!

##

Copy link

Member

fingolfin Jun 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't remove this line.

sebasguts force-pushed the IsLIBGAP_const branch from ec2adae to 6619d52 Compare June 11, 2018 12:01

fingolfin reviewed Jun 11, 2018

View reviewed changes

fingolfin approved these changes Jun 11, 2018

View reviewed changes

sebasguts force-pushed the IsLIBGAP_const branch from 6619d52 to 863c24b Compare June 11, 2018 13:36

Added IsLIBGAP constant

b99b8ed

This constant can be set to true in the initialize_libgap function (future work) to stop GAP from starting an interactive session

sebasguts force-pushed the IsLIBGAP_const branch from 863c24b to b99b8ed Compare June 11, 2018 15:58

fingolfin approved these changes Jun 11, 2018

View reviewed changes

fingolfin merged commit d05bbd2 into gap-system:master Jun 12, 2018

fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jun 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added IsLIBGAP constant #2528

Added IsLIBGAP constant #2528

sebasguts commented Jun 7, 2018

fingolfin Jun 7, 2018

fingolfin left a comment

markuspf left a comment

fingolfin Jun 7, 2018

fingolfin Jun 7, 2018

codecov bot commented Jun 7, 2018 •

edited

Loading

sebasguts commented Jun 7, 2018

fingolfin Jun 10, 2018

fingolfin Jun 10, 2018

fingolfin Jun 11, 2018

sebasguts Jun 11, 2018

fingolfin Jun 11, 2018

fingolfin Jun 11, 2018

sebasguts Jun 11, 2018

fingolfin Jun 11, 2018

sebasguts Jun 11, 2018

sebasguts Jun 11, 2018 •

edited

Loading

sebasguts commented Jun 11, 2018

Added IsLIBGAP constant #2528

Added IsLIBGAP constant #2528

Conversation

sebasguts commented Jun 7, 2018

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

markuspf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 7, 2018 • edited Loading

Codecov Report

sebasguts commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebasguts Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

sebasguts commented Jun 11, 2018

codecov bot commented Jun 7, 2018 •

edited

Loading

sebasguts Jun 11, 2018 •

edited

Loading