search

gap-packages / io

GAP package IO to do input and output
https://gap-packages.github.io/io/
Other
14 stars 14 forks source link

Add a Cygwin job to the GitHub Actions CI #97

Closed wilfwilson closed 2 years ago

wilfwilson commented 3 years ago

This adds a Cygwin job to the GitHub Actions CI.

codecov[bot] commented 3 years ago

Codecov Report

Merging #97 (f48d002) into master (98b1c40) will decrease coverage by 0.43%. The diff coverage is n/a.

:exclamation: Current head f48d002 differs from pull request most recent head 3b9443a. Consider uploading reports for the commit 3b9443a to get more accurate results

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
- Coverage   61.65%   61.21%   -0.44%     
==========================================
  Files          15       15              
  Lines        4944     4888      -56     
==========================================
- Hits         3048     2992      -56     
  Misses       1896     1896
Impacted Files Coverage Δ
src/io.c 68.77% <0.00%> (-1.30%) :arrow_down:
wilfwilson commented 3 years ago

Actually, this isn't l looking so bad. The package seems to compile successfully, and indeed all of the test files run successfully (albeit slowly – which is not reflected in the timings shown by GAP) with the exception ontestgap.tst, in which all 50 of the duplicated lines gap> for f in files do Read(Filename(d[1], f)); od; fail.

Perhaps this failure can be explained or excused 🙂 

D:\a\io\io>C:\cygwin64\bin\bash -l -c "pkg-ci-scripts/run_tests.sh"
+ /home/runneradmin/gap/bin/gap.sh -l '/cygdrive/d/a/io/io/gaproot;' --quitonbreak --cover coverage/oFsRPm.coverage tst/testall.g
 ┌───────┐   GAP 4.12dev built on 2021-04-09 13:29:42+0000
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-cygwin-default64-kv8
 Configuration:  gmp 6.2.1, GASMAN, readline
 Loading the library and packages ...
 Packages:   AClib 1.3.2, Alnuth 3.1.2, AtlasRep 2.1.0, AutoDoc 2020.08.11,
             AutPGrp 1.10.2, CRISP 1.4.5, Cryst 4.1.23, CrystCat 1.1.9,
             CTblLib 1.3.1, FactInt 1.6.3, FGA 1.4.0, Forms 1.2.5,
             GAPDoc 1.6.4, genss 1.6.6, IO 4.7.0dev, IRREDSOL 1.4.1,
             LAGUNA 3.9.3, orb 4.8.3, Polenta 1.3.9, Polycyclic 2.15.1,
             PrimGrp 3.4.1, RadiRoot 2.8, recog 1.3.2, ResClasses 4.7.2,
             SmallGrp 1.4.2, Sophus 1.24, SpinSym 1.5.2, TomLib 1.2.9,
             TransGrp 3.0, utils 0.69
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
Architecture: x86_64-pc-cygwin-default64-kv8

testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/all.tst
      16 ms (0 ms GC) and 206KB allocated for all.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/bugfix.tst
     453 ms (0 ms GC) and 24.1MB allocated for bugfix.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/children.tst
    1781 ms (16 ms GC) and 4.25MB allocated for children.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/funcs.tst
       0 ms (0 ms GC) and 65.9KB allocated for funcs.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/sendstringbackground.tst
     938 ms (0 ms GC) and 60.8KB allocated for sendstringbackground.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/testgap.tst
########> Diff in /cygdrive/d/a/io/io/gaproot/pkg/io/tst/testgap.tst:
8
# Input is:
for f in files do Read(Filename(d[1], f)); od;
# Expected output:
# But found:
Error, Unable to read compressed file correctly: 6
########
########> Diff in /cygdrive/d/a/io/io/gaproot/pkg/io/tst/testgap.tst:
9
# Input is:
for f in files do Read(Filename(d[1], f)); od;
# Expected output:
# But found:
Error, Unable to read compressed file correctly: 6
########

[...etc., omitted...]

    5094 ms (188 ms GC) and 642MB allocated for testgap.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/timeout.tst
      78 ms (78 ms GC) and 111KB allocated for timeout.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/waitpid.tst
      78 ms (78 ms GC) and 55.3KB allocated for waitpid.tst
-----------------------------------
total      8438 ms (360 ms GC) and 671MB allocated
             50 failures in 1 of 8 files

#I  Errors detected while testing

##[error]Process completed with exit code 1.
wilfwilson commented 2 years ago

I've now cleaned up the implementation, so from that point of view, I think this PR would be ready to merge (although please feel to disagree).

However, the test failures that I observed last year in https://github.com/gap-packages/io/pull/97#issuecomment-816748793 still persist, in particular:

+ /home/runneradmin/gap/bin/gap.sh -l '/cygdrive/d/a/io/io/gaproot;' --quitonbreak tst/testall.g
 ┌───────┐   GAP 4.12dev built on 2022-01-25 16:15:03+0000
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-cygwin-default64-kv8
 Configuration:  gmp 6.2.1, GASMAN
 Loading the library and packages ...
 Packages:   AClib 1.3.2, Alnuth 3.1.2, AtlasRep 2.1.0, AutoDoc 2020.08.11, 
             AutPGrp 1.10.2, CRISP 1.4.5, Cryst 4.1.24, CrystCat 1.1.9, 
             CTblLib 1.3.2, FactInt 1.6.3, FGA 1.4.0, Forms 1.2.6, 
             GAPDoc 1.6.4, genss 1.6.6, IO 4.7.2, IRREDSOL 1.4.3, 
             LAGUNA 3.9.3, orb 4.8.4, Polenta 1.3.9, Polycyclic 2.16, 
             PrimGrp 3.4.1, RadiRoot 2.8, recog 1.3.2, ResClasses 4.7.2, 
             SmallGrp 1.4.2, Sophus 1.24, SpinSym 1.5.2, TomLib 1.2.9, 
             TransGrp 3.3, utils 0.72
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
Architecture: x86_64-pc-cygwin-default64-kv8

testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/all.tst
       0 ms (0 ms GC) and 207KB allocated for all.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/bugfix.tst
     234 ms (15 ms GC) and 24.1MB allocated for bugfix.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/children.tst
    1437 ms (0 ms GC) and 4.25MB allocated for children.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/funcs.tst
       0 ms (0 ms GC) and 65.9KB allocated for funcs.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/sendstringbackground.tst
     469 ms (0 ms GC) and 60.8KB allocated for sendstringbackground.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/testgap.tst
########> Diff in /cygdrive/d/a/io/io/gaproot/pkg/io/tst/testgap.tst:
8
# Input is:
for f in files do Read(Filename(d[1], f)); od;
# Expected output:
# But found:
Error, Unable to read compressed file correctly: 6
########
########> Diff in /cygdrive/d/a/io/io/gaproot/pkg/io/tst/testgap.tst:
9
# Input is:
for f in files do Read(Filename(d[1], f)); od;
# Expected output:
# But found:
Error, Unable to read compressed file correctly: 6
########

[...etc., omitted...]

    2828 ms (184 ms GC) and 643MB allocated for testgap.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/timeout.tst
      78 ms (63 ms GC) and 111KB allocated for timeout.tst
testing: /cygdrive/d/a/io/io/gaproot/pkg/io/tst/waitpid.tst
      63 ms (63 ms GC) and 55.5KB allocated for waitpid.tst
-----------------------------------
total      5109 ms (325 ms GC) and 672MB allocated
             50 failures in 1 of 8 files

#I  Errors detected while testing

Again, as with https://github.com/gap-packages/profiling/pull/91, perhaps @ChrisJefferson is best positioned to have some idea about what is going wrong?

ChrisJefferson commented 2 years ago

Irritatingly, this works fine if I run it from (a) Cygwin on my machine, or (b) A GAP installer from github.com/gap-system/gap

wilfwilson commented 2 years ago

That's frustrating! Perhaps as a compromise, we should therefore not run that test in the Cygwin job.

wilfwilson commented 2 years ago

@ChrisJefferson I have made the output of the failing tests a bit more verbose, and you can see the result for example at https://github.com/gap-packages/io/runs/4956614926?check_suite_focus=true

And the immediate culprit is clear!....

Unexpected contents of compressed file: [ "xyz\r\n", "abc\r\n" ]

whereas, supposedly, the expected line-by-line contents of the file are: [ "xyz\n", "abc\n" ].

The problem is the addition of the \r on Cygwin. Good old line endings, causing trouble!

So the question is: is this a bug or not? In other words, on Cygwin, should IO_WriteLine finish the line with \n (in which case there is currently a bug), or is \r\n okay (in which case the test simply needs making more accommodating)?

wilfwilson commented 2 years ago

Allowing [ "xyz\r\n", "abc\r\n" ] does the trick 🙂

ChrisJefferson commented 2 years ago

Seeing as these are all functions in IO, they don't tend to try to do any rewriting, so I think this is "fine" (it's certainly been behaviour for a long time).

wilfwilson commented 2 years ago

I'm going to merge this now so that we can us either 🙂. There is still intermittent trouble with the macOS job, but that is unrelated to this PR.