Thread: [MASSMAIL]ci: Allow running mingw tests by default via environment variable

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.

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'.


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 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?


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.


Greetings,

Andres Freund

Hi,

Thank you for working on this!

On Sat, 13 Apr 2024 at 05:12, Andres Freund <andres@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

On Fri, Apr 26, 2024 at 12:02 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> On Sat, 13 Apr 2024 at 05:12, Andres Freund <andres@anarazel.de> wrote:
> > 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.

Here's a generalised version of 0001.  If you put this in your
repository settings on Cirrus's website:

REPO_CI_AUTOMATIC_TRIGGER_TASKS="mingw netbsd"

... then it defines:

CI_TRIGGER_TYPE_MINGW=automatic
CI_TRIGGER_TYPE_NETBSD=automatic
CI_TRIGGER_TYPE_OPENBSD=manual

The set of tasks that get this treatment is defined by this line in
.cirrus.star:

    default_manual_trigger_tasks = ['mingw', 'netbsd', 'openbsd']

Then the individual tasks in .cirrus.tasks.yml use those variables:

     - name: NetBSD - Meson
+      # See REPO_CI_AUTOMATIC_TRIGGER_TASKS in .cirrus.star
+      trigger_type: $CI_TRIGGER_TYPE_NETBSD

Unfortunately the funky Starlark language doesn't seem to have regular
expressions, so it's just searching for those substrings without
contemplating delimiters.  That doesn't seem to be a problem at this
scale...

(I didn't look at the second patch today.)

Hi,

On Tue, 4 Mar 2025 at 03:28, Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Fri, Apr 26, 2024 at 12:02 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> > On Sat, 13 Apr 2024 at 05:12, Andres Freund <andres@anarazel.de> wrote:
> > > 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.
>
> Here's a generalised version of 0001.  If you put this in your
> repository settings on Cirrus's website:

Thank you for working on this!

> REPO_CI_AUTOMATIC_TRIGGER_TASKS="mingw netbsd"
>
> ... then it defines:
>
> CI_TRIGGER_TYPE_MINGW=automatic
> CI_TRIGGER_TYPE_NETBSD=automatic
> CI_TRIGGER_TYPE_OPENBSD=manual
>
> The set of tasks that get this treatment is defined by this line in
> .cirrus.star:
>
>     default_manual_trigger_tasks = ['mingw', 'netbsd', 'openbsd']
>
> Then the individual tasks in .cirrus.tasks.yml use those variables:
>
>      - name: NetBSD - Meson
> +      # See REPO_CI_AUTOMATIC_TRIGGER_TASKS in .cirrus.star
> +      trigger_type: $CI_TRIGGER_TYPE_NETBSD

I confirm that this works as expected and LGTM.

> Unfortunately the funky Starlark language doesn't seem to have regular
> expressions, so it's just searching for those substrings without
> contemplating delimiters.  That doesn't seem to be a problem at this
> scale...

You can load 're' and use it as in the v2-0002 but I think what you
did is good enough.

I rebased Andres' v2-0002 on top of recent changes and wrote a small
commit message. v4 is attached.

--
Regards,
Nazir Bilal Yavuz
Microsoft

Hi,

On 2025-03-04 13:27:29 +1300, Thomas Munro wrote:
> On Fri, Apr 26, 2024 at 12:02 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> > On Sat, 13 Apr 2024 at 05:12, Andres Freund <andres@anarazel.de> wrote:
> > > 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.
> 
> Here's a generalised version of 0001.  If you put this in your
> repository settings on Cirrus's website:
> 
> REPO_CI_AUTOMATIC_TRIGGER_TASKS="mingw netbsd"
> 
> ... then it defines:
> 
> CI_TRIGGER_TYPE_MINGW=automatic
> CI_TRIGGER_TYPE_NETBSD=automatic
> CI_TRIGGER_TYPE_OPENBSD=manual

Nice!


> The set of tasks that get this treatment is defined by this line in
> .cirrus.star:
> 
>     default_manual_trigger_tasks = ['mingw', 'netbsd', 'openbsd']
> 
> Then the individual tasks in .cirrus.tasks.yml use those variables:
> 
>      - name: NetBSD - Meson
> +      # See REPO_CI_AUTOMATIC_TRIGGER_TASKS in .cirrus.star
> +      trigger_type: $CI_TRIGGER_TYPE_NETBSD
> 
> Unfortunately the funky Starlark language doesn't seem to have regular
> expressions, so it's just searching for those substrings without
> contemplating delimiters.  That doesn't seem to be a problem at this
> scale...

FWIW, It does:

https://cirrus-ci.org/guide/programming-tasks/#re
https://github.com/qri-io/starlib/tree/master/re


Your edited version is looking good to me!

Greetings,

Andres Freund

Hi,

On 2025-03-04 11:53:30 +0300, Nazir Bilal Yavuz wrote:
> I rebased Andres' v2-0002 on top of recent changes and wrote a small
> commit message. v4 is attached.

Thanks for doing that, it does look a lot better after, I think!

Greetings,

Andres Freund

Hi,

I'm inclined to think we should apply to this to all branches with CI support,
not just master. It's kind of annoying to have CI infrastructure changes like
this that differ between branches.  Thoughts?

Greetings,

Andres Freund

Hi,

On Wed, 5 Mar 2025 at 18:51, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> I'm inclined to think we should apply to this to all branches with CI support,
> not just master. It's kind of annoying to have CI infrastructure changes like
> this that differ between branches.  Thoughts?

I am sharing patches per branch until we come up with a better
solution. Some remarks:

REL_15: It doesn't have any manual tasks, so per repository trigger
patch (Thomas' patch) is omitted.
REL_16: Only Mingw task is manual.
REL_17: Only Mingw task is manual.

While rebasing them, I realized that we use
'REPO_CI_AUTOMATIC_TRIGGER_TASKS' for all branches. So, we don't have
per branch 'REPO_CI_AUTOMATIC_TRIGGER_TASKS' configuration option. Is
that a problem?

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

Hi,

On 2025-04-07 16:03:48 +0300, Nazir Bilal Yavuz wrote:
> On Wed, 5 Mar 2025 at 18:51, Andres Freund <andres@anarazel.de> wrote:

> > I'm inclined to think we should apply to this to all branches with CI support,
> > not just master. It's kind of annoying to have CI infrastructure changes like
> > this that differ between branches.  Thoughts?
> 
> I am sharing patches per branch until we come up with a better
> solution. Some remarks:
> 
> REL_15: It doesn't have any manual tasks, so per repository trigger
> patch (Thomas' patch) is omitted.
> REL_16: Only Mingw task is manual.
> REL_17: Only Mingw task is manual.
> 
> While rebasing them, I realized that we use
> 'REPO_CI_AUTOMATIC_TRIGGER_TASKS' for all branches. So, we don't have
> per branch 'REPO_CI_AUTOMATIC_TRIGGER_TASKS' configuration option. Is
> that a problem?

I think that's not a problem. I can't think of a case where one would want to
do this only for some of the branches.  Does anybody disagree?

Greetings,

Andres Freund

On Tue, Apr 8, 2025 at 1:43 AM Andres Freund <andres@anarazel.de> wrote:
> On 2025-04-07 16:03:48 +0300, Nazir Bilal Yavuz wrote:
> > On Wed, 5 Mar 2025 at 18:51, Andres Freund <andres@anarazel.de> wrote:
> > > I'm inclined to think we should apply to this to all branches with CI support,
> > > not just master. It's kind of annoying to have CI infrastructure changes like
> > > this that differ between branches.  Thoughts?
> >
> > I am sharing patches per branch until we come up with a better
> > solution. Some remarks:
> >
> > REL_15: It doesn't have any manual tasks, so per repository trigger
> > patch (Thomas' patch) is omitted.
> > REL_16: Only Mingw task is manual.
> > REL_17: Only Mingw task is manual.
> >
> > While rebasing them, I realized that we use
> > 'REPO_CI_AUTOMATIC_TRIGGER_TASKS' for all branches. So, we don't have
> > per branch 'REPO_CI_AUTOMATIC_TRIGGER_TASKS' configuration option. Is
> > that a problem?
>
> I think that's not a problem. I can't think of a case where one would want to
> do this only for some of the branches.  Does anybody disagree?

Nope.

I think we should push all this stuff and turn it on for cfbot, not
only for the benefit of new development but also for the v18
stabilisation period.

Is it safe to assume that CI changes come under the same code-freeze
exception as tests and docs?  I would assume so, it's just scripting
glue in .cirrus.XXX files not affecting one bit of the software, but I
don't recall that we specifically clarified that before, so can
someone from the RMT please +1 this?  Then I'll go ahead and push
these.

On Sat, Apr 12, 2025 at 08:18:35PM +1200, Thomas Munro wrote:
> Is it safe to assume that CI changes come under the same code-freeze
> exception as tests and docs?  I would assume so, it's just scripting
> glue in .cirrus.XXX files not affecting one bit of the software, but I
> don't recall that we specifically clarified that before, so can
> someone from the RMT please +1 this?  Then I'll go ahead and push
> these.

That seems reasonable to me.  But to play devil's advocate, is there a
strong reason for doing this before July?

-- 
nathan

Hi,

On April 14, 2025 9:22:40 PM GMT+02:00, Nathan Bossart <nathandbossart@gmail.com> wrote:
>On Sat, Apr 12, 2025 at 08:18:35PM +1200, Thomas Munro wrote:
>> Is it safe to assume that CI changes come under the same code-freeze
>> exception as tests and docs?  I would assume so, it's just scripting
>> glue in .cirrus.XXX files not affecting one bit of the software, but I
>> don't recall that we specifically clarified that before, so can
>> someone from the RMT please +1 this?  Then I'll go ahead and push
>> these.
>
>That seems reasonable to me.  But to play devil's advocate, is there a
>strong reason for doing this before July?

We get more coverage of the changes we merge until then.

Greetings,

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Mon, Apr 14, 2025 at 09:26:59PM +0200, Andres Freund wrote:
> On April 14, 2025 9:22:40 PM GMT+02:00, Nathan Bossart <nathandbossart@gmail.com> wrote:
>>On Sat, Apr 12, 2025 at 08:18:35PM +1200, Thomas Munro wrote:
>>> Is it safe to assume that CI changes come under the same code-freeze
>>> exception as tests and docs?  I would assume so, it's just scripting
>>> glue in .cirrus.XXX files not affecting one bit of the software, but I
>>> don't recall that we specifically clarified that before, so can
>>> someone from the RMT please +1 this?  Then I'll go ahead and push
>>> these.
>>
>>That seems reasonable to me.  But to play devil's advocate, is there a
>>strong reason for doing this before July?
> 
> We get more coverage of the changes we merge until then. 

Sure.  To be clear, I think this change is fine to proceed, especially
given we're less than a week into feature freeze and have plenty of time to
stabilize any cfbot breakage.  If anything, this change should _help_
stabilize v18.  I do wonder if we should be more cautious about these kinds
of changes closer to release dates, but that's not a concern here.

-- 
nathan