Opened 3 years ago

Closed 3 years ago

#20423 closed defect (fixed)

Updated patch for building MPFR on Cygwin

Reported by: embray Owned by:
Priority: major Milestone: sage-7.2
Component: porting: Cygwin Keywords: windows cygwin days77
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: 8856152 (Commits) Commit: 88561528acf90120d298a14d21062ee7ede2600b
Dependencies: Stopgaps:

Description

This is the update to #20380 with a new patch mirroring the upstream fix for the same issue. The patch I added was taking directly from SVN and applies to the current version of MPFR in sage with some fuzz (I don't know if this matters or not).

I confirmed that, as expected, like #20380 this fixes building MPFR on Cygwin.

Change History (19)

comment:1 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 3 years ago by jdemeyer

One detail: could you add some comment on the top of the patch file? In particular, a link to the SVN repo and this Sage ticket would be useful.

I cannot test on Cygwin myself, but if you tell me that it works for you, then I'll believe that. I checked that it doesn't break anything on Linux.

comment:3 follow-up: Changed 3 years ago by embray

Sure--perhaps something in the SPKG.txt as well?

comment:4 in reply to: ↑ 3 Changed 3 years ago by jdemeyer

Replying to embray:

Sure--perhaps something in the SPKG.txt as well?

That's not really needed. Better just document it in one place.

comment:5 Changed 3 years ago by git

  • Commit changed from 69079c3a1179a9d396e05ca9d4b53b040a9555b4 to 5f4777aa7ebb6e31d9439e21842182d074ec18ec

Branch pushed to git repo; I updated commit sha1. New commits:

5f4777aAdded a brief explanatory comment to the patch.

comment:6 Changed 3 years ago by jdemeyer

  • Component changed from build to porting: Cygwin
  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:7 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

merge conflict

comment:8 follow-up: Changed 3 years ago by embray

So my patch from #20380 was merged? That was very non-obvious.

comment:9 Changed 3 years ago by git

  • Commit changed from 5f4777aa7ebb6e31d9439e21842182d074ec18ec to 88561528acf90120d298a14d21062ee7ede2600b

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

48e2235Patch to mpfr required for it to get it building on Cygwin again
8856152Added a brief explanatory comment to the patch.

comment:10 Changed 3 years ago by embray

Rebased.

Last edited 3 years ago by embray (previous) (diff)

comment:11 Changed 3 years ago by jdemeyer

If it's ready for review, change the status to needs_review.

comment:12 in reply to: ↑ 8 Changed 3 years ago by jdemeyer

Replying to embray:

So my patch from #20380 was merged? That was very non-obvious.

When a ticket is closed as "fixed", it means that it's merged in Volker's private branch https://github.com/vbraun/sage/tree/develop This happens before buildbot testing.

This is really a private branch, which means that you should not use it for development. It is common for git history to be rewritten in that branch.

After buildbot testing, Volker will release a new beta release and Volker's branch will become the "real" develop branch.

comment:13 follow-up: Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

Thanks for the explanation--that explains why I didn't see it merged in develop.

I'm no so sure how I feel about that. It would be nice to have a branch in the main repo representing what is currently merged, rather than on someone else's fork. I'm also confused why it would be closed as "fixed" if buildbot testing hasn't run yet--isn't it possible the ticket will be reopened if the testing fails?

comment:14 in reply to: ↑ 13 Changed 3 years ago by jdemeyer

Replying to embray:

isn't it possible the ticket will be reopened if the testing fails?

Yes, that's possible and it happens occasionally.

comment:15 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:16 follow-up: Changed 3 years ago by embray

I'd suggest an additional workflow stage representing this. To me something isn't "fixed" if the "fix" breaks the build.

Last edited 3 years ago by embray (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 3 years ago by jdemeyer

Replying to embray:

I'd suggest an additional workflow stage representing this. To me something isn't "fixed" if the "fix" breaks the build.

I would suggest no additional workflow stage, just to close the ticket as fixed only when it's actually going to be merged. That was how I operated as release manager.

But it's not my decision, you have to convince Volker.

comment:18 Changed 3 years ago by embray

Either way.

comment:19 Changed 3 years ago by vbraun

  • Branch changed from u/embray/mpfr-cygwin-patch-2 to 88561528acf90120d298a14d21062ee7ede2600b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.