#26383 closed defect (fixed)

Include missing patch for openblas from #24669

Reported by: embray Owned by:
Priority: blocker Milestone: sage-8.4
Component: porting: Cygwin Keywords:
Cc: vbraun Merged in:
Authors: Erik Bray Reviewers: Timo Kaufmann
Report Upstream: N/A Work issues:
Branch: 13e768b (Commits) Commit: 13e768bd6b3819f9b622ef5388bd89d6a26373ef
Dependencies: Stopgaps:

Description

My branch for #24669 originally included this patch (the patch is even mentioned in the ticket description). But when I rebased the branch on 8.4.beta0 it looks like I somehow dropped the commit that included the patch.

I have good reason to believe this is what caused the Cygwin buildbot to hang, but I am in the process of rebuilding with the patch now in order to test.

I just discovered this issue or obviously I would have reported sooner.

Change History (10)

comment:1 Changed 15 months ago by embray

Not sure if I should bump the patch level or not. The main body of the patch itself only impacts Cygwin (everything it changes is protected by an #ifdef), but it also updates one test that will run on other platforms.

I just manually rebuilt openblas with SAGE_CHECK=yes and this patch on my Ubuntu machine and it passed, but maybe it should run on the other buildbots as well...?

The additional test is a less important part of the patch, and can be removed if it is a problem.

comment:2 Changed 15 months ago by git

  • Commit changed from d1a93c54edbcd30584aac8ee0effdfd5d5bd6f93 to 4ef32f5d8098f96d97ef7b126232646113394a95

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

4ef32f5Add patch for supporting threads + fork() on Cygwin

comment:3 Changed 15 months ago by embray

  • Status changed from new to needs_review

Went ahead and bumped the package version.

I confirmed that this fixed cases of code which uses openblas hanging on Cygwin (which is a relief, since after the last time I fixed this I had a hard time imagining what else could be broken unless there was some deep bug in Cygwin).

comment:4 follow-up: Changed 15 months ago by gh-timokau

Can you please add documentation to the patch? What is the upstreaming status?

comment:5 in reply to: ↑ 4 Changed 15 months ago by embray

Replying to gh-timokau:

Can you please add documentation to the patch? What is the upstreaming status?

As I wrote on the original ticket, this fix is already accepted upstream and is in a later release. Next time we update the openblas package for Sage we can probably drop it.

comment:6 Changed 15 months ago by git

  • Commit changed from 4ef32f5d8098f96d97ef7b126232646113394a95 to 13e768bd6b3819f9b622ef5388bd89d6a26373ef

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

13e768bAdd patch for supporting threads + fork() on Cygwin

comment:7 Changed 15 months ago by embray

Fine. I added a comment to the patch file. Note, you can also always look at the git log for the file to see where it came from. But I agree it's often more convenient to have it right in the file.

comment:8 Changed 15 months ago by gh-timokau

Thank you! Yes its usually possible to find the origin of a patch with git-log, but its far from pleasant. We should document at least all new patches. Its a bit less urgent when its already accepted upstream, but its still nice to be able to see that immediately.

LGTM now, positive review pending on local tests.

comment:9 Changed 15 months ago by gh-timokau

  • Reviewers set to Timo Kaufmann
  • Status changed from needs_review to positive_review

comment:10 Changed 14 months ago by vbraun

  • Branch changed from u/embray/ticket-24669-2 to 13e768bd6b3819f9b622ef5388bd89d6a26373ef
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.