Add TOPCOM 0.17.8 builder #3900
Conversation
|
Thanks a lot for your contribution! I've pushed a change to avoid the symlinks, which I didn't really like (although I can see why you had to do that...). Tell me what you think 🙂 I think my change is less maintainable (it's a large patch), but at least the build script is more straightforward. I'm happy to revert it if you don't like it, I did it mostly as an exercise 😄 |
giordano
force-pushed
the
lk/TOPCOM
branch
2 times, most recently
from
ab33eae
to
f0a91b1
Compare
|
@giordano @lkastner what do you think of this one instead: benlorenz@770ddb7 |
|
I have also added a commit to build the shared libraries. They can't be used directly from Julia as they are since it's C++, but may be useful in the future nevertheless. Happy to get feedback on this as well! |
|
@benlorenz that looks great and also makes my last commit useless (but it's along the same lines) 😁 |
|
Yeah, I think I like your change (much) better than mines 😄 |
|
unfortunately my version doesn't seem to work on windows yet ... |
Neither does mine 😂 Building the shared library seems to hit undefined symbols (only on the platforms where the linker actually check them by default). And also dlopening the libs fails |
|
I disabled windows for now, lets see if the other platforms work. The libraries libTOPCOM and libREGCHECK seem to depend on each other which is not really feasible like that on windows, and might cause the dlopen problems as well. I can't really test all the platforms locally as I constantly get those annoying download failures: |
|
@staticfloat we're getting these download errors very frequently now, see also JuliaPackaging/BinaryBuilderBase.jl#178 (comment) |
|
@benlorenz do you have PkgServer disabled by any chance? |
and enable libraryproducts libCHECKREG needs to link against libTOPCOM
|
I have added the LibraryProducts now, dlopen does seem to work during the audit with the added libs, I will try to have a look at mingw tomorrow.
I did add If I try to download such a file manually with wget I get the following: Note that the total duration is slightly more than 2 minutes and I have seen even about 5 minutes and in some cases wget also failed after a while. Most of the time it is just stuck at |
Exactly, I wonder if Pkg has a rather low timeout, while GitHub is clearly taking a lot to serve the file. Response time from PkgServer is way faster for me instead: https://pkg.julialang.org/artifact/daf4fe73a24fc28599072795093e51dbfa6397c1 |
circular library-dependencies are fun and try splitting up the build_tarballs to remove the LibraryProduct for windows
|
I have set mingw to do static linking and removed the From my point of view this is ready now. |
|
@giordano @benlorenz In the end almost everything got replaced, so thank you for doing all the work. ;) |
| products_unix = [ | ||
| LibraryProduct("libTOPCOM", :libTOPCOM), | ||
| LibraryProduct("libCHECKREG", :libCHECKREG), | ||
| products_win... | ||
| ] |
There was a problem hiding this comment.
I'm slightly unhappy about having different products on different platforms, this can be confusing for users/package developers who'd expect to find libTOPCOM everywhere if TOPCOM_jll.is_available() is true. I thought about including products also in JuliaPackaging/BinaryBuilderBase.jl#174, but it looked error-prone to me. What do you think?
There was a problem hiding this comment.
We could leave out these products for all platforms or just remove the mingw platforms altogether, both would be fine for me.
But I really don't want to reorganize the source for these libraries, and currently both need symbols from the other one.
There was a problem hiding this comment.
We might also split this up into a libTOPCOM_jll which has just the shared libraries for unix platforms and TOPCOM_jll for all platforms with the binaries (which then might use platform-dependent dependencies ....). But this seems to be a little bit too much effort.
There was a problem hiding this comment.
But this seems to be a little bit too much effort.
Agreed. Ok, well, let's merge this as it is and let's see how it goes! Thanks everybody!
If it wasn't for the weird requirement about having gmp and cddlib as static libraries in a very specific location, your initial PR would have been perfect! |
* Add TOPCOM 0.17.8 builder * T/TOPCOM: Expand to remaining platforms * [TOPCOM] less static linking and some minor improvements * [TOPCOM] link libraries against gmp and cdd and enable libraryproducts libCHECKREG needs to link against libTOPCOM * [TOPCOM] link statically on windows circular library-dependencies are fun and try splitting up the build_tarballs to remove the LibraryProduct for windows Co-authored-by: Benjamin Lorenz <[email protected]>
Add builder for TOPCOM