Re: Mingw task for Cirrus CI - Mailing list pgsql-hackers

On Sat, Sep 03, 2022 at 12:52:54AM +0300, Melih Mutlu wrote:
> Justin Pryzby <pryzby@telsasoft.com>, 19 Ağu 2022 Cum, 05:34 tarihinde şunu yazdı:
> 
> > Inline notes about changes since the last version.
> >
> > On Thu, Jul 28, 2022 at 05:44:28PM -0500, Justin Pryzby wrote:
> > > I think the "only_if" should allow separately running one but not both
> > of the
> > > windows instances, like:
> > >
> > > +  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~
'.*\nci-os-only:[^\n]*mingw64'
> > >
> > > I'm not sure, but maybe this task should only run "by request", and omit the
> > > first condition:
> > >
> > > +  only_if: $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*mingw64'
> >
> > The patch shouldn't say this during development, or else cfbot doesn't run it..
> > Oops.
>
> Actually, making MinGW task optional for now might make sense. Due to
> windows resource limits on Cirrus CI and slow builds on Windows, adding
> this task as non-optional may not be an efficient decision
> I think that continuing with this patch by changing MinGW to optional for
> now, instead of waiting for more resource on Cirrus or faster builds on
> Windows, could be better. I don't see any harm.

I agree that maybe it should be optional if merged to postgres.

But cfbot should run the Mingw task for this patch's own commitfest
entry.  But right now (because cfbot doesn't include the original commit
message/s), it doesn't get run :(

> +  only_if: $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-include:[^\n]*mingw.* ||
>             $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*mingw.*'
> 
> Added this line to allow run only mingw task or run all tasks including
> mingw.
> 
> What do you all think about this change? Does it make sense?

You're allowing to write "ci-os-include: mingw" to "opt-in" to the task,
without opting-out of all the other tasks (and without enumerating all
the tasks by writing "ci-os-only: mingw,windows,macos,freebsd,linux".
That makes sense, and the logic looks right.  But that still has to be
commented during development to be run by cfbot.

Also, the first half was missing a closing quote.
https://cirrus-ci.com/build/5874178241855488

> > +  setup_additional_packages_script: |
> > > +    REM C:\msys64\usr\bin\pacman.exe -S --noconfirm busybox
> >
> > This should include choco, too.
> 
> Added pacman.exe line. Do we really need choco here? I don't think mingw
> would require any package via choco.

I guess choco isn't needed.

> Also is ending pacman.exe line with busybox intentional? I just added that
> line with "..." at the end instead of any package name.

Yeah, the busybox part was unintentional.

> > >  for item in `find "$sourcetree" -name Makefile -print -o -name GNUmakefile -print | grep -v
"$sourcetree/doc/src/sgml/images/"`;do
 
> > > -    filename=`expr "$item" : "$sourcetree\(.*\)"`
> > > -    if test ! -f "${item}.in"; then
> > > -        if cmp "$item" "$buildtree/$filename" >/dev/null 2>&1; then : ; else
> > > -            ln -fs "$item" "$buildtree/$filename" || exit 1
> > > -        fi
> > > -    fi
> > > +    filename=${item#$sourcetree}
> > > +    [ -e "$buildtree/$filename" ] && continue
> >
> > I fixed this to check for ".in" files as intended.
> >
> > It'd be a lot better if the image didn't take so long to start. :(
> 
> One question would be that should this patch include "prep_buildtree"? It
> doesn't seem to me like it's directly related to adding MinGW into CI but
> more like an improvement for builds on Windows.
> Maybe we can make it a seperate patch if it's necessary.

I don't know what direction that idea is going, but it makes working
with this patch a bit easier when configure is less slow.  Fine with me
to split it into a separate patch :)

> Sharing a new version of the patch. It also moves the above line so that it
> will apply to mingw task too. Otherwise mingw task was failing.

I saw that but hadn't tracked it down yet.  Do you know if the tar
failures were from a TAP test added since you first posted the mingw
patch, or ??

Also: your original patch said --host=x86_64-w64-mingw32, but the task
is called MinGW64.  The most recent patches don't use --host at all, and
were building for a 32 bit environment, even though the OS image says
MSYSTEM=UCRT64.

Also: right now src/test and src/interfaces/*/test aren't being built
during the build phase, which means that they're 1) not compiled in
parallel; and 2) not cached.  This isn't specific to MinGW.  Other than
compiling those dirs specifically, one option is to put
"always: upload_caches: ccache" after "test_world_script" (in that case,
if the CI instance is rescheduled during tests, the compilation won't be
pushed to cache).  Actually, it seems better to compile stuff during
"build" or else any compilation warnings should up in the middle of
"check-world.."

Also: I'm having second thoughts about the loop around ./configure.  It
could happen that a cached configure would succeed, but then the build
would later fail, and it wouldn't fix itself.  I think a slow configure
is okay for an "opt-in" task.

Also: my backtrace call was using a path to cygwin rather than msys.
This seemed to work before, but doesn't seem to be working now...

-- 
Justin