Skip to content

MAINT: Pin rtools version on Windows.#23678

Merged
charris merged 1 commit intonumpy:mainfrom
charris:fix-windows-32-bit-ci
May 3, 2023
Merged

MAINT: Pin rtools version on Windows.#23678
charris merged 1 commit intonumpy:mainfrom
charris:fix-windows-32-bit-ci

Conversation

@charris
Copy link
Copy Markdown
Member

@charris charris commented Apr 28, 2023

See scipy/scipy#18374 for the reason.

Closes #23675.

@charris charris added this to the 1.25.0 release milestone Apr 28, 2023
@charris
Copy link
Copy Markdown
Member Author

charris commented Apr 28, 2023

Not sure this will work, we will see.

@andyfaff
Copy link
Copy Markdown
Member

andyfaff commented Apr 28, 2023

That looks right to me. How does the RTOOLS40_HOME environment variable get created?

@charris
Copy link
Copy Markdown
Member Author

charris commented Apr 28, 2023

How does the RTOOLS40_HOME environment variable get created?

Looks like it is set by rtools during installation. Maybe that is why not all the windows tests were failing?

So maybe using RTOOLS43_HOME would also fix things?

@charris
Copy link
Copy Markdown
Member Author

charris commented Apr 28, 2023

Darn, still failing.

@charris
Copy link
Copy Markdown
Member Author

charris commented Apr 28, 2023

Note that full tests on 64 bit windows passed.

run: |
echo "PLAT=i686" >> $env:GITHUB_ENV
echo "PATH=$env:RTOOLS40_HOME\mingw32\bin;$env:PATH" >> $env:GITHUB_ENV
echo "PATH=$env:RTOOLS43_HOME\mingw32\bin;$env:PATH" >> $env:GITHUB_ENV
Copy link
Copy Markdown
Member

@andyfaff andyfaff Apr 28, 2023

Choose a reason for hiding this comment

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

I don't think you want this. RTOOLS43_HOME would be for the one that's just been released (it's not clear to me what's actually creating that environment variable).

On scipy we use:

          choco install rtools -y --no-progress --force --version=4.0.0.20220206
          echo "c:\rtools40\ucrt64\bin;" >> $env:GITHUB_PATH

that puts the 64 bit gcc/gfortran/pkg-config on PATH. If you want the 32-bit tools they're in C:\rtools40\mingw32\bin. You can either put those directories on PATH or set the RTOOLS40_HOME variable correctly.

To fix the issue I think you need to set the RTOOLS40_HOME variable, i.e.

$env:RTOOLS40_HOME = "c:\rtools40"

or

echo "RTOOLS40_HOME=c:\rtools40" >> "$GITHUB_ENV"

@@ -109,7 +109,7 @@ jobs:
- name: setup rtools for 32-bit
run: |
echo "PLAT=i686" >> $env:GITHUB_ENV
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you need a choco install rtools -y --no-progress --force --version=4.0.0.20220206 line here. Otherwise you'll get rtools43 by default.


- powershell: |
choco install -y rtools --force --version=4.0.0.20220206
choco install -y rtools
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The RTOOLS43_HOME variable may not have been set?

$env:CFLAGS = "-m32"
$env:LDFLAGS = "-m32"
$env:PATH = "$env:RTOOLS40_HOME\\mingw32\\bin;$env:PATH"
$env:PATH = "$env:RTOOLS43_HOME\\mingw32\\bin;$env:PATH"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you want to use rtools43 it'd be worth checking that the tools are in the same place.

@charris charris force-pushed the fix-windows-32-bit-ci branch 2 times, most recently from b9de6fb to b9e1489 Compare April 28, 2023 21:21
@andyfaff
Copy link
Copy Markdown
Member

Those tests look as if they're using gfortran to compile code. If you look at https://dev.azure.com/numpy/numpy/_build/results?buildId=29282&view=logs&j=d02055d6-a30d-5402-19aa-ca2310eeee52&t=3b8a6fc4-0b0a-5f32-7dd9-04a675f63363&l=1691 I think the 64 bit compiler is being picked up, rather than the 32 bit. The 32 bit is in $env:RTOOLS40_HOME\mingw32\bin.

@charris
Copy link
Copy Markdown
Member Author

charris commented Apr 29, 2023

The path and flags look correct:

- powershell: |
    If ($(BITS) -eq 32) {
        $env:CFLAGS = "-m32"
        $env:LDFLAGS = "-m32"
        $env:PATH = "$env:RTOOLS40_HOME\\mingw32\\bin;$env:PATH"
    }
    Else
    {
        $env:PATH = "$env:RTOOLS40_HOME\\mingw64\\bin;$env:PATH"
    }
    python runtests.py -n --show-build-log --mode=$(TEST_MODE) -- -rsx --junitxml=junit/test-results.xml

What I cannot determine is what changed to make the tests suddenly fail. Do we need some flags for gfortran?

@charris charris force-pushed the fix-windows-32-bit-ci branch from e2dfebc to 110878c Compare May 3, 2023 16:18
@seberg
Copy link
Copy Markdown
Member

seberg commented May 3, 2023

Seems like adding the path worked, just harder to do then one would think. Is this the right thing then (with maybe tiny cleanup?).
To me it seems like we might as well give it a try, the constant failure isn't helpful after all...

EDIT: Is the echo "c:\rtools40\ucrt64\bin;" >> $env:GITHUB_PATH a way to add things to the path that is more reliable maybe?!

@charris charris force-pushed the fix-windows-32-bit-ci branch from bba74e6 to ea7e831 Compare May 3, 2023 20:09
@charris
Copy link
Copy Markdown
Member Author

charris commented May 3, 2023

Is the echo "c:\rtools40\ucrt64\bin;" >> $env:GITHUB_PATH a way to add things to the path

I'm giving it a shot, let's see how it does. I would prefer that as it is hopefully global.

It turns out that rtools 42+ do not support 32 bit builds, so we are stuck with pinning rtools until we drop the 32 bit Windows builds.

@charris
Copy link
Copy Markdown
Member Author

charris commented May 3, 2023

I'm also curious to see if the 32 bit wheel builds work tonight.

@andyfaff
Copy link
Copy Markdown
Member

andyfaff commented May 3, 2023

Is the echo "c:\rtools40\ucrt64\bin;" >> $env:GITHUB_PATH a way to add things to the path

Yes, I think that's the way to add things permanently to path. If you add an environment variable in one step they're not carried through to subsequent steps.

See https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-an-environment-variable. GITHUB_ENV does environment variables, GITHUB_PATH does path I think.

See https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#adding-a-system-path as well.

Copy link
Copy Markdown
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Looks promising and like its probably gonna work. (I guess the test isn't far enough to guess yet after all, but in any case... so long the tests passes I think this is good, in whatever variation it ends up being)

@andyfaff
Copy link
Copy Markdown
Member

andyfaff commented May 3, 2023

(I did try making a PR against @charris fork to try to illustrate this)

@charris charris force-pushed the fix-windows-32-bit-ci branch from ea7e831 to 0e43b3f Compare May 3, 2023 21:57
@charris
Copy link
Copy Markdown
Member Author

charris commented May 3, 2023

Used the (different) method we used in numpy-wheels for setting global variables and it worked. There was an unrelated test failure that turns up now and then and that can be ignored. I'm going to put this in.

@charris charris merged commit a2d21d8 into numpy:main May 3, 2023
@charris
Copy link
Copy Markdown
Member Author

charris commented May 3, 2023

Thanks for the help @andyfaff @seberg.

@charris charris removed the 25 - WIP label May 4, 2023
@charris charris modified the milestones: 1.25.0 release, 1.24.4 release May 4, 2023
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label May 5, 2023
@charris charris removed this from the 1.24.4 release milestone May 5, 2023
@charris charris deleted the fix-windows-32-bit-ci branch May 11, 2023 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: f2py is failing on 32-bit windows on main

3 participants