Skip to content

Conversation

@echoix
Copy link
Member

@echoix echoix commented Apr 20, 2025

Closes #4554

When it will be time to merge this, enable this workflow to be a required check, then merge (as the Windows 2019 run won't have a result). Then other PRs would need to be updated.

There is a problem with r.mapcalc giving different weird return codes., making tests fail after reruns. Something isn't right, and I can't pin it down. I think it's the same reason I didn't create a PR here last year for that.

Help is needed.

@echoix echoix added the help wanted Extra attention is needed label Apr 20, 2025
@github-actions github-actions bot added windows Microsoft Windows specific CI Continuous integration labels Apr 20, 2025
@echoix
Copy link
Member Author

echoix commented May 8, 2025

Merged main to see if #5601 of r.plane changes something to the r.plane failure. And also since some improvements to the stdbool handling.

@echoix
Copy link
Member Author

echoix commented May 8, 2025

Note that the brownouts are starting in June 2025, (in three weeks), and the windows-2019 will be gone at in July, after 4 brownouts during June.

@echoix
Copy link
Member Author

echoix commented May 17, 2025

I think I found a clue on what's going on. The tests that fail here (like that are using r.mapcalc), are failing with return code 3221225781, and a little google search brings this expanded at the top result:

On Windows with Python subprocess, an executable may be missing DLL from PATH. This environment error can be detected and a message given to the user. The return code of the subprocess can be checked for the error code 3221225781, which corresponds to hex error code C0000135.Jan 24, 2024

So something is not finding the DLLs for r.mapcalc in the pytest step.

@echoix
Copy link
Member Author

echoix commented Jun 4, 2025

I think I found it, it worked on a "dirty", mixed, PR on my fork. I'm trying out here just the minimum that worked once reverting piece by piece what wasn't needed.

@echoix
Copy link
Member Author

echoix commented Jun 4, 2025

If this run fails, then it might mean #5717 was needed, it's one of the changes that I tried with but didn't try again without it.

@echoix
Copy link
Member Author

echoix commented Jun 4, 2025

Once this would be ready to merge, we will need to add windows 2022 to the required checks, and after that, disable windows-2019 from the required checks. At this point, PRs won't have the required status check for the "windows-2019", so can't be merged. Then merge this PR. Then, PRs can be updated from main to be able to merge.

Question:
Should I also make the "runs-on" static and fixed to a single runner? It would change the name of job, once, but afterwards, the changes in the settings wouldn't be needed again for this kind of change.

@echoix
Copy link
Member Author

echoix commented Jun 4, 2025

After rereading for an uncountable number of times the contents of the runner images of windows-2019 and windows-2022, today the note (present in both pages) that said that msys2 is preinstalled but not in PATH gave me a new idea to explore. I looked in the custom script "build_osgeo4w.sh", the one that diverges from the one used by osgeo4w, and tried changing three lines that used the c:\msys64 path (the location of the pre-installed one), and changed it to the custom location installed by us in the first steps. Then it worked. So, I removed the changes one by one, and only the one remaining here was needed. I then adapted it to use a env var to supply that folder, with the default value being the currently used value.

@echoix echoix requested a review from wenzeslaus June 4, 2025 12:21
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me and most of the tests still run.

I can change the required check from 2019 to 2022 for starters.

@echoix
Copy link
Member Author

echoix commented Jun 4, 2025

Ok, so you would merge as is? When are you ready for the switch? Would you want to change to not use a matrix strategy for the OS now, or at a later time ?

And the same number of tests (files) run and pass, so no change here

@wenzeslaus
Copy link
Member

The Windows 2022 check is now mandatory and 2019 is not. @echoix I'll let you do the merge.

Everyone, all PRs will need an update from main to pass the checks again after this PR is merged.

Let's do the matrix change separately. As for timing, with the brownouts, it seems we need to switch now. Sorry, for the delay here.

@echoix echoix merged commit 4aa64fc into OSGeo:main Jun 6, 2025
25 checks passed
@echoix
Copy link
Member Author

echoix commented Jun 6, 2025

Update the PRs that need to be merged today. I'll more slowly update ones that are close to be ready later in the next evenings, when there is less CI activity, to not slow down work during the day.

@github-actions github-actions bot added this to the 8.5.0 milestone Jun 6, 2025
@echoix echoix added the backport to 8.4 PR needs to be backported to release branch 8.4 label Jun 16, 2025
@echoix
Copy link
Member Author

echoix commented Jun 16, 2025

@neteler I think this need to be backported if we continue to commit on 8.4

@neteler
Copy link
Member

neteler commented Jun 16, 2025

@neteler I think this need to be backported if we continue to commit on 8.4

Sure. I just tried but get

both modified: .github/workflows/osgeo4w.yml

Shall I sync that file entirely to G84?

@echoix
Copy link
Member Author

echoix commented Jun 16, 2025

Hmmm, were the patches that handles using a different package list+copying dlls applied in 8.4? If not, it should be only the relevant parts of this PR to apply, not the full workflow

echoix added a commit to echoix/grass that referenced this pull request Jun 17, 2025
The windows-2019 image is being removed in less than a month. We need
to use windows-2022. The changes needed in our build_osgeo4w.sh script
relate to the msys2 paths added to path. They need to use the location
defined in the CI step, otherwise, another version already installed
in the runner, but normally not on path, is used. On windows-2019, we
didn’t see errors of this misconfiguration, but we did for
windows-2022.

* CI(OSGeo4W): Upgrade image to windows-2022

* mswindows: Update path for dumpbin in mklibs.sh

* windows: Use MSYS2_LOCATION env var to create script from templates
in build_osgeo4w.sh

* CI(OSGeo4W): Add MSYS2_LOCATION env var to build stage
echoix added a commit to echoix/grass that referenced this pull request Jun 17, 2025
The windows-2019 image is being removed in less than a month. We need
to use windows-2022. The changes needed in our build_osgeo4w.sh script
relate to the msys2 paths added to path. They need to use the location
defined in the CI step, otherwise, another version already installed
in the runner, but normally not on path, is used. On windows-2019, we
didn’t see errors of this misconfiguration, but we did for
windows-2022.

* CI(OSGeo4W): Upgrade image to windows-2022

* mswindows: Update path for dumpbin in mklibs.sh

* windows: Use MSYS2_LOCATION env var to create script from templates
in build_osgeo4w.sh

* CI(OSGeo4W): Add MSYS2_LOCATION env var to build stage
@neteler neteler removed the backport to 8.4 PR needs to be backported to release branch 8.4 label Jun 17, 2025
@echoix echoix deleted the windows-2022-ci branch October 18, 2025 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI Continuous integration help wanted Extra attention is needed windows Microsoft Windows specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feat] Support newer Windows server 2022 and 2025

3 participants