Opened 6 months ago

Closed 6 months ago

#27438 closed defect (fixed)

Properly re-enable USE_TLS=1 when building OpenBLAS SPKG

Reported by: embray Owned by:
Priority: blocker Milestone: sage-8.7
Component: packages: standard Keywords: openblas
Cc: vbraun Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: cfc5a11 (Commits) Commit: cfc5a116ba40e3b22c9a8e04383969e856c1139f
Dependencies: Stopgaps:

Description

#27256 did not fully fix the issue it was intended to fix.

I missed the fact that the spkg-install has two invocations of make, the first one being somewhat obscured within an if statement.

The second make attempt is only run if the first one failed (e.g. due to the target CPU not being well supported).

Change History (10)

comment:1 Changed 6 months ago by embray

  • Keywords openblas added

comment:2 Changed 6 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-27438
  • Commit set to aebc79cc17b84929d68803c40f2578faaa697985
  • Status changed from new to needs_review

New commits:

aebc79cTrac #27438: Make sure to pass USE_TLS=1 to all make invocations in the script

comment:3 Changed 6 months ago by embray

  • Status changed from needs_review to needs_work

Before moving forward with this; I think I might know what's up with #27434, and if I'm right about it would like to incorporate its solution into this ticket.

comment:4 Changed 6 months ago by embray

  • Status changed from needs_work to needs_review

Whatever the remaining problem is in #27434 it's definitely more subtle than I originally thought, and I think not related in any way to the issue this is fixing, so I'll deal with it independently.

comment:5 Changed 6 months ago by jdemeyer

How should I test this, how do I know that OpenBLAS is really built with TLS enabled?

comment:6 Changed 6 months ago by jdemeyer

  • Summary changed from Followup to #27256 to Properly re-enable USE_TLS=1 when building OpenBLAS SPKG

comment:7 Changed 6 months ago by jdemeyer

  • Branch changed from u/embray/ticket-27438 to u/jdemeyer/ticket-27438

comment:8 Changed 6 months ago by jdemeyer

  • Commit changed from aebc79cc17b84929d68803c40f2578faaa697985 to cfc5a116ba40e3b22c9a8e04383969e856c1139f
  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

OK, I see -DUSE_TLS in the compiler output, so it works.

I do think that we should bump the package version since this is a functional change in the package.


New commits:

cfc5a11Bump OpenBLAS package version

comment:9 Changed 6 months ago by embray

Thank you, and yes I agree with bumping the package version (even though it means having to rebuild quite a bit, alas).

I'm not sure why we don't make $(BLAS) a prerequisite-only dependency for more things. As long as they are dynamically linking to the libopenblas.so it should't be necessary to rebuild most things that depend on it, exception being if there is a non-trivial dependency on anything that might change in the headers due to re-configuration.

comment:10 Changed 6 months ago by vbraun

  • Branch changed from u/jdemeyer/ticket-27438 to cfc5a116ba40e3b22c9a8e04383969e856c1139f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.