Towards v3

[stertooy]

This PR aims to tackle a few of the issues in this repo:

• Support using GAP releases instead of branch snapshots #24
  The input gap-version can now take the following inputs:
  • latest, which will build the latest release of GAP using the release tarball;
  • default or master or main, which will build the default branch of GAP using git clone;
  • branchname, which will build the corresponding branch using git clone;
  • tagname, which will build the corresponding tag using git clone;
  • X.Y.Z or vX.Y.Z, which will build the corresponding release of GAP (including pre-releases), using the release tarball;
  • X or vX or X.Y or vX.Y, which will build the most recent release of GAP whose version is of the form X.Y.Z (excluding pre-releases), using the release tarball.
• Musings on GAPROOT #31
  The action now adds the variable GAPROOT to GITHUB_ENV, so it can be picked up by other steps/actions in a workflow following this action.
  At the same time, it is also ensured that GAP is available from PATH by calling gap.
• Support building a particular GAP pull request #36
  The repository input allows downloading gap from other repositories.
• Problem with 32-bit ubuntu builds? #45
  Support for 32-bit is completely dropped.
• Towards setup-gap@v3 #47
  • The input GAP_PKGS_TO_CLONE has been removed completely. The input GAP_PKGS_TO_BUILD was renamed to gap-pkgs-to-build and is now intended only to build the packages that come with GAP (either from the release tarball or from running make bootstrap-pkg-full).
  • The HPCGAP input has been removed
  • ioand profiling have not been removed from the default gap-pkgs-to-build input, and json has in fact been added. This is a temporary measure, the plan is to let the actions using these packages take care of this themselves in the future.

Other noteworthy changes:

• The GAP_BOOTSTRAP input has been removed. It would have done nothing when GAP was built from a release tarball anyway, and setting this to minimal was rarely done as most workflows/actions/tests required at least some package not found in the minimal setup.
• The CI for this action has been expanded quite a bit.
• If Cygwin was setup via Use official Cygwin install action and run bash.exe from batch file setup-cygwin#14, this action no longer needs a separate cygwin branch (see Add support for building on Windows / Cygwin #18).
• An input token has been added, which should be supplied with ${{ secrets.GITHUB_TOKEN }}. This is used for authenticating with the GitHub API, which is very strongly rate-limited for unauthenticated users.

Remarks / Questions:

• Should setup-cygwin be a part of this action? Currently, to use this action on Windows, is needs to be preceded by something like

  - name: "Setup Cygwin"
    if: ${{ matrix.os == 'windows-latest' }}
    uses: stertooy/setup-cygwin@official-action-and-batch
  

  That works just fine, but we could also do this as part of this action instead.

• It is possible to build a particular PR (see Support building a particular GAP pull request #36) using the repository and gap-version inputs. But I could also allow gap-version to take an input of the form #XYZW to do this automatically. E.g. given the inputs repository: gap-system/gap and gap-version: #5956, the action would use the GitHub API to decide that it should build GAP using branch mh/proto-rereading from repository fingolfin/gap. Should I add such mechanism to this PR?

• This action uses the GitHub API to get releases... so it only works with releases available on GitHub. In practice, this is v4.10.0 and later.

• It appears that GAP versions < 4.13 do not work properly on the macos-latest runner. In particular:

  • Versions < 4.13 cannot find the system's GMP and will therefore always attempt to install the GMP version bundled with GAP.
  • Version 4.12 will succeed in installing with the bundled GMP, but GAP will often (though not always) crash when started. This always happens at

    Syntax warning: Unbound global variable in /Users/runner/gap/lib/primality.gi:\
    515
      Np:=(N-1)/p;
      ^^
    

    but the final error varies (segfault, must be a list, ...).
  • Versions < 4.12 cannot even install the bundled GMP, as they try to configure it with ABI=64 causing

    configure: error: ABI=64 is not among the following valid choices: 32
    

  I don't think it's worth looking into this since it only happens with old versions of GAP, but it might be prudent to warn users that we do not support the combination of gap <4.13 and MacOS runners?

• Installing the bundled GMP on Cygwin sometimes fails, e.g. with

  make[4]: Entering directory '/home/runneradmin/gap/extern/build/gmp/demos/calc'
  test -f calc.c || /bin/sh /home/runneradmin/gap/extern/gmp/ylwrap /home/runneradmin/gap/extern/gmp/demos/calc/calc.y y.tab.c calc.c y.tab.h `echo calc.c | sed -e s/cc$/hh/ -e s/cpp$/hpp/ -e s/cxx$/hxx/ -e s/c++$/h++/ -e s/c$/h/` y.output calc.output -- yacc -d 
  /home/runneradmin/gap/extern/gmp/ylwrap: line 176: yacc: command not found
  make[4]: *** [Makefile:456: calc.c] Error 127
  

  Again, I'm not sure whether it's worth looking into this any further, and we could just decide that Cygwin + --with-gmp=builtin is not supported?

• It would probably be a good idea for other actions to use the GAPROOT variable and/or run gap in the command line instead of $HOME/gap/gap. In particular, these other actions could then be used with a docker container that also adds gap to PATH and sets the GAPROOT variable.

As a "proof-of-concept": this workflow run uses the run-pkg-tests workflow, but with this PR instead of the usual setup-gap and with gap-actions/setup-cygwin#14 instead of the usual setup-cygwin.

[fingolfin]

THANK YOU for working on this!

Answering some (not all) questions (and using numbers to make replies perhaps a bit easier):

1. Should setup-cygwin be a part of this action?

   • would be OK by me, but maybe let's do this in a later step, this is already a big change?

2. This action uses the GitHub API to get releases.

   • how about we instead use a JSON/CSV/... which lists the releases and associated metadata (for now: the URL of the source archive, and perhaps a checksum for that)? Then we don't have API rate throttling issues and don't need the new token field. Updating this file would be easy enough, even manually, and could simply be added to the GAP release workflow documentation as another step (in dev/releases/README.md)

3. It appears that GAP versions < 4.13 do not work properly on the macos-latest runner.

   • I agree it's not important; but it also shouldn't be hard. The following should do it

   BP=$(brew --prefix)
   $gapdir/configure --with-gmp=$BP --with-readline=$BP/opt/readline

4. we could just decide that Cygwin + --with-gmp=builtin is not supported?

   • indeed, let's agree to that :-)

5. It would probably be a good idea for other actions to use the GAPROOT variable and/or run gap in the command line

   • sure, but that'll be a breaking change for those actions, too, i.e., they will then also have to go from vN to v(N+1). But I think this can wait until we merged this and released setup-gap@v3 ?

[stertooy]

To reply to @fingolfin's post:

1. Agreed.

2. I definitely prefer that suggestion over using the GitHub API (with a slight preference towards JSON over CSV). As for the data it should contain: in addition to the version and archive URL, I can currently obtain "directly" from the API whether or not a certain release is a pre-release or not, and which release is the "latest" one.

   We could add these to the file, but under the following (very mild?) assumptions this wouldn't be necessary:

   • versions of the form X.Y.Z are proper releases and versions of the form X.Y.Z-foo are pre-releases,
   • the "latest" release is always the one you would expect based on semver sorting (after filtering out pre-releases).

3. I'll give that a go and update the PR after some testing.

4. I'll try and remove this from the CI, then.

5. Yes, no need whatsoever to do this together with or before releasing setup-gap@v3. The other actions either define a GAPROOT variable as GAPROOT=${GAPROOT-$HOME/gap} (which will pick up the environment variable we now set), or flat out assume GAP is located at $HOME/gap, which is indeed where we install GAP.

[ChrisJefferson]

Hi, sorry, only just looking at this.

I will say our current cygwin plan is very much 'we do whatever is best for releases'. I think the set of people who want to build their own GAP on cygwin is the empty set :) If you are good enough at computers to use a unix-style command line, you would be using linux rather than cygwin nowadays!

This looks sensible to me. If it's easy to support older GAPs that's fine, but personally I wouldn't try too hard, anything before 4.13, if anyone wanted to update packages, I'd suggest they just get a newer GAP.

[fingolfin]

This is a really nice effort but now has a conflict. It would be good to get it over the finish line.

[stertooy]

Conflict resolved. Some remarks, still:

• The setup-cygwin action will need a new release, since this action depends somewhat on the changes made in Use official Cygwin install action and run bash.exe from batch file setup-cygwin#14.
• If the idea is still to get information on GAP releases from a JSON file instead of via git, then this file should exist somewhere? If that's not the case (yet): we could leave things as they are now and change this later, as this change should not affect users in any way.
• This PR removes the functionality for cloning and building additional packages. I'm not sure if it was decided yet to provide at additional action to provide this functionality, or to leave that entirely up to the user?

[fingolfin]

I tagged gap-actions/setup-cygwin@v2, and also documented the changes in the README.md (note that you also have full access to the repo, feel free to make such changes as well)

[fingolfin]

Regarding "information on GAP releases", what do we need? A list of releases (versions), each pointing to the URL of a tarball plus perhaps a SHA256 checksum?

I'd say we should create such a JSON file, but then we need an issue on gap-system/gap to remind us about it. But for now we can use the code here as-is, I think: if we switch to that JSON file later, that should be transparent to users anyway, right?

[stertooy]

We would need a file containing a list of the releases, with for each release:

• Version number
• URL for a release tarball
• (optional) checksum
• (optional) boolean saying whether this is a pre-release or not
• (optional) boolean saying whether this is the latest release or not

The last two could of course be determined from the version number instead of mentioned explicitly.

[stertooy]

It turns out that an action can access a repository's GITHUB_TOKEN without the need for it to be passed as input. So I implemented this change and removed the token input.

This should make the conversion from v2 to v3 easier, as well as the (eventual) change to getting release information from the JSON file instead of from the GitHub API.

[fingolfin]

Excellent, thank you so much!

There is one thing missing though: the README should have a section about "what's new in v3" that indicates the breaking changes and how to deal with them.

[stertooy]

I ended up waiting to release this since the JSON file progressed really quickly thanks to Joe.

I have an update ready to this PR which uses the JSON file, all that's missing is the "correct" URL to get the file from.

[fingolfin]

Thanks! A bunch if more remarks/requests -- but it'd be totally fine by me to merge this as-is and then perhaps taking care of these later.

[fingolfin]

Perhaps we can have an example for doing that somewhere in this document? I started writing a suggestion in my computer late last night but now just have my phone...

[fingolfin]

Based on your examples it seems one can also specify v4.11.1 (so tags?) or 4 and v4.10 (not tags). Could this be explained please?

Also "default branch" is not clear --presumably this means "GAP development version built from the default branch of the GAP repository"

I guess this hints at a translation key for people upgrading: "master" should become "default" and "stable-4.X" becomes "4.X" (or it stays, if branch names are supported? But better to suggest "4.X")

[stertooy]

I've tried explaining this in a bit more detail now. Let me know if there's anything still unclear.

EDIT: just realised I explained it in the Changes section, not in the inputs section. I added a line referring to the Changes section, but I'm not sure this is very user-friendly :/

[fingolfin]

(I started writing the below text last night, and adding it now so it is not completely lost, even though it may be out of date)

I think it would be good if this or another example showed how to replace GAP_PKGS_TO_CLONE. There are of course multiple ways.

A simple example might do

      - run: |
         cd $GAPROOT/pkg
         rm -rf cvec
         git clone https://github.com/gap-packages/cvec

It is actually weird that we have to delete an existing copy of the package. Perhaps better to install into a separate root directory, e.g. ~/.gap/pkg/, or even /tmp/gaproot/pkg (which several other GitHub actions already use for a symlink to the package being tested... perhaps we should formalize that here.. Indeed we could even ensure this is a GAP root here without fiddling with package dirs... E.g. by providing a ~/.gap/gap.ini file which calls ExtendRootDirectories).

Alternatively one could suggest using PackageManager (which also compiles),
though for some stuff one really needs the clone, so perhaps both

      - run: |
         gap -c 'LoadPackage("PackageManager"); InstallPackage("cvec"); QUIT;'

and also

      - run: |
         gap -c 'LoadPackage("PackageManager"); InstallPackage("https://github.com/gap-packages/cvec"); QUIT;'

could be useful examples ?

[stertooy]

I've added this as part of the second example now: one step using git clone in $HOME/.gap/pkg, a second using PackageManager.

[fingolfin]

Currently also build-pkg-docs sets GAPROOT in $GITHUB_ENV like that. But of course it "belongs" into setup-gap. So as a note to self: the next breaking version of build-pkg-docs probably should do away with setting GAPROOT (and GAP) in the environment.

[fingolfin]

I'd like to reiterate that all my ramblings are not meant to delay this PR which is huge and already does a lot. I'd be fine with merging, then turning my suggestions into issues (or just following them up with further PRs).

This might also make it easier to collaborate on certain fixes... but in the end, do it the way that works best for you, @stertooy -- I simply want to make sure you don't get frustrated by me constantly having additional ideas or requests ;-)

[stertooy]

No frustrations here, I appreciate the helpful feedback!

As for merging this PR: I think we should decide on the $GAP variable first, since that may impact (the development of) other actions.

Refinements to the README and switching to the JSON file should not impact users or other actions, so those could be done after a release.

[fingolfin]

Why do we now also build json?

I really would like to just get rid of gap-pkgs-to-build as soon as possible. I just made gap-actions/process-coverage#22 to this end

[stertooy]

It's needed for the release-pkg action.

I'm fine with opening a similar PR to release-pkg and removing this input here before releasing v3?

[fingolfin]

Sounds good to me. I'll work on getting gap-actions/process-coverage#22 ready and once that is good we can replicate it for release-pkg.

In the meantime, none of this needs to block this PR, as long as we still remove this before v3 is released.