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:

Status badges

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 embray

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.

comment:2 Changed 6 years ago by embray

  • Authors set to Erik Bray
  • 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:

1dc8611Remove Cygwin-only patch to numpy.linalg.setup

comment:3 Changed 6 years ago by gouezel

  • Status changed from needs_review to positive_review

Works for me as well.

comment:4 Changed 6 years ago by gouezel

  • Reviewers set to Sebastien Gouezel

comment:5 Changed 6 years ago by vbraun

  • 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 vbraun

  • 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 embray

Yup, that's a typo.

comment:8 Changed 6 years ago by embray

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 embray

  • Branch changed from 1dc86119db2155471346b82d99877af72bd898cf to u/embray/ticket-20487
  • Commit set to 76b3369f7386d0082f7e7d19d78550a307fde610
  • Status changed from new to needs_info

I'm marking this as "needs_info" out of concern that this was previously marked "fixed" and it's not clear how the issue was caught.


New commits:

0293325Remove Cygwin-only patch to numpy.linalg.setup
76b3369Don't put spurious quotes around wildcard expansion.

comment:10 Changed 6 years ago by vbraun

Numpy was not recompiled because you didn't change package-version.txt

comment:11 follow-up: Changed 6 years ago by embray

Why would I change package-version.txt?

comment:12 Changed 6 years ago by vbraun

to trigger recompilation if necessary

comment:13 in reply to: ↑ 11 Changed 6 years ago by jdemeyer

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: Changed 6 years ago by embray

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 jdemeyer

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 git

  • Commit changed from 76b3369f7386d0082f7e7d19d78550a307fde610 to 702cb85f74093dd905a9c8337ff096acd8e2e22d

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

702cb85Bump patch level since a patch was removed.

comment:17 Changed 6 years ago by embray

  • Status changed from needs_info to needs_review

comment:18 Changed 6 years ago by jdemeyer

  • 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 jdemeyer

  • Status changed from needs_review to positive_review

comment:20 Changed 6 years ago by jdemeyer

Builds fine on Linux too.

comment:21 Changed 6 years ago by vbraun

  • Branch changed from u/embray/ticket-20487 to 702cb85f74093dd905a9c8337ff096acd8e2e22d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.