Opened 2 years ago
Closed 2 years ago
#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 2 years ago by
comment:2 Changed 2 years ago by
- Commit changed from d1a93c54edbcd30584aac8ee0effdfd5d5bd6f93 to 4ef32f5d8098f96d97ef7b126232646113394a95
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
| 4ef32f5 | Add patch for supporting threads + fork() on Cygwin
|
comment:3 Changed 2 years ago by
- 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: ↓ 5 Changed 2 years ago by
Can you please add documentation to the patch? What is the upstreaming status?
comment:5 in reply to: ↑ 4 Changed 2 years ago by
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 2 years ago by
- Commit changed from 4ef32f5d8098f96d97ef7b126232646113394a95 to 13e768bd6b3819f9b622ef5388bd89d6a26373ef
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
| 13e768b | Add patch for supporting threads + fork() on Cygwin
|
comment:7 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
- Reviewers set to Timo Kaufmann
- Status changed from needs_review to positive_review
comment:10 Changed 2 years ago by
- Branch changed from u/embray/ticket-24669-2 to 13e768bd6b3819f9b622ef5388bd89d6a26373ef
- Resolution set to fixed
- Status changed from positive_review to closed
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=yesand 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.