Use official Cygwin install action and run bash.exe from batch file#14
Conversation
|
|
||
| - `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'` |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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-utilsis 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
|
I've added |
|
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. |
|
As a "proof-of-concept": this workflow run uses the |
|
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>
|
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. |
This is pretty much a complete overhaul. To summarise the changes (more or less in order of importance)
cygwin-install-action.bash.batis created which setsCHERE_INVOKINGto1and runsbash.exewith the options--login -e -o pipefail -o igncrwhile ignoring other arguments. That batch file is added to PATH.core.autocrlf input.input-varinstead ofINPUT_VAR.This should allow running other actions with
shell: bashinstead of needingshell: 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:
crlfmess. Any multi-line run (i.e. steps withrun | ...) in theaction.ymlfile were still written to a.shwith\r's present, causing them to fail when ran frombash.igncrtoSHELLOPTSworked for the most part, but caused problems when building GAP. Probably related to the following warning found incygwin-install-action'sREADME:By default, GitHub runs bash with the options
--noprofile --norc -e -o -pipefail. It seems to be really necessary to run Cygwin with--loginpresent and--noprofile --norcnot 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