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

[PETSc] Attempt to rebuild with good include files (and don't rename libpetsc) #3801

Merged
merged 20 commits into from
Mar 9, 2022

Conversation

rayegun
Copy link
Contributor

@rayegun rayegun commented Oct 23, 2021

This is just a first attempt at building PETSc a little differently. It may be totally the wrong way to do it, but it achieves two things: keeps generated include files for each scalar type version, and avoids renaming.

I'm not sure the latter is necessary, but it helps me avoid symlinking/digging into build systems for packages that depend on -lpetsc.

@rayegun rayegun changed the title attempt to rebuild with good include files (and don't rename libpetsc) [PETSc] Attempt to rebuild with good include files (and don't rename libpetsc) Oct 23, 2021
@vchuravy
Copy link
Member

cc: @jkozdon

P/PETSc/build_tarballs.jl Outdated Show resolved Hide resolved
@jkozdon
Copy link
Contributor

jkozdon commented Oct 25, 2021

I'm not a PETSc build expert, but this seems reasonable to me and cleaner than the mess I created.

@rayegun
Copy link
Contributor Author

rayegun commented Oct 25, 2021

I have no idea why it can't dlopen for libgfortran4, but at least it's consistent. I imagine it's probably necessary to expand libgfortran here right? Could we safely skip that version?

@giordano
Copy link
Member

I have no idea why it can't dlopen for libgfortran4, but at least it's consistent.

That's the only version for which dlopening is attempted, because that's the actual host platform, the only one that can dlopen libraries. If it fails there it likely fails everywhere else as well, we just can't test it.

I imagine it's probably necessary to expand libgfortran here right?

You're already doing it?

Could we safely skip that version?

Not at all 🙂

@rayegun
Copy link
Contributor Author

rayegun commented Oct 25, 2021

I had imagined as much, but hoped otherwise. I haven't really changed anything involved in building, just the location. I'll build locally and check it out.

I imagine it's probably necessary to expand libgfortran here right?

As in, could we skip expansion. I don't think so, there are fortran libraries in PETSc AFAIK.

@rayegun
Copy link
Contributor Author

rayegun commented Oct 27, 2021

Finally looks good. The actual paths to files have been changed, and the include files are no longer in the include directory. But include files probably aren't used downstream (and they're in $libdir/petsc/... if necessary), and the library paths should(?) be handled by the wrapper.

P/PETSc/build_tarballs.jl Outdated Show resolved Hide resolved
P/PETSc/build_tarballs.jl Outdated Show resolved Hide resolved
P/PETSc/build_tarballs.jl Outdated Show resolved Hide resolved
@giordano
Copy link
Member

The tarball for x86_64-linux-gnu-libgfortran5 has size ~144 MiB, the build from v3.15.2+0 has size about half of that. What am I missing? Are there duplicates or we're now building "more stuff"?

@rayegun
Copy link
Contributor Author

rayegun commented Oct 27, 2021

So we do have ~8x the number of include files, of which there are many, otherwise I don't think anything should have changed. In a bit I can build locally and see what's up.

@giordano
Copy link
Member

A bit unfortunate that the pkgconfig files are not in the standard paths anymore, users will need to manually update PKG_CONFIG_PATH to add the specific version they want, but that's maybe to be expected?

@giordano
Copy link
Member

So we do have ~8x the number of include files, of which there are many, otherwise I don't think anything should have changed. In a bit I can build locally and see what's up.

I don't think compressed header files can justify 70 MiB extra in tarball size.

@giordano
Copy link
Member

giordano commented Oct 27, 2021

@giordano
Copy link
Member

I don't think compressed header files can justify 70 MiB extra in tarball size.

% find lib/petsc -type d -name "include" | xargs du -hsc 
5.3M    lib/petsc/single_real_Int64/include
5.3M    lib/petsc/single_real_Int32/include
5.3M    lib/petsc/single_complex_Int64/include
5.3M    lib/petsc/single_complex_Int32/include
5.3M    lib/petsc/double_real_Int64/include
5.3M    lib/petsc/double_real_Int32/include
5.3M    lib/petsc/double_complex_Int64/include
5.3M    lib/petsc/double_complex_Int32/include
43M     total

This is the total size of the uncompressed include directories.

@giordano
Copy link
Member

giordano commented Nov 13, 2021

@staticfloat @vchuravy do you have ideas about how to get out of this mess? Below is a summary of my understanding, please do correct me if I'm missing something.

The main issue is that there are multiple variants of libpetsc, with different kind of precisions and types, and we want all of them. What we do at the moment is to build them like this (build_petsc is a function which basically repeats the build steps for the different variants):

build_petsc double real Int32
build_petsc single real Int32
build_petsc double complex Int32
build_petsc single complex Int32
build_petsc double real Int64
build_petsc single real Int64
build_petsc double complex Int64
build_petsc single complex Int64

they're all installed in ${libdir}, with different basenames:

% tar tzvf PETSc.v3.15.2.x86_64-linux-musl-libgfortran3.tar.gz|grep "lib/libpetsc_"
-rwxr-xr-x 0/0        22284552 1970-01-01 01:00 lib/libpetsc_double_complex_Int32.so
-rwxr-xr-x 0/0        22468912 1970-01-01 01:00 lib/libpetsc_double_complex_Int64.so
-rwxr-xr-x 0/0        22072824 1970-01-01 01:00 lib/libpetsc_double_real_Int32.so
-rwxr-xr-x 0/0        22265376 1970-01-01 01:00 lib/libpetsc_double_real_Int64.so
-rwxr-xr-x 0/0        22964528 1970-01-01 01:00 lib/libpetsc_single_complex_Int32.so
-rwxr-xr-x 0/0        23169368 1970-01-01 01:00 lib/libpetsc_single_complex_Int64.so
-rwxr-xr-x 0/0        22146472 1970-01-01 01:00 lib/libpetsc_single_real_Int32.so
-rwxr-xr-x 0/0        22343120 1970-01-01 01:00 lib/libpetsc_single_real_Int64.so

but they all have the same soname (libpetsc.so), which at the moment makes everything works in PETSc.jl by accident: currently PETSc_jll.jl offers (via JLLWrappers) as variable the full absolute path of the library, and that's what you'd ccall into, but this would be changed by JuliaPackaging/JLLWrappers.jl#31 (which was requested by @vchuravy), which would offer as variable the soname of the libs, which are all the same 💥

On the other hand, with this PR @Wimmerer would like to have libraries all with the same basename (and soname again...) but installed in different subdirectories of ${libdir}, in order to make it easier to link other libraries against libpetsc, but honestly I have no idea how this is going to work at all: depending libraries would have a dynamic link to libpetsc.so, they have no clue which of the many libraries we offer here would be picked up. @jkozdon said that PETSc.jl would still work after this PR, but JuliaPackaging/JLLWrappers.jl#31 would break it again (this is similar to the problem of downstream libraries dynamically linking to libetsc.so).

I can't see any simple solutions. Only thing would be to force all different variants to have different basenames & sonames, but that'd be a massive complication in build recipes, both in PETSc_jll builder and all downstream packages, in addition to being incompatible with all other libraries out there linking to libpetsc.

@jkozdon
Copy link
Contributor

jkozdon commented Nov 13, 2021

I should say, if we need to only support loading of a single PETSc library at a time that's fine. There is no real strong use case for supporting multiple libraries simultaneously, but I do think that flexibility in types is needed in the library (even if not in the individual run) for multiple real and float type (Float64, Float32, real, and complex).

I haven't had time to really scope out how to do this at a user facing level yet though.

@jkozdon
Copy link
Contributor

jkozdon commented Nov 13, 2021

Also, from what I recall this PR isn't really the issue, more the overall way that PETSc.jl is currently structured.

I think that it would probably be fine to merge this PR, since the problem exists with the current PETSc binaries as well.

If I remember correctly, FFTW does something similar as well so PETSC.jl might not be the only problematic package.

@giordano
Copy link
Member

If I remember correctly, FFTW does something similar as well so PETSC.jl might not be the only problematic package.

They are two different libraries with different sonames:

julia> using FFTW_jll

julia> filter!(l -> contains(l, "SONAME"), readlines(`readelf -a $(FFTW_jll.libfftw3f_path)`))
1-element Vector{String}:
 " 0x000000000000000e (SONAME)             Library soname: [libfftw3f.so.3]"

julia> filter!(l -> contains(l, "SONAME"), readlines(`readelf -a $(FFTW_jll.libfftw3_path)`))
1-element Vector{String}:
 " 0x000000000000000e (SONAME)             Library soname: [libfftw3.so.3]"

Here all sonames are the same, that's the problem.

@staticfloat
Copy link
Member

Alright Mosè successfully nerdsniped me into writing up a minimal example showcasing one possible path forward here. The basic idea is to use dlmopen() to load libraries into a separate linker namespace, then directly dlsym() any symbols you want to use from that new namespace.

We could teach Julia how to use dlmopen() on Linux platforms, or we can use a thin wrapper as I defined in that repository. The example showcases using a wrapper to load libconfigurable.so, which is a library that gets built multiple times with different configurations but the same SONAME and basename, as well as libdependent.so which is a library that needs to use symbols from libconfigurable. Note that even though I only have one example of libdependent.so on disk, I can load it multiple times if I use multiple linker namespaces.

This is a glibc-only thing, it doesn't work anywhere else, but perhaps that's okay for this? :P

@giordano
Copy link
Member

giordano commented Nov 19, 2021

Thanks for looking into this! So this would work for for dlopen-ing the libraries and ccall-ing into them on the Julia side, right? But what about downstream libraries that needs to dynamically link to a specific implementation of libpetsc.so? The dynamic linker would find the "first" libpetsc.so, whatever it is? I don't think we have a way to "point" it to the right one, do we?

@staticfloat
Copy link
Member

That’s exactly what the example showcases; libdependent dynamically links against libconfigurable, without using dlopen. libwrapper loads two copies of each of those libraries at once, in two different namespaces.

@rayegun
Copy link
Contributor Author

rayegun commented Jan 20, 2022

To pick this back up, I'm not super clear here on whether I would need to make modifications to the PETSc build system, and the build systems of C libraries that depend on PETSc. It looks like I that dependsonPETSc.so finds the correct PETSc (that's handled by the PETSc build variables typically).

@jkozdon
Copy link
Contributor

jkozdon commented Jan 20, 2022

@Wimmerer I am happy for you to modify PETSc_jll so that it works for what you are trying to do. It seems that we will have some reworking of PETSc.jl to do regardless of your changes.

@mohamed82008
Copy link

bump :)

@giordano
Copy link
Member

giordano commented Mar 4, 2022

I'm still convinced this is hopeless in the general case.

@rayegun
Copy link
Contributor Author

rayegun commented Mar 4, 2022

I'm going to modify this to just include the headers in specific subfolders, and just handle the renaming in downstream build system manually (I've been trying to make a new findPETSC.cmake, which is a whole lot of fun).

That removes the issue of the name (although I'll probably keep double_real as libpetsc), but the sonames will still eventually be an issue no matter what I do here right @giordano?

@giordano
Copy link
Member

giordano commented Mar 4, 2022

Unless you fix the sonames with patchelf and install_name_tool, yes, the names will be a problem. And even if you manually fix the sonames, downstream packages won't expect different names because PETSc never had this concept to start with, so you'll have to fix also all downstream packages, which is double more work, error-prone, and annoying to say the least.

@rayegun
Copy link
Contributor Author

rayegun commented Mar 4, 2022

If I just add the headers for now in subdirectories could that be merged? And we wait until calling by path breaks? That way we at least have a way to try and build downstream packages without changing the status quo.

@rayegun
Copy link
Contributor Author

rayegun commented Mar 8, 2022

(If this passes) is this ok to go @giordano?

@giordano giordano merged commit aa1766b into JuliaPackaging:master Mar 9, 2022
@mohamed82008
Copy link

🎉🎉🎉🎉🎉

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.

7 participants