Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump OpenBLAS 0.3.9 #621

Merged
merged 7 commits into from Mar 27, 2020

Conversation

haampie
Copy link
Contributor

@haampie haampie commented Mar 14, 2020

I've rebased the patches to 0.3.9, let's see if they are still working properly.

#endif
}

-#if defined(_MSC_VER) && !defined(__clang__)
Copy link
Contributor Author

@haampie haampie Mar 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Keno I think your patch was included in OpenBLAS 0.3.9 already (OpenMathLib/OpenBLAS@23f322f), except for those lines. Otherwise it seems to be just formatting changes. Should I simplify the .patch to include only these lines / are they still relevant?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Keno Could you review? Would be nice to get this merged.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, most of this patch still applies. You can remove the part that was already applied, but other than that it's still needed. Somebody needs to work with upstream to get this all cleaned up, but I haven't had the time so far.

Copy link
Contributor Author

@haampie haampie Mar 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! @Keno, I checked out OpenBLAS 0.3.7, applied the patch, rebased to 0.3.9, and extracted the patch again, so should be fine.

The new commit below basically just removes all the autoformatting / whitespace changes from the patch that are not really important I imagine.

@haampie haampie mentioned this pull request Mar 15, 2020
2 tasks
@haampie
Copy link
Contributor Author

haampie commented Mar 23, 2020

So this should be ready then :).

I'll add the same Windows patch we have here to JuliaLang/julia#35113 as well

@ViralBShah
Copy link
Contributor

@giordano Will you merge it when it looks all good?

@giordano
Copy link
Member

@haampie while we are here, can you please delete the static archives from the tarballs? See #642. Toward the end of the build script, you can do something like

rm ${prefix}/lib/*.a

@haampie
Copy link
Contributor Author

haampie commented Mar 24, 2020

That would be *.lib on windows? 🤔 (nvm, I could just download the binaries 😅 to see)

@haampie
Copy link
Contributor Author

haampie commented Mar 24, 2020

@haampie
Copy link
Contributor Author

haampie commented Mar 24, 2020

So the last commit affects 0.3.5, 0.3.7 and 0.3.9.

But CI only builds for 0.3.9 now?

@giordano
Copy link
Member

Oh yeah, if there is an option to directly disable building the static archives it's even better: hopefully we also save building time!

@haampie
Copy link
Contributor Author

haampie commented Mar 24, 2020

Nah, it's only skipping them in make install iiuc :p but it is still better

@haampie
Copy link
Contributor Author

haampie commented Mar 24, 2020

Oof... an issue for Windows. I don't know anything about building for Windows.

With NO_STATIC=1 we only get

[12:24:43] -rw-r--r-- 1 root root 5598770 Mar 24 12:24 libopenblas.dll.a

what does .dll.a even mean... .dll.a == .lib?

I think it needs setopt null_glob, let's see 🤞

@haampie
Copy link
Contributor Author

haampie commented Mar 24, 2020

Yay, it's green again

I randomly downloaded some tarballs: Windows only contains libopenblas.dll.a, Mac only libopenblas64_.0.3.9.dylib + symlinks, Linux only libopenblas64_.0.3.9.so + symlinks. Seems correct.

And the sizes are -50% or so.

Ready to merge @giordano?

@giordano
Copy link
Member

Windows only contains libopenblas.dll.a

That file should not be there, it's a static archive, we don't need it. That's why I suggested to do

rm ${prefix}/lib/*.a

(or maybe rm -f to avoid fiddling with non-existing files) 🙂

@haampie
Copy link
Contributor Author

haampie commented Mar 24, 2020

Are you sure @giordano? From the previous release the contents are:

lib $ pwd
/home/.../Downloads/OpenBLAS.v0.3.7.x86_64-w64-mingw32-libgfortran4/lib
lib $ ls 
cmake  libopenblas64_.0.3.7.a  libopenblas64_.a  libopenblas64_.dll.a  pkgconfig

there's no .dll there.

Edit. Doh... Windows dll's are in bin/.

Ok, so we might have to file an issue at OpenBLAS then for installing import libs on Windows when setting NO_STATIC=1

@giordano
Copy link
Member

Windows dll's are in bin/

Yes, libraries for Windows need to live in bin/, 😉

@giordano
Copy link
Member

Looks like there is a conflict 😕

@haampie
Copy link
Contributor Author

haampie commented Mar 24, 2020

Or is there?!

@haampie
Copy link
Contributor Author

haampie commented Mar 24, 2020

Fixed the issue upstream, for a next release... OpenMathLib/OpenBLAS#2531

@haampie
Copy link
Contributor Author

haampie commented Mar 24, 2020

So, do we believe by now it is OK?

The import library / dll.a is back again...

@staticfloat staticfloat merged commit 4707e30 into JuliaPackaging:master Mar 27, 2020
@staticfloat
Copy link
Member

Thanks @haampie!

@haampie haampie deleted the bump-openblas-to-0.3.9 branch March 27, 2020 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants