search

icecube / photospline

https://docs.icecube.aq/photospline/v2.0.7/
BSD 2-Clause "Simplified" License
5 stars 12 forks source link

fixed LAPACK/BLAS detection for old cmake. #8

Closed olivas closed 6 years ago

olivas commented 6 years ago

On Ubuntu 16.04, cmake 3.5, and libopenblas-dev installed this would fail to find LAPACK/BLAS initially, run through the five vendors and eventually settle on "Generic" which doesn't look like intended behavior.

cnweaver commented 6 years ago

Is this expected to make a functional difference? Both before and after applying this patch I see the exact same set of libraries (including libopenblas.so.0) linked with the photospline-test-fit program. It does make the cmake output look a little nicer, so I have no objection, I'm just not sure if I'm missing the point.

olivas commented 6 years ago

There's no functional difference that I could tell. It does amount mostly, I think, to a cleanup of the build output. At first, not being that familiar with the build, I thought it failed to detect BLAS, but was also surprised that it seemed to build and link just find. Then thought maybe BLAS was optional, but I had to read a little more carefully to see that it did in fact find BLAS libs eventually and all was likely OK.

cnweaver commented 6 years ago

The existing output is ugly and confusing, so it would be good to do away with it. My only remaining concern, then, is whether there was some good reason it wasn't done this way all along; I'm going to attempt to summon jvansanten's expertise and memory of the matter.

jvansanten commented 6 years ago

I honestly can’t remember why I implemented the detection this way, though I suspect it was a ham-handed attempt to make it work only as a fall-back. If this patch makes the output clearer, that’s great.