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

[TopOpt_in_PETSc] WIP: new builder #3648

Merged
merged 17 commits into from
Mar 13, 2022
Merged

Conversation

rayegun
Copy link
Contributor

@rayegun rayegun commented Sep 19, 2021

This shouldn't be merged in its current state. There is a pretty big problem:

I currently have to vendor PETSc. The reason is that the current PETSc builder clobbers lots of files when it builds for each set of scalars (real/complex, single/double, and Int32/Int64). In particular it clobbers all of .h files in $includedir, and all of the $libdir/petsc folder which contains configuration files.

The version required by this library double_real_Int32 is built first, so only the .so file remains at the end. In order to have a dependency on PETSc these files need to be kept in some form. I'm just not sure how to do that correctly in the PETSc builder.

The second issue is that this repo has no license. Found it hiding in plain sight.

@rayegun
Copy link
Contributor Author

rayegun commented Sep 21, 2021

@vchuravy do you have any thoughts on this?

I can also try not using PETSc's build system to build TopOpt, and just manually edit the compiler calls. That would be fragile (they recommend you don't do that), but I could specify the location of the right files rather than using -lpetsc.

That still doesn't solve the issue with the PETSc builder deleting the .h files from previous (different number type) builds.

@vchuravy
Copy link
Member

I haven't looked at the PETSc builder in detail, maybe @jkozdon has ideas?

@jkozdon
Copy link
Contributor

jkozdon commented Sep 23, 2021

I think we may need to save ${PETSC_DIR}/${PETSC_ARCH}/include/petscconf.h (and maybe the other .h files in the directory).

I'm fine with the .h files being saved in some way. I think that you'd need to do that after this:

# add suffix to library name
if [[ "${target}" == *-mingw* ]]; then
# changing the extension from so to dll.
mv ${prefix}/lib/libpetsc.so.*.*.* "${libdir}/libpetsc_${1}_${2}_${3}.${dlext}"
elif [[ "${target}" == *-apple* ]]; then
mv ${prefix}/lib/libpetsc.*.*.*.${dlext} "${libdir}/libpetsc_${1}_${2}_${3}.${dlext}"
else
mv ${prefix}/lib/libpetsc.${dlext}.*.*.* "${libdir}/libpetsc_${1}_${2}_${3}.${dlext}"
fi

in the PETSc build script. Not sure if something like:

    # add suffix to library name
    if [[ "${target}" == *-mingw* ]]; then
        # changing the extension from so to dll.
        mv ${prefix}/lib/libpetsc.so.*.*.* "${libdir}/libpetsc_${1}_${2}_${3}.${dlext}"
    elif [[ "${target}" == *-apple* ]]; then
        mv ${prefix}/lib/libpetsc.*.*.*.${dlext} "${libdir}/libpetsc_${1}_${2}_${3}.${dlext}"
    else
        mv ${prefix}/lib/libpetsc.${dlext}.*.*.* "${libdir}/libpetsc_${1}_${2}_${3}.${dlext}"
    fi
    mkdir "${libdir}/include_${1}_${2}_${3}"
    mv ${prefix}/include/*.h "${libdir}/include_${1}_${2}_${3}"

would work?

@rayegun
Copy link
Contributor Author

rayegun commented Sep 28, 2021

I'll make a PR to try that. I forget, is there a way to test more than one builder locally?

Relatedly, what should I do about the libraries? Just manually build against libpetsc_${1}_${2}...? Or use -lpetsc and make a symlink to the required library? I'll have to sidestep PETSc's build variables/system in order to do the former.

@mohamed82008
Copy link

bump

@mohamed82008
Copy link

Sorry to bother you all again with this, but is there any hope this PR can be done in the near future?

@rayegun
Copy link
Contributor Author

rayegun commented Jan 25, 2022

#3801 needs to be finished up. In particular I need to figure out how to do what Elliot recommended there towards the bottom. I have a few more questions about that, I'll ping him on Slack. Once that's done this should just work like normal.

Sorry about the long gap there! The PETSc build system is a little frustrating to work around.

@mohamed82008
Copy link

No worries, I appreciate your help :)

@mohamed82008
Copy link

With #3801 merged, is this PR not blocked anymore?

@rayegun
Copy link
Contributor Author

rayegun commented Mar 10, 2022

I have to finish it up but no it's not blocked anymore. I'll try to finish it tonight

@rayegun
Copy link
Contributor Author

rayegun commented Mar 10, 2022

I expect almost all of those are red from using patchelf right now.

@giordano this will now compile but running ./topopt within the builder results in:

Inconsistency detected by ld.so: dl-call-libc-early-init.c: 37: _dl_call_libc_early_init: Assertion `sym != N86_64-linux-gnu/sys-root/lib -L/opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/lib -Wl,-rpath,/opt/x86_64-lin

(it gets cut off I suppose)

There's two questions here:

  1. Will I be forced to use patchelf so long as the SONAMEs in petsc_jll are still libpetsc.so.3.16?
  2. What does this error even mean? It looks like perhaps topopt could not dynamically link to libpetsc_double_real_Int32.so? And why does Julia detect that dlopen is fine? This is the function where the assertion fails:
   25 void
   26 _dl_call_libc_early_init (struct link_map *libc_map, _Bool initial)
   27 {
   28   /* There is nothing to do if we did not actually load libc.so.  */
   29   if (libc_map == NULL)
   30     return;
   31 
   32   const ElfW(Sym) *sym
   33     = _dl_lookup_direct (libc_map, "__libc_early_init",
   34                          0x069682ac, /* dl_new_hash output.  */
   35                          "GLIBC_PRIVATE",
   36                          0x0963cf85); /* _dl_elf_hash output.  */
   37   assert (sym != NULL);
   38   __typeof (__libc_early_init) *early_init
   39     = DL_SYMBOL_ADDRESS (libc_map, sym);
   40   early_init (initial);
   41 }

Is this even a valid test of whether ./topopt will work on a users machine.

@giordano
Copy link
Member

Ok, the problem is that relpath(new_libdir, dirname(path)) at https://github.com/JuliaPackaging/BinaryBuilder.jl/blob/f20e6d0b9ddd66a097fa4a0beab70c2ea2f9b704/src/auditor/dynamic_linkage.jl#L440 returns

julia> relpath("/workspace/destdir/lib/petsc/double_real_Int32/lib", "/workspace/destdir/lib")
"petsc/double_real_Int32/lib"

and so normalize_rpath gets the path completely wrong.

@giordano
Copy link
Member

I can confirm this is fixed by JuliaPackaging/BinaryBuilder.jl#1186.

T/TopOpt_in_PETSc/build_tarballs.jl Outdated Show resolved Hide resolved
T/TopOpt_in_PETSc/bundled/Makefile Outdated Show resolved Hide resolved
@rayegun
Copy link
Contributor Author

rayegun commented Mar 11, 2022

Thanks a bunch @giordano, so fast I didn't have a chance to fix! How long will it take to merge the fix to BB?

@giordano giordano closed this Mar 12, 2022
@giordano giordano reopened this Mar 12, 2022
@giordano
Copy link
Member

All green now 🙂

@rayegun rayegun marked this pull request as ready for review March 13, 2022 04:15
@giordano giordano merged commit 2224462 into JuliaPackaging:master Mar 13, 2022
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.

5 participants