Don't start a session when loading workspace and --nointeract is given

[markuspf]

Fixes #2832

[dimpase]

OK, great, this fixes the issue #2832
and makes the corr. test in #2833 pass.

[codecov]

Codecov Report

Merging #2840 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2840      +/-   ##
==========================================
- Coverage   76.05%   76.05%   -0.01%     
==========================================
  Files         480      480              
  Lines      241022   241116      +94     
==========================================
+ Hits       183310   183378      +68     
- Misses      57712    57738      +26

Impacted Files	Coverage Δ
lib/session.g	58.49% <100%> (+1.62%)	⬆️
hpcgap/lib/hpc/stdtasks.g	63.42% <0%> (-0.77%)	⬇️
src/hpc/threadapi.c	43.55% <0%> (+0.1%)	⬆️
src/objset.c	84.71% <0%> (+0.22%)	⬆️
src/io.c	60.6% <0%> (+0.89%)	⬆️

[fingolfin]

That is clearly correct. Of course a test would be nice, but I guess we'll get one as a side effect of @dimpase's work?

[embray]

Somehow this still appears to be broken, though I don't fully understand why. First off, --norepl / --nointeract are not parsed by the kernel in system.c during InitSystem. That shouldn't be a problem though since system.g installs the post-restore function that re-scans the command-line arguments, and includes handling for --nointeract and --norepl. That should be getting run before SESSION(), so we should have CommandLineOptions.norepl = true at this point. But somehow that appears to be failing...

I can reproduce this at the command line, not just with libgap. When I run gap -m 64m -E -T --nointeract -L <path-to-workspace> test.gap, where test.gap just contains Print(GAPInfo);, if I don't pass the -L option it works as expected. But if I do, it still drops into the interactive REPL. This, despite GAPInfo containing:

...
  CommandLineOptions := rec(
      A := false,
      B := "",
      D := false,
      E := false,
      K := "0",
      L := "<path-to-my-workspace>",
      M := false,
      N := false,
      O := false,
      R := false,
      T := true,
      a := "0",
      alwaystrace := false,
      b := false,
      cover := "",
      e := false,
      enableMemCheck := false,
      f := false,
      g := 0,
      h := false,
      l := [ "<path-to-gap-root>" ],
      m := "64m",
      memprof := "",
      n := false,
      nointeract := true,
      norepl := true,
      o := "2g",
      p := false,
      prof := "",
      q := false,
      quitonbreak := false,
      r := false,
      s := "4g",
      x := "",
      y := "",
      z := "20" ),
  CommandLineOptionsPrev := [ rec(
          A := false,
          B := "",
          D := false,
          E := false,
          K := "0",
          L := "",
          M := false,
          N := false,
          O := false,
          R := false,
          T := true,
          a := "0",
          alwaystrace := false,
          b := false,
          cover := "",
          e := false,
          enableMemCheck := false,
          f := false,
          g := 0,
          h := false,
          l := [ "<path-to-gap-root>" ],
          m := "64m",
          memprof := "",
          n := false,
          nointeract := true,
          norepl := true,
          o := "581m",
          p := false,
          prof := "",
          q := true,
          quitonbreak := false,
          r := false,
          s := "581m",
          x := "",
          y := "",
          z := "20" ) ],

  KernelInfo := rec(
      BUILD_DATETIME := "01-Nov-2018",
      BUILD_VERSION := "4.10.0",
      COMMAND_LINE := [ "/path/to/gap", "-l",
          "<path-to-gap-root>", "-m", "64m", "-E", "-T", "--nointeract", "-L",
          "<path-to-my-workspace>", "test.gap" ],
...

[embray]

Scratch that--clearly my mistake. I'm running the GAP 4.10 release which does not have this fix. I assumed it did since this was merged > a month before the release came out.

[embray]

Now my question would just be if there's some obvious way to work around this using the 4.10 release without having to patch the sources...

[olexandr-konovalov]

@embray this went to master, while 4.10 release made from stable-4.10 branch. That branch was created off master more than a month ago. This is how GAP major releases are made.

[embray]

I understand that, though I wonder why this wasn't backported to the 4.10-stable branch.

[ChrisJefferson]

The boring answer is it wasn't fixing an (in our minds) serious issue, as this couldn't lead to incorrect reults and we tend to only merge minimum changes back to stable. Also, someone has to choose to mark the PR as to be merged back.

[embray]

Sure, it happens. Should I open a new issue or PR to backport this fix?

[olexandr-konovalov]

@embray no, I can cherry-pick it once we agree that it could be backported (it seems for me that it could)

[ChrisJefferson]

If you label this as should be backported to 4.10, then it should happen at some point (someone usually makes a pass over the tag every so often (I've actually just done it for you)

[embray]

Thanks--I don't think I can apply labels.

[fingolfin]

No worries, the labelling and backporting was already done, namely in 8cc677b

BTW @alex-konovalov I usually insert the SHA-1 of the backported commits into the PRs, that makes it much easier to double check that the backporting was really done. It would be great if you (and others doing backporting) could also do this (and yeah, we probably should "codify" this somewhere sigh)