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

Cygwin CI: replace AppVeyor by GitHub Actions #28

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wilfwilson
Copy link
Member

@wilfwilson wilfwilson commented Jan 27, 2022

This does what the title says.

@wilfwilson wilfwilson changed the title CI: add a Cygwin job CI: add a GitHub Actions Cygwin job Jan 27, 2022
@fingolfin
Copy link
Member

Thank you!!

I just merged PR #27

if: ${{ !(github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) }}
runs-on: windows-latest
env:
CHERE_INVOKING: 1
Copy link
Member

Choose a reason for hiding this comment

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

Just to understand: why do we need this here? don't all the actions already set it?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is probably not necessary here. However, in my opinion it is not worth my time to remove it and wait to see if it works.

However, I think it would be required if one ever added a step to this job that ran some commands directly in the Cygwin bash shell. Keeping this (otherwise harmless) environment variable set means that adding such a step would be as seamless as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to rebase, I've commented it out for now.

Copy link
Member Author

@wilfwilson wilfwilson Jan 27, 2022

Choose a reason for hiding this comment

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

It seems it does need to be there, I don't really understand why, but if it's not then gap-actions/build-pkg and gap-actions/run-pkg-tests start in the GAP directory, and not in the ace directory, and so they fail at building the package and running its tests, respectively.

Here's an example of it running without the environment variable set: https://github.com/gap-packages/ace/runs/4965188634?check_suite_focus=true

@wilfwilson wilfwilson changed the title CI: add a GitHub Actions Cygwin job Cygwin CI: replace AppVeyor by GitHub Actions Jan 27, 2022
@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #28 (f87a9dc) into master (1496851) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #28   +/-   ##
=======================================
  Coverage   39.00%   39.00%           
=======================================
  Files          10       10           
  Lines        2761     2761           
=======================================
  Hits         1077     1077           
  Misses       1684     1684           

@wilfwilson
Copy link
Member Author

wilfwilson commented Jan 27, 2022

This PR is now in a state where the package builds and loads, but sadly the package does not work. Copied from https://github.com/gap-packages/ace/runs/4965449425?check_suite_focus=true ...

+ /home/runneradmin/gap/bin/gap.sh -l '/cygdrive/d/a/ace/ace/gaproot;' --quitonbreak tst/testall.g
 ┌───────┐   GAP 4.12dev built on 2022-01-27 11:34:29+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, GAPDoc 1.6.4, 
             IRREDSOL 1.4.3, LAGUNA 3.9.3, Polenta 1.3.9, Polycyclic 2.16, 
             PrimGrp 3.4.1, RadiRoot 2.8, 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'
---------------------------------------------------------------------------
Loading    ACE (Advanced Coset Enumerator) 5.3
GAP code by Greg Gamble <[email protected]> (address for correspondence)
       Alexander Hulpke (https://www.math.colostate.edu/~hulpke)
           [uses ACE binary (C code program) version: 3.001]
C code by  George Havas (http://staff.itee.uq.edu.au/havas)
           Colin Ramsay <[email protected]>
Co-maintainer: Max Horn <[email protected]>

                 For help, type: ?ACE
---------------------------------------------------------------------------
Architecture: x86_64-pc-cygwin-default64-kv8

testing: /cygdrive/d/a/ace/ace/gaproot/pkg/ace/tst/aceds.tst
########> Diff in /cygdrive/d/a/ace/ace/gaproot/pkg/ace/tst/aceds.tst:
24
# Input is:
i := ACEStart( fgens, rels, [ b, t ] );
# Expected output:
1
# But found:
#I  =========================================
#I  ACE 3.001        Thu Jan 27 11:39:54 2022
#I  Sorry. Process stream has died!
#I  You might like to try using 'ACEResurrectProcess(<i>);'
#I  Sorry. Process stream has died!
#I  You might like to try using 'ACEResurrectProcess(<i>);'
#I  Sorry. Process stream has died!
#I  You might like to try using 'ACEResurrectProcess(<i>);'
Error, failed to find any more of line (iostream dead?)

########
########> Diff in /cygdrive/d/a/ace/ace/gaproot/pkg/ace/tst/aceds.tst:
26
# Input is:
stats:=ACEStats( i );; Unbind(stats.cputime); stats;
# Expected output:
rec( activecosets := 80, cputimeUnits := "10^-2 seconds", index := 8\
0, 
  maxcosets := 123, totcosets := 187 )
# But found:
Error, Variable: 'i' must have a value
Error, Variable: 'stats' must have a value
Error, Variable: 'stats' must have a value
########
########> Diff in /cygdrive/d/a/ace/ace/gaproot/pkg/ace/tst/aceds.tst:
29
# Input is:
ACEDeleteSubgroupGenerators( i, [ t ] );             
# Expected output:
[ b ]
# But found:
Error, Variable: 'i' must have a value
########
########> Diff in /cygdrive/d/a/ace/ace/gaproot/pkg/ace/tst/aceds.tst:
34
# Input is:
ACEDeleteSubgroupGenerators( i, [ 2 ] );
# Expected output:
#I  ** ERROR (continuing with next line)
#I     first argument out of range
#I  start = yes, continue = yes, redo = yes
#I  ***
#I  INDEX = 640 (a=640 r=1673 h=1 n=1673; l=2 c=0.00; m=811 t=1672)
#I    #--- ACE 3.001: Run Parameters ---
#I  Group Name: G;
#I  Group Relators: (s)^2, (t)^2, (u)^2, (v)^2, (d)^2, aad, (b)^3, (st)^2, 
#I    (uv)^2, (su)^2, (sv)^2, (tu)^2, (tv)^2, Asau, Atav, Auas, Avat, Bvbu, 
#I    dAda, dBdb, (ds)^2, (dt)^2, (du)^2, (dv)^2, Bubvu, Bsbdvt, Btbvuts, 
#I    (ab)^5;
#I  Subgroup Name: H;
#I  Subgroup Generators: b;
#I    #---------------------------------
[ b ]
# But found:
Error, Variable: 'i' must have a value
########
########> Diff in /cygdrive/d/a/ace/ace/gaproot/pkg/ace/tst/aceds.tst:
# Input is:
ACEQuit(i);
# Expected output:
# But found:
Error, Variable: 'i' must have a value
########
      94 ms (63 ms GC) and 2.49MB allocated for aceds.tst
-----------------------------------
total        94 ms (63 ms GC) and 2.49MB allocated
              5 failures in 1 of 1 files

#I  Errors detected while testing

@fingolfin
Copy link
Member

README.md also references AppVeyor, the badge needs to be removed

Weird that it fails here and works in AppVeyor.

@wilfwilson
Copy link
Member Author

Yeah, weird, but also not too surprising, because I imagine the AppVeyor and GitHub Actions versions of Cygwin are somewhat different.

@fingolfin
Copy link
Member

Interesting: in order to debug this, I added as a hack a commit which executes SetInfoLevel(InfoACE, 4); before the tests are run. With that the commands seemed to work (but the tests of course failed due to the extra output). See https://github.com/gap-packages/ace/runs/5180258469?check_suite_focus=true.

@wilfwilson
Copy link
Member Author

No there are still proper failures, I think:

Architecture: x86_64-pc-cygwin-default64-kv8

testing: /cygdrive/d/a/ace/ace/gaproot/pkg/ace/tst/aceds.tst
########> Diff in /cygdrive/d/a/ace/ace/gaproot/pkg/ace/tst/aceds.tst:
24
# Input is:
i := ACEStart( fgens, rels, [ b, t ] );
# Expected output:
1
# But found:
#I  ACE 3.001        Mon Feb 14 07:56:07 2022
#I  =========================================
#I  Host information:
#I    name = fv-az41-727
#I  ToACE> Group Generators: abstuvd;
#I  Sorry. Process stream has died!
#I  You might like to try using 'ACEResurrectProcess(<i>);'
#I  =========================================
#I  ACE 3.001        Mon Feb 14 07:56:07 2022
#I  Sorry. Process stream has died!
#I  You might like to try using 'ACEResurrectProcess(<i>);'
#I  Sorry. Process stream has died!
#I  You might like to try using 'ACEResurrectProcess(<i>);'
Error, failed to find any more of line (iostream dead?)

########
########> Diff in /cygdrive/d/a/ace/ace/gaproot/pkg/ace/tst/aceds.tst:
26
# Input is:
stats:=ACEStats( i );; Unbind(stats.cputime); stats;
# Expected output:
rec( activecosets := 80, cputimeUnits := "10^-2 seconds", index := 8\
0, 
  maxcosets := 123, totcosets := 187 )
# But found:
Error, Variable: 'i' must have a value
Error, Variable: 'stats' must have a value
Error, Variable: 'stats' must have a value
########
########> Diff in /cygdrive/d/a/ace/ace/gaproot/pkg/ace/tst/aceds.tst:
29
# Input is:
ACEDeleteSubgroupGenerators( i, [ t ] );             
# Expected output:
[ b ]
# But found:
Error, Variable: 'i' must have a value
########

i.e. ACEStart doesn't work.

@fingolfin
Copy link
Member

Indeed. I think I must have looked at the wrong log 😂

@fingolfin
Copy link
Member

Hm, tmate access doesn't seem to work :-(

@ChrisJefferson
Copy link
Member

Just for information, I hacked at this a bit (you can see my code at ChrisJefferson ).

The code runs fine locally on my machine, and also in a release made from GAP master.

My best guess as to what is going wrong on github actions is that 'ace' believes there is nothing to read from stdin, so immediately exits. However, I cannot figure out remotely why that would be the case.

@ChrisJefferson
Copy link
Member

I had another hack and this, and tried fixing some things in core GAP, or actions, I hoped might be affecting things. They didn't fix anything, and I can't reproduce this, so unfortunately I think we will have to leave it as a mystery.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants