Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#20380 closed defect (fixed)

Patch to MPFR to get it building on Cygwin again

Reported by: Erik Bray Owned by:
Priority: major Milestone: sage-7.2
Component: build Keywords: windows cygwin days77
Cc: Travis Scrimshaw, Sebastien Gouezel, Jean-Pierre Flori, Paul Zimmermann Merged in:
Authors: Erik Bray Reviewers: Sebastien Gouezel
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: 76cb015 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jean-Pierre Flori)

The attached adds a patch to mpfr required for it to build successfully on Cygwin (no other guarantees beyond that it builds). I have tested that this resolves the problem. The original discussion where this came up is here: https://groups.google.com/d/msg/sage-devel/c6mj0MxEovI/zl_7H7-fDgAJ

The patch came from: https://github.com/Alexpux/MSYS2-packages/blob/master/mpfr/mpfr-3.1.4-1.src.patch

Upstream applied a slightly different patch. Please report if it works.

Change History (19)

comment:1 Changed 6 years ago by Erik Bray

Status: newneeds_review

comment:2 Changed 6 years ago by Samuel Lelièvre

Keywords: days77 added

comment:3 Changed 6 years ago by Travis Scrimshaw

Cc: Travis Scrimshaw Sebastien Gouezel Jean-Pierre Flori added

comment:4 Changed 6 years ago by Vincent Delecroix

Please add your name in the Author field.

comment:5 Changed 6 years ago by Erik Bray

Authors: embray

comment:6 Changed 6 years ago by Vincent Delecroix

Authors: embrayErik Bray

(should be your full name, I just changed it)

comment:7 Changed 6 years ago by Sebastien Gouezel

Reviewers: Sebastien Gouezel
Status: needs_reviewpositive_review

Looks good to me. It would be nice if this were integrated in upstream directly.

comment:8 Changed 6 years ago by Jean-Pierre Flori

Cc: Paul Zimmermann added

@Paul: could you integrate this into MPFR? We also have Andreas with us so we can talk to him.

comment:9 Changed 6 years ago by Paul Zimmermann

I've applied the (slightly different) following patch upstream:

--- src/mpfr-impl.h     (revision 10254)
+++ src/mpfr-impl.h     (working copy)
@@ -209,18 +209,18 @@
 #endif
 
 /* Detect some possible inconsistencies under Unix. */
-#if defined(__unix__)
-# if defined(_WIN32)
-#  error "Both __unix__ and _WIN32 are defined"
-# endif
-# if __GMP_LIBGMP_DLL
-#  error "__unix__ is defined and __GMP_LIBGMP_DLL is true"
-# endif
-# if defined(MPFR_WIN_THREAD_SAFE_DLL)
-#  error "Both __unix__ and MPFR_WIN_THREAD_SAFE_DLL are defined"
-# endif
+#if defined(__unix__) && defined(_WIN32)
+# error "Both __unix__ and _WIN32 are defined"
 #endif

+#if defined(__unix__) && __GMP_LIBGMP_DLL && !defined(__CYGWIN__)
+# error "__unix__ is defined and __GMP_LIBGMP_DLL is true"
+#endif
+
+#if defined(__unix__) && defined(MPFR_WIN_THREAD_SAFE_DLL)
+# error "Both __unix__ and MPFR_WIN_THREAD_SAFE_DLL are defined"
+#endif
+
 #if defined(__MPFR_WITHIN_MPFR) || !defined(MPFR_WIN_THREAD_SAFE_DLL)
 extern MPFR_THREAD_ATTR mpfr_flags_t __gmpfr_flags;
 extern MPFR_THREAD_ATTR mpfr_exp_t   __gmpfr_emin;

Please tell me if it works. Paul

comment:10 Changed 6 years ago by Paul Zimmermann

I'll revert the above patch upstream since we (the MPFR developers) are not sure this is the best way to solve the problem. We are looking to this problem.

comment:11 Changed 6 years ago by Jean-Pierre Flori

Description: modified (diff)
Report Upstream: None of the above - read trac for reasoning.Fixed upstream, but not in a stable release.

comment:12 Changed 6 years ago by Volker Braun

Branch: u/embray/mpfr-cygwin-patch76cb01574b70f6df2cb2b427bb57272f7c4605c8
Resolution: fixed
Status: positive_reviewclosed

comment:13 Changed 6 years ago by Paul Zimmermann

Commit: 76cb01574b70f6df2cb2b427bb57272f7c4605c8

note that upstream we decided to remove completely the paragraph Detect some possible inconsistencies under Unix. Paul

comment:14 Changed 6 years ago by Erik Bray

I'm a little confused as to the outcome of this. zimmerma proposed a slight different patch above, but then suggested that it's no longer needed?

Why was this ticket closed? Even if the issue has been fixed (somehow--it's not clear) upstream that doesn't mean it's fixed in Sage. A patch is still needed for now in Sage in order for it to build on Cygwin.

comment:15 Changed 6 years ago by Jean-Pierre Flori

What happened here is as follow:

  • this ticket proposed to integrate the Cygwin-folk patch, was positively reviewed and integrated to Sage git version (that's more or less what closed mean).
  • in the meantime Paul was CC'ed to be aware of the need of a patch to build on Cygwin and ended up with a different solution.

So I think the next steps would be:

  • check that upstream patch works well,
  • open a new ticket to replace the Cygwin patch we currently ship by the upstream patch.

It may have been cleaner to wait for upstream decision to positively review this ticket, but the situation we are in now is still better than before: MPFR builds on Cygwin (and should still after replacing our/Cygwin patch with the upstream one).

comment:16 Changed 6 years ago by Erik Bray

jpflori,

Thanks for the overview. What you wrote more or less matches my understanding, but where I was confused was that it wasn't obvious that my patch was merged into the main branch. I guess that was was going on here where it reads "Branch changed from u/embray/mpfr-cygwin-patch to 76cb01574b70f6df2cb2b427bb57272f7c4605c8"?

comment:17 Changed 6 years ago by Erik Bray

FWIW the upstream patch in question is here: https://gforge.inria.fr/scm/viewvc.php/mpfr/trunk/src/mpfr-impl.h?r1=10257&r2=10260

It looks like it should work since it ended up removing the problem code entirely. But I will still test and open a new ticket.

comment:18 Changed 6 years ago by Erik Bray

Okay, so it looks like my original patch wasn't merged after all...

comment:19 Changed 6 years ago by Erik Bray

Followup in #20423

Note: See TracTickets for help on using tickets.