Skip to content
This repository has been archived by the owner on Dec 9, 2018. It is now read-only.

Fix and clean up comments in cblas64/128 #154

Merged
merged 3 commits into from
Feb 3, 2016
Merged

Conversation

vladimir-ch
Copy link
Member

No description provided.

@vladimir-ch
Copy link
Member Author

I'm confused. Why does the build fail due to missing sqrt and why has the coverage increased?

@kortschak
Copy link
Member

I don't have time to look into this for the next few days. Both things are mysterious.

@kortschak
Copy link
Member

Has coverage increased because total line count has decreased?

@vladimir-ch
Copy link
Member Author

I don't get the coveralls UI, so I cannot confidently interpret it. My command line says:

$ wc -l cblas128/cblas128.go cblas64/cblas64.go 
  510 cblas128/cblas128.go
  510 cblas64/cblas64.go
 1020 total

$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.

$ wc -l cblas128/cblas128.go cblas64/cblas64.go
  497 cblas128/cblas128.go
  497 cblas64/cblas64.go
  994 total

@kortschak
Copy link
Member

The link failure isn't repeatable on my machine here, but a restart on travis shows it is there. Presumably libmath is not being specified somewhere that we were getting for free previously.

@vladimir-ch
Copy link
Member Author

I tried to restart a rebuild of master and now it is failing too, previously it wasn't. Shall I try to add -lm to gonum/blas/.travis/linux/OpenBLAS/install.sh?

@kortschak
Copy link
Member

I guess it's worth a try. Please take a look at the failure on Brendan's dsterf PR in lapack and see if that failure is the same - I restarted one to see the log (travis doesn't like this machine for unknowable reasons) and it passed, but the rest failed.

@vladimir-ch
Copy link
Member Author

That failure is also from the linker but the reason is different, many errors about undefined references to LAPACKE functions.

@kortschak
Copy link
Member

kortschak commented Feb 2, 2016 via email

@vladimir-ch
Copy link
Member Author

Adding -lm seems promising, the branch openblas-lm is all green on travis. Submitting PR.

@kortschak
Copy link
Member

LGTM

Presumably you will rebase this onto the new master with -lm?

@vladimir-ch
Copy link
Member Author

The build passes, no rebase is necessary. Merging.

@vladimir-ch vladimir-ch merged commit a38f644 into master Feb 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants