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

[libflame] Initial build scripts. #8671

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

jd-foster
Copy link
Contributor

@jd-foster jd-foster commented May 14, 2024

Build scripts for libflame

Addresses #7660

Currently MacOS is omitted until linking errors can be resolved cf. flame/libflame#100

@imciner2
Copy link
Member

Why is it split into a build_tarballs and a common? This looks simple enough we should just keep it all in a single build_tarballs to make updating the recipe simple.

@jd-foster
Copy link
Contributor Author

Yes, that also occurred to me after the initial commit. Will reduce to a single build_tarball

L/libflame/build_tarballs.jl Outdated Show resolved Hide resolved
L/libflame/build_tarballs.jl Outdated Show resolved Hide resolved
@jd-foster
Copy link
Contributor Author

jd-foster commented May 19, 2024

So clearly libflame.1.dll !== libflame-1.dll !== libflame.dll ...

[ Info: Checking lib/libflame.1.dll with RPath list String[]
[ Info: Ignored system libraries msvcrt.dll, KERNEL32.dll
┌ Warning: lib/libflame.1.dll should be in `bin`!
└ @ BinaryBuilder.Auditor /cache/julia-buildkite-plugin/depots/e2fd9734-29d8-45cd-b0eb-59f7104f3131/packages/BinaryBuilder/L8tBh/src/Auditor.jl:233
┌ Warning: Simple buildsystem detected; Moving all `.dll` files to `bin`!
└ @ BinaryBuilder.Auditor /cache/julia-buildkite-plugin/depots/e2fd9734-29d8-45cd-b0eb-59f7104f3131/packages/BinaryBuilder/L8tBh/src/Auditor.jl:244
[ Info: Normalising timestamps in import library /cache/build/yggy-amdci7-15/julialang/yggdrasil/L/libflame/build/x86_64-w64-mingw32/NiP3KxjS/x86_64-w64-mingw32-libgfortran5-cxx11/destdir/lib/libflame.dll.a
[ Info: Checking license file
[ Info: Found license file(s): LICENSE
[ Info: Found a valid dl path libatomic-1.dll while looking for libflame
[ Info: Found a valid dl path libblastrampoline-5.dll while looking for libflame
[ Info: Found a valid dl path libflame.1.dll while looking for libflame
[ Info: Found a valid dl path libgcc_s_seh-1.dll while looking for libflame
[ Info: Found a valid dl path libgfortran-5.dll while looking for libflame
[ Info: Found a valid dl path libgomp-1.dll while looking for libflame
[ Info: Found a valid dl path libquadmath-0.dll while looking for libflame
[ Info: Found a valid dl path libssp-0.dll while looking for libflame
[ Info: Found a valid dl path libstdc++-6.dll while looking for libflame
[ Info: Found a valid dl path libwinpthread-1.dll while looking for libflame
[ Info: Could not locate libflame inside ["/cache/build/yggy-amdci7-15/julialang/yggdrasil/L/libflame/build/x86_64-w64-mingw32/NiP3KxjS/x86_64-w64-mingw32-libgfortran5-cxx11/destdir/bin"]
┌ Error: Built libflame but libflame still unsatisfied:
└ @ BinaryBuilder /cache/julia-buildkite-plugin/depots/e2fd9734-29d8-45cd-b0eb-59f7104f3131/packages/BinaryBuilder/L8tBh/src/AutoBuild.jl:946
ERROR: LoadError: Cannot continue with unsatisfied build products!

@jd-foster jd-foster requested a review from imciner2 May 20, 2024 03:38
@jd-foster
Copy link
Contributor Author

Note: now that it's working on mingw32, we should see if it's possible to upstream the configure script patches, which are based on the newer BLIS scripts anyway.

@imciner2
Copy link
Member

I see that it is currently building the static library. In general, we do like to avoid having those in the main JLLs because they can really eat up space (the artifact here shows it is 15MB on Linux). Can we disable the static library build?

Also, are we sure the aarch64 macos build is working properly? I downloaded the artifact to check, and the dylib it makes is extremely tiny compared to the static library (and also compared to the size it is on Linux):

$ ll libflame.v5.2.0.x86_64-linux-gnu/lib/
/home/im1716/dev/julia/binary
Permissions Size User   Group  Date Modified Date Created Name
.rw-r--r--@  15M im1716 im1716  1 Jan  1970  20 May 10:07 libflame.a
lrwxrwxrwx@    - im1716 im1716  1 Jan  1970  20 May 10:07 libflame.so -> libflame.so.1.0.0
lrwxrwxrwx@    - im1716 im1716  1 Jan  1970  20 May 10:07 libflame.so.1 -> libflame.so.1.0.0
.rwxr-xr-x@ 8.7M im1716 im1716  1 Jan  1970  20 May 10:07 libflame.so.1.0.0

$ ll libflame.v5.2.0.aarch64-apple-darwin/lib/
/home/im1716/dev/julia/binary
Permissions Size User   Group  Date Modified Date Created Name
.rwxr-xr-x@  17k im1716 im1716  1 Jan  1970  20 May 10:07 libflame.1.0.0.dylib
lrwxrwxrwx@    - im1716 im1716  1 Jan  1970  20 May 10:07 libflame.1.dylib -> libflame.1.0.0.dylib
.rw-r--r--@ 7.9M im1716 im1716  1 Jan  1970  20 May 10:07 libflame.a
lrwxrwxrwx@    - im1716 im1716  1 Jan  1970  20 May 10:07 libflame.dylib -> libflame.1.0.0.dylib

Can someone on macos confirm what is in this dylib?

@jd-foster
Copy link
Contributor Author

$ nm libflame.v5.2.0.x86_64-apple-darwin/lib/libflame.dylib
                 U dyld_stub_binder


# Build the tarballs
build_tarballs(ARGS, name, version, sources, script, platforms, products, dependencies;
preferred_gcc_version=v"11", lock_microarchitecture=false, julia_compat="1.6")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
preferred_gcc_version=v"11", lock_microarchitecture=false, julia_compat="1.6")
preferred_gcc_version=v"11", clang_use_lld=false, lock_microarchitecture=false, julia_compat="1.6")

Lets try not using LLD and instead using the other linker to see if these flags work then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mixed up the gcc ld -whole-archive vs. clang lld -force_load but didn't seem to like it anyway. Might need to try your approach.

Copy link
Member

Choose a reason for hiding this comment

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

Mixed up the gcc ld -whole-archive vs. clang lld -force_load

  1. this isn't gcc vs clang, but Linux vs macOS
  2. this is why we provide the flagon utility.

@jd-foster
Copy link
Contributor Author

@imciner2 I think I've done as much as I can here with what I know. What do you think should be next steps?

@imciner2
Copy link
Member

I don't know how to get the mac build to use the static library to compile the shared library. The best fix would be to have it just compile the shared library using each individual object file again, since then the linker won't remove anything, however I see that upstream says there is a command-line length issue when doing that - hence this solution.

@ViralBShah
Copy link
Member

Since the build is all green, is it ok to merge, or is mac still broken?

@imciner2
Copy link
Member

The macOS dylib is still tiny compared to the static library - 17.2kB vs. 7.9MB, so I think it is still broken.

@jd-foster
Copy link
Contributor Author

I think we should try to port over how BLIS does it, since the make script is a downstream improvement on libflame.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants