Hi, Thank you for working on this!
On Sat, 13 Apr 2024 at 05:12, Andres Freund <and...@anarazel.de> wrote: > > Hi, > > We have CI support for mingw, but don't run the task by default, as it eats up > precious CI credits. However, for cfbot we are using custom compute resources > (currently donated by google), so we can afford to run the mingw tests. Right > now that'd require cfbot to patch .cirrus.tasks.yml. And I think mingw ends up not running most of the time. +1 to running it as default at least on cfbot. Also, this gives people a chance to run mingw as default on their personal repositories (which I would like to run). > While one can manually trigger manual task in one one's own repo, most won't > have the permissions to do so for cfbot. > > > I propose that we instead run the task automatically if > $REPO_MINGW_TRIGGER_BY_DEFAULT is set, typically in cirrus' per-repository > configuration. > > Unfortunately that's somewhat awkward to do in the cirrus-ci yaml > configuration, the set of conditional expressions supported is very > simplistic. > > To deal with that, I extended .cirrus.star to compute the required environment > variable. If $REPO_MINGW_TRIGGER_BY_DEFAULT is set, CI_MINGW_TRIGGER_TYPE is > set to 'automatic', if not it's 'manual'. > Changes look good to me. My only complaint could be using only 'true' for the REPO_MINGW_TRIGGER_BY_DEFAULT, not a possible list of values but this is not important. > We've also talked in other threads about adding CI support for > 1) windows, building with visual studio > 2) linux, with musl libc > 3) free/netbsd > > That becomes more enticing, if we can enable them by default on cfbot but not > elsewhere. With this change, it'd be easy to add further variables to control > such future tasks. I agree. > I also attached a second commit, that makes the "code" dealing with ci-os-only > in .cirrus.tasks.yml simpler. While I think it does nicely simplify > .cirrus.tasks.yml, overall it adds lines, possibly making this not worth it. > I'm somewhat on the fence. > > > Thoughts? Although it adds more lines, this makes the .cirrus.tasks.yml file more readable and understandable which is more important in my opinion. > On the code level, I thought if it'd be good to have a common prefix for all > the automatically set variables. Right now that's CI_, but I'm not at all > wedded to that. I agree with your thoughts and CI_ prefix. I tested both patches and they work as expected. -- Regards, Nazir Bilal Yavuz Microsoft