Skip to content

Comments

Use official Cygwin install action and run bash.exe from batch file#14

Merged
ChrisJefferson merged 6 commits intogap-actions:masterfrom
stertooy:official-action-and-batch
May 7, 2025
Merged

Use official Cygwin install action and run bash.exe from batch file#14
ChrisJefferson merged 6 commits intogap-actions:masterfrom
stertooy:official-action-and-batch

Conversation

@stertooy
Copy link
Contributor

@stertooy stertooy commented Apr 28, 2025

This is pretty much a complete overhaul. To summarise the changes (more or less in order of importance)

  • Cygwin is now installed using the official cygwin-install-action.
  • A batch file bash.bat is created which sets CHERE_INVOKING to 1 and runs bash.exe with the options --login -e -o pipefail -o igncr while ignoring other arguments. That batch file is added to PATH.
  • Git is configured with core.autocrlf input.
  • The default list of packages has been shortened to only include those needed to build and run GAP (I think).
  • Inputs are now of the form input-var instead of INPUT_VAR.

This should allow running other actions with shell: bash instead of needing shell: C:\cygwin64\bin\bash.exe --login --norc -o igncr '{0}'. Which in turn should mean that separate Cygwin branches for actions are no longer needed.


I'm not overly fond of having to use a batch file, but I wasn't able to get this to work otherwise. Some remarks on this:

  • Configuring git wasn't enough to fix the whole crlf mess. Any multi-line run (i.e. steps with run | ...) in the action.yml file were still written to a .sh with \r's present, causing them to fail when ran from bash.
  • Putting igncr to SHELLOPTS worked for the most part, but caused problems when building GAP. Probably related to the following warning found in cygwin-install-action's README:

Warning: Putting igncr in the SHELLOPTS environment variable seems like it should have the same effect, but this can have unintended side-effects (by default, SHELLOPTS is a shell variable and moving it to the environment causes all shell options to propagate to child shells).

  • By default, GitHub runs bash with the options --noprofile --norc -e -o -pipefail. It seems to be really necessary to run Cygwin with --login present and --noprofile --norc not present to get it to work properly.

  • The batch script currently ignores all arguments given to bash except the last one, because that is usually (always?) the shell script that bash should run. This, uhh, doesn't feel very safe to me, so if someone has a better solution, I'd really welcome it.

EDIT: most of this stems from trying out the ideas posted in gap-actions/setup-gap#18

@stertooy stertooy changed the title Use official Cygwin install action and run bash.exe from bash script Use official Cygwin install action and run bash.exe from batch file Apr 28, 2025

- `PKGS_TO_INSTALL`:
- A comma separated list of packages to install
- default: `'wget,git,gcc-g++,gcc-core,m4,libgmp-devel,make,automake,libtool,autoconf,autoconf2.5,zlib-devel,libreadline-devel,libmpc-devel,libmpfr-devel,xdg-utils'`
Copy link
Member

Choose a reason for hiding this comment

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

m4 might be needed to run autoconf, no? GAP itself doesn't need automake, but several packages do. So perhaps those two should be retained?

I don't remember why xdg-utils is on there; I have a niggling thought that I tried to removing it in https://github.com/gap-system/gap-windows and @ChrisJefferson stopped from doing it (if he did, then certainly for good reasons), but I just don't remember details.

So fine by me to remove it and see what happens. But I think this strongly suggests that we should document for everything in the list why we have it on there.

Regarding libmpc & libmpfr: those are needed by the float package but if someone wants to build that, it'd be fair to require them to use the extra-pkgs-to-install option.

Of course we can say the same for automake and indeed remove it.

(I was about to suggest that we then also can remove libtool as GAP doesn't need it ; but of course if we want to be able to build GAP from older branches that still use it, we need it; so best to keep it for now).

(Of course none of automake/autoconf/libtool are required to build GAP from a release tarball, they are only needed for building git snapshots.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

m4 might be needed to run autoconf, no?

It is still being installed, exactly because it is a dependency of autoconf. I didn't see a reason to include it explicitly, though such reason could still exist of course.

As for which packages to include/exclude: I don't have a strong opinion on this, but my personal preference is to try and be pragmatic. If automake is used to build many GAP packages — whatever "many" may be — I would include it, while I would exclude things used by only few GAP packages (e.g. libmpc and libmpfr).

I don't remember why xdg-utils is on there; I have a niggling thought that I tried to removing it in https://github.com/gap-system/gap-windows and @ChrisJefferson stopped from doing it (if he did, then certainly for good reasons), but I just don't remember details.

Looking at ba0af81, it is used "to open external files". I don't quite know what that means, but if that includes some of the core functionality of GAP I would definitely keep the package, whereas if it is only needed by few packages or only by https://github.com/gap-system/gap-windows it could (should?) be supplied via extra-pkgs-to-install instead.

Copy link
Contributor

@ChrisJefferson ChrisJefferson Apr 30, 2025

Choose a reason for hiding this comment

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

xdg-utils is needed for xdg-open, which is used for OpenExternal, which opens a file in an external program.

Also (and I don't deny this is horrible, and not documented), at the moment we use the presence of xdg-open to check we have a "fairly complete" linux base, rather than the old super-minimal linux distro. This code is mainly here while we moved to "assume a full cygwin base system" instead of trying to get away with just the cygwin dll.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added xdg-utils back in.

As for the "completeness" of the linux base, I suppose you're referencing the IS_ARCH_WINDOWS-function. It indeed returns true when xdg-utils is not installed, and returns false when it is installed.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me now

Copy link
Member

Choose a reason for hiding this comment

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

automake is used by five distributed packages:

$ ls pkg/*/Makefile.am
pkg/anupq/Makefile.am  pkg/float/Makefile.am  pkg/nq/Makefile.am  pkg/simpcomp/Makefile.am

However, automake is only needed if one is not installing these packages from a release tarball. So it only is relevant for the CI tests of those specific packages... So we might as well drop it from the list of installed packages, and let those packages use extra-pkgs-to-install (if they even ever want to add the cygwin variant to their CI tests...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I wasn't aware of this when I made #16 (which is now merged), so automake in. I added it because Digraphs uses automake in its autogen.sh. If you feel strongly about getting rid of automake from this list @stertooy, then I can happily go back to adding automake via extra-pkgs-to-install.

@stertooy
Copy link
Contributor Author

I've added --norc back in. It seems only --noprofile was causing problems.

@ChrisJefferson
Copy link
Contributor

In general, I'm in favour of this, and moving to the official cygwin github, as long as nothing breaks -- this looks reasonable, and we might have to try it and see if everything still works, and back it out if something ends up breaking and we can't fix it.

@stertooy
Copy link
Contributor Author

stertooy commented May 3, 2025

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

@ChrisJefferson
Copy link
Contributor

I'm happy with this. I'm currently on holiday without a laptop, until Monday, so if everyone else is happy, let's wait till then and merge it (just in case something breaks, I don't expect it).

Co-authored-by: Max Horn <max@quendi.de>
@stertooy stertooy marked this pull request as ready for review May 6, 2025 15:19
@ChrisJefferson ChrisJefferson merged commit 624551e into gap-actions:master May 7, 2025
2 checks passed
@stertooy stertooy deleted the official-action-and-batch branch September 1, 2025 19:17
@wilfwilson
Copy link
Contributor

This is brilliant! Thanks for your hard work @stertooy, I really wished I could come up with this kind of a solution back when I was hacking around with this stuff in 2021/2022.

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.

4 participants