Re: Mingw task for Cirrus CI - Mailing list pgsql-hackers
| From | Melih Mutlu |
|---|---|
| Subject | Re: Mingw task for Cirrus CI |
| Date | |
| Msg-id | CAGPVpCRDCXAFRwFOfExo1-WXKDS_-w=e6tRQ1LUKQBoZ6OWwYA@mail.gmail.com Whole thread Raw |
| In response to | Re: Mingw task for Cirrus CI (Justin Pryzby <pryzby@telsasoft.com>) |
| Responses |
Re: Mingw task for Cirrus CI
|
| List | pgsql-hackers |
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.
+ 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?
Thanks for your contributions/reviews Justin!
> I think it should include something like
>
> + setup_additional_packages_script: |
> + REM C:\msys64\usr\bin\pacman.exe -S --noconfirm ...
>
> Let's see what others think about those.
>
> Do you know if this handles logging of crash dumps ?
It does now, although I hardcoded "postgres.exe" ...
merged this with my patch
> + 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.
Also is ending pacman.exe line with busybox intentional? I just added that line with "..." at the end instead of any package name.
> - CXXFLAGS='-Og -ggdb'"
> + CXXFLAGS='-Og -ggdb' && break;
> + rm -v ${CCACHE_DIR}/configure.cache;
> + done
I noticed that this doesn't seem to do the right thing with the exit status -
configure can fail without cirrusci noticing, and then the build fails at the
next step.
merged.
> 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.
What do you think?
TAR: "c:/windows/system32/tar.exe"
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.
Thanks,
Melih
Attachment
pgsql-hackers by date: