Opened 6 years ago
Closed 6 years ago
#20487 closed defect (fixed)
Numpy build broken on Cygwin since #20450
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.3 |
Component: | porting: Cygwin | Keywords: | cygwin windows numpy |
Cc: | Merged in: | ||
Authors: | Erik Bray | Reviewers: | Sebastien Gouezel, Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | 702cb85 (Commits, GitHub, GitLab) | Commit: | 702cb85f74093dd905a9c8337ff096acd8e2e22d |
Dependencies: | Stopgaps: |
Description
Since the upgrade to Numpy 1.11, the patch cygwin-lapack_lite-setup.py.patch no longer applies. This is hardly the first time (#17014, etc. :)
It's not entirely clear whether this patch is even needed anymore. The first version of it was added 8 years ago: http://git.sagemath.org/sage.git/commit/?h=develop&id=f0ef7690e4f601e289608a09fe30df9d242eb3e8
It's not exactly clear what issue this is fixing, and I can't find any obvious reference to what it might have been--not in any of the more recent Cygwin-related documentation or anywhere. Numpy compiles fine without it.
Change History (21)
comment:1 Changed 6 years ago by
comment:2 Changed 6 years ago by
- Branch set to u/embray/ticket-20487
- Commit set to 1dc86119db2155471346b82d99877af72bd898cf
- Keywords cygwin windows numpy added
- Status changed from new to needs_review
From my testing I've concluded that the patch in question can be dropped. It seems that with some ancient version of Numpy the -shared
flag was being excluded when linking this module on Cygwin for some reason or other. That is no longer the case though.
New commits:
1dc8611 | Remove Cygwin-only patch to numpy.linalg.setup
|
comment:3 Changed 6 years ago by
- Status changed from needs_review to positive_review
Works for me as well.
comment:4 Changed 6 years ago by
- Reviewers set to Sebastien Gouezel
comment:5 Changed 6 years ago by
- Branch changed from u/embray/ticket-20487 to 1dc86119db2155471346b82d99877af72bd898cf
- Resolution set to fixed
- Status changed from positive_review to closed
comment:6 Changed 6 years ago by
- Commit 1dc86119db2155471346b82d99877af72bd898cf deleted
- Resolution fixed deleted
- Status changed from closed to new
Ok this prevents all patches from being applied. Note the difference:
$ for patch in "patches/*.patch"; do echo $patch; done patches/numpy-1.10.1-asarray_conversion.patch patches/numpy-1.10.2-no-hardcode-blas.patch $ for patch in patches/*.patch; do echo $patch; done patches/numpy-1.10.1-asarray_conversion.patch patches/numpy-1.10.2-no-hardcode-blas.patch
comment:7 Changed 6 years ago by
Yup, that's a typo.
comment:8 Changed 6 years ago by
This raises a question: If the patchbot passed for the original version of this change, what is being patched in Numpy that there isn't a regression test for?
comment:9 Changed 6 years ago by
- Branch changed from 1dc86119db2155471346b82d99877af72bd898cf to u/embray/ticket-20487
- Commit set to 76b3369f7386d0082f7e7d19d78550a307fde610
- Status changed from new to needs_info
comment:10 Changed 6 years ago by
Numpy was not recompiled because you didn't change package-version.txt
comment:11 follow-up: ↓ 13 Changed 6 years ago by
Why would I change package-version.txt?
comment:12 Changed 6 years ago by
to trigger recompilation if necessary
comment:13 in reply to: ↑ 11 Changed 6 years ago by
Replying to embray:
Why would I change package-version.txt?
To elaborate a bit on this: the build system of Sage tracks versions of installed packages. If you change the spkg-install
script or change patches of a package, Sage will not automatically rebuild that package.
If you do want the package to be rebuilt, you need to increase the patchlevel in package-version.txt
. This means either appending .p0
or changing .pN
to .p(N+1)
.
You should always increase the patchlevel when changing the spkg-install
script or patches of a package unless there is a good reason not to. Typical reasons "not to":
- A completely trivial change, like a typo.
- A change which affects only the build system of the package but does not change the installed package (not even in a minor way).
- The change affects only a particular platform and the package did not build on that platform before.
Regardless of the above reasons, a reviewer should always check that the changed package builds and works fine. This means doing a forced rebuild of the package using sage -f PACKAGE
. Ideally, a build from scratch (make distclean; make
) should also be tested.
comment:14 follow-up: ↓ 15 Changed 6 years ago by
Okay, that sounds reasonable-ish. I'll touch the patch number then. Should I also just go ahead and squash my commits or do we not care about that?
comment:15 in reply to: ↑ 14 Changed 6 years ago by
Replying to embray:
Should I also just go ahead and squash my commits or do we not care about that?
I would say that Sage has a policy to not squash commits, at least not while a branch is in review.
comment:16 Changed 6 years ago by
- Commit changed from 76b3369f7386d0082f7e7d19d78550a307fde610 to 702cb85f74093dd905a9c8337ff096acd8e2e22d
Branch pushed to git repo; I updated commit sha1. New commits:
702cb85 | Bump patch level since a patch was removed.
|
comment:17 Changed 6 years ago by
- Status changed from needs_info to needs_review
comment:18 Changed 6 years ago by
- Milestone changed from sage-7.2 to sage-7.3
- Reviewers changed from Sebastien Gouezel to Sebastien Gouezel, Jeroen Demeyer
comment:19 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:20 Changed 6 years ago by
Builds fine on Linux too.
comment:21 Changed 6 years ago by
- Branch changed from u/embray/ticket-20487 to 702cb85f74093dd905a9c8337ff096acd8e2e22d
- Resolution set to fixed
- Status changed from positive_review to closed
I think there may also be an issue with Numpy and BLAS configuration. After rebasing my Cython branch I'm running into some build issues with SciPy? that I never encountered before, which seem to do with how Numpy is reporting what libraries it was built with. I need to spend more time investigating though.