Skip to content

Enable testing against MPFR on windows-gnu #901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented May 1, 2025

ci: skip-extensive

@tgross35
Copy link
Contributor Author

tgross35 commented May 1, 2025

@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
Copy link

mati865 commented May 2, 2025

I can take a look tomorrow.

@tgross35
Copy link
Contributor Author

tgross35 commented May 2, 2025

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
Copy link

mati865 commented May 3, 2025

@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
Copy link

mati865 commented May 3, 2025

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
Copy link

mati865 commented May 7, 2025

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
Copy link
Contributor Author

tgross35 commented May 7, 2025

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
Copy link
Contributor Author

tgross35 commented May 7, 2025

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

@tgross35
Copy link
Contributor Author

tgross35 commented May 7, 2025

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.

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.

2 participants