Skip to content

Enable testing against MPFR on windows-gnu - #901

Open
tgross35 wants to merge 3 commits into
rust-lang:mainfrom
tgross35:windows-ci
Open

Enable testing against MPFR on windows-gnu#901
tgross35 wants to merge 3 commits into
rust-lang:mainfrom
tgross35:windows-ci

Conversation

@tgross35

@tgross35 tgross35 commented May 1, 2025

Copy link
Copy Markdown
Member

ci: skip-extensive
ci: test-libm

@tgross35
tgross35 force-pushed the windows-ci branch 2 times, most recently from b5fe4b9 to 53de1aa Compare May 1, 2025 23:31
@tgross35

tgross35 commented May 1, 2025

Copy link
Copy Markdown
Member Author

@mati865 any chance you'd be able to give me a hand here if you're familiar with MinGW on GHA? Our more thorough tests need to build gmp_mpfr_sys which should work on windows-gnu, but I can't seem to get a setup that has both needed tools (gcc, make, m4, diffutils) and Rust.

@mati865

mati865 commented May 2, 2025

Copy link
Copy Markdown
Member

I can take a look tomorrow.

@tgross35

tgross35 commented May 2, 2025

Copy link
Copy Markdown
Member Author

Feel free to scrap this if you know of a better way than going through msys2/setup-msys2 in GHA. The actual functional part of this diff is *windows-gnu*) ;; -> *windows-gnu*) mflags+=(--features libm-test/build-mpfr) ;; in ci/run.sh.

@mati865

mati865 commented May 3, 2025

Copy link
Copy Markdown
Member

@tgross35 I think what is missing is just setting up the PATH to contain rustup. Windows has some questionable design decisions for using PATH for binaries and shared objects (dynamic libraries), so MSYS2 by default doesn't include it entirely: https://github.com/msys2/setup-msys2#path-type

So the suggestion from Rustup:

To get started you may need to restart your current shell.
This would reload its PATH environment variable to include
Cargo's bin directory (%USERPROFILE%.cargo\bin).

Is not helpful at this time.
To solve it, you can either add Cargo bin dir explictly (export PATH=$USERPROFILE/.cargo/bin:$PATH) or setup MSYS2 to inherit whole PATH.

EDIT: okay, adding PATH is not that easy with GHA.

@mati865

mati865 commented May 3, 2025

Copy link
Copy Markdown
Member

So there are 2 solutions:

  • add echo 'export PATH="/c/Users/$USERNAME/.cargo/bin:$PATH"' >> ~/.bash_profile in ci/run_windows.sh
  • add path-type: inherit in .github/workflows/main.yaml

Then Rust installed with rustup is found. I don't have much insight on what happens later, but if you still need help, let me know.

@mati865

mati865 commented May 7, 2025

Copy link
Copy Markdown
Member

Hmm, it fails on:

Test compile: long long reliability test 1
configure:6585: gcc -O2 -pedantic -fomit-frame-pointer -m64  conftest.c >&5
conftest.c: In function 'f':
conftest.c:12:48: error: too many arguments to function 'g'; expected 0, have 6
   12 | for(i=0;i<1;i++){if(e(got,got,9,d[i].n)==0)h();g(i,d[i].src,d[i].n,got,d[i].want,9);if(d[i].n)h();}}
      |                                                ^ ~
conftest.c:7:6: note: declared here
    7 | void g(){}
      |      ^
configure:6588: $? = 1
failed program was:
/* The following provokes a segfault in the compiler on powerpc-apple-darwin.
   Extracted from tests/mpn/t-iord_u.c.  Causes Apple's gcc 3.3 build 1640 and
   1666 to segfault with e.g., -O2 -mpowerpc64.  */

#if defined (__GNUC__) && ! defined (__cplusplus)
typedef unsigned long long t1;typedef t1*t2;
void g(){}
void h(){}
static __inline__ t1 e(t2 rp,t2 up,int n,t1 v0)
{t1 c,x,r;int i;if(v0){c=1;for(i=1;i<n;i++){x=up[i];r=x+1;rp[i]=r;}}return c;}
void f(){static const struct{t1 n;t1 src[9];t1 want[9];}d[]={{1,{0},{1}},};t1 got[9];int i;
for(i=0;i<1;i++){if(e(got,got,9,d[i].n)==0)h();g(i,d[i].src,d[i].n,got,d[i].want,9);if(d[i].n)h();}}
#else
int dummy;
#endif

int main () { return 0; }
configure:7072: result: no, long long reliability test 1

Which looks like a legit error, this code is plainly wrong. Maybe GCC <15 somehow allowed it but they fixed the problem?

@tgross35

tgross35 commented May 7, 2025

Copy link
Copy Markdown
Member Author

Great catch and thanks for finding it, I didn't get a chance to look into it further. This is a compiler version difference https://gcc.godbolt.org/z/YTd6hnbM3.

I think the code is actually valid as the old school untyped function signatures, it would have to be void g(void) {} to actually be a zero-parameter function. I guess I wouldn't be too surprised if gmp is using k&r syntax somewhere.

@tgross35

tgross35 commented May 7, 2025

Copy link
Copy Markdown
Member Author

Looks like they're aware of it https://gmplib.org/list-archives/gmp-bugs/2024-November/005550.html

@tgross35

tgross35 commented May 7, 2025

Copy link
Copy Markdown
Member Author

It has been fixed upstream but that hasn't made it to the Rust version yet. Opened https://gitlab.com/tspiteri/gmp-mpfr-sys/-/issues/39 requesting an update.

@tgross35

Copy link
Copy Markdown
Member Author

No response from the rug maintainer yet. @mati865 is it possible to install an older version of GCC as the default, or at least something that we could export to the CC env? A bit hacky but I think that should get this over the line.

@mati865

mati865 commented May 29, 2025

Copy link
Copy Markdown
Member

MSYS2 doesn't support old packages but keeps them at: repo.msys2.org/mingw/mingw64/
So, if somebody is brave and dedicated enough, can install old toolchain.

IIRC, pacman can accept URLs with -U, so pacman -U https://repo.msys2.org/mingw/mingw64/mingw-w64-x86_64-gcc-14.2.0-3-any.pkg.tar.zst https://repo.msys2.org/mingw/mingw64/mingw-w64-x86_64-gcc-libs-14.2.0-3-any.pkg.tar.zst might work, fail to install because of unsatisfied dependencies or result in a broken toolchain...

I can try tomorrow.

EDIT: Hmm, we could also try asking on GH.
@tspiteri could you look into https://gitlab.com/tspiteri/gmp-mpfr-sys/-/issues/39?

@tgross35

Copy link
Copy Markdown
Member Author

Maybe there is a bit of an easier solution - the most recent GCC seems to accept the code with --std=C17. If I can get CFLAGS to the right place then I think that might work.

@rustbot

This comment has been minimized.

@tgross35
tgross35 force-pushed the windows-ci branch 3 times, most recently from 828e7e1 to c5376ec Compare September 2, 2026 04:02
@rustbot

rustbot commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Comment thread .github/workflows/main.yaml Outdated
python3 etc/update-api-list.py --check
cargo test -p update-api-list

- uses: msys2/setup-msys2@v2
tgross35 and others added 2 commits September 1, 2026 23:07
Poke libm ci

Squashed revisions

Disable most CI targets for testing

simplify CI more, exclude tests of compiler-builtins

Create a setup script

try with msys2 shell

install more

fix script

install rustup

try installing rustup again?

install rustup from the script

fix sh

disable interactive

don't try to run cargo

inspect path

try splitting so path reloads

Squahsed revisions

Add Cargo bin dir to PATH
Fix the path
Typo
Try with bashrc
DBG
Try inheriting the PATH
profile instead of bashrc
bash_profile?
revert path inheritance
Install correct C toolchain
Install pactoys
--noconfirm

print logs

print more

fix log

try setting cflags

skip tests since they fail without getrusage
@rustbot

rustbot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (possibly #1278) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants