GenericMappingTools / GMT.jl

Generic Mapping Tools Library Wrapper for Julia
Other
194 stars 28 forks source link

Executing commands at top level #1049

Closed maleadt closed 1 year ago

maleadt commented 1 year ago

GMT.jl currently executes a command (gmt --show-library) at top level: https://github.com/GenericMappingTools/GMT.jl/blob/466ca0dbf68eb72fda5a8456f57c1ad86e569420/src/GMT.jl#L63

This is strongly discouraged. Instead, you should use a deps/build.jl to perform discovery at package installation time, or during __init__, but not during precompilation. Specifically, this is incompatible with PkgEval, so we can't currently include this package (and dependents like SeisPDF.jl) in our daily testing of the upcoming Julia version.

joa-quim commented 1 year ago

I fear this is not going to be easy. Any way that I can test that PkgEval will accept a future solution for this?

maleadt commented 1 year ago

Yes, PkgEval is easy to use, but the issue caused by the toplevel gmt invocation doesn't seem to be easy to reproduce (and it's currently masked by an unrelated Conda issue). Generally though, doing these kind of operations during precompilation is not a great idea, regardless of the PkgEval issue. Why is it hard to restructure how the library is determined?

maleadt commented 1 year ago

Hmm, upon closer inspection I think the PkgEval incompatibility is caused by a different issue. I definitely saw it crash once because of the top-level command, but that does not seem to be the reason for the consistent failures to test GMT/SeisPDF. So feel free to close this, although I would definitely recommend replacing the global commands by something more reliable/safe.

joa-quim commented 1 year ago

Trying to address the Why is it hard to restructure how the library is determined?

Honestly I don't remember how I ended up with this solution but I do know why. The big main issue is that I can't build GMT with the BinaryBuilder. Why? Because it forces that everything is buildable from Unix in a cross compiling way and GMT does not build with MinGW. Along the time (GMT is > 30 years old) we learned how to build GMT on Windows using the Microsoft compiler. We had to learn about all the cross dlls shits, on how text files mus be open in text mode and so on. And once we finally did it there was no more motivation to invest in MinGW/Cygwin, all with their own head-aches.

So I can't build GMT with MinGW and I'm not interested either in a BinaryBuilder package that would use a GDAL that is not able to build with NetCDF/HDF libraries. Also here the problem seems to lie on trying to force a unix way only and Windows part not being satisfied. At the end, this harms both sides of this story.

Not having a GMT_JLL what I do is:

I'll scratch my head again trying to come out with something more robust to store/find the names of those shared libs.

You mention also a crash in the tests. Unfortunately I suspect what it was but I've tried a lot and can't find why there a random crash apparently caused by the module makecpt. It doesn't crash if I execute it thousands of times in the REPL but it triggers some GC boom when run from the tests. But not always in same place and not even always.

maleadt commented 1 year ago

I'm not arguing for a JLL -- that would solve the problem too, but it's understandably more invasive -- but just for the fact that the whole discovery/building process ideally doesn't happen at toplevel, but either during Pkg.build or at run time. For the former, you'd dump the library names in an ext.jl that you then load from your packages (you already seem to do this to some extent), for the latter you'd make the accessors for your library lazy (i.e. make them functions, libfoo(), you can use that with ccall nowadays). And in both cases it's recommended to put files you download/build in a scratch space using Scratchspaces.jl. Both these recommendations ensure that your package is optimally precompilable, and that the resulting package compilation cache is reusable regardless of the binary set-up.

But again, I think that the issue I was seeing on PkgEval was related to something else, so just treat the above as best practices.

joa-quim commented 1 year ago

Thanks for the advises, I'll give it a try when some more urgent issues are finished. Maybe https://github.com/timholy/SnoopCompile.jl/issues/307 is related to this anyhow.

maleadt commented 1 year ago

FYI, ran into this again in the context of another package, AnyMOD.jl. The problem that's common between that package and GMT.jl is the global invocation of Conda.

joa-quim commented 1 year ago

Is it by any chance related to this. In particular an error like (even if caused by other shared libs)?

ERROR: LoadError: could not load library "/tmp/jl_OnmZUn/conda/3/lib/libgmt.so" /opt/hostedtoolcache/julia/1.8.3/x64/bin/../lib/julia/libstdc++.so.6: version `GLIBCXX_3.4.30' not found (required by /tmp/jl_OnmZUn/conda/3/lib/./libgdal.so.31)

I thought the libstdc++ issue had finally been ingested but unfortunately it skipped 1.8 again.

maleadt commented 1 year ago

No, I encountered that error too, but only with Julia 1.8. Running under 1.9 (specifically, the latest nightly, but we should have an alpha soon) worked fine.

joa-quim commented 1 year ago

That error wouldn't bother me in particular if it wasn't for the situation that it's stopping me to automatically register new versions since last July.

joa-quim commented 1 year ago

Done in #1077

maleadt commented 1 year ago

Thanks. Could you give another ping when this enters a release? I'll remove the PkgEval blacklist then.

joa-quim commented 1 year ago

It's now in v0.44.0 Thanks for the push to this. Petty that it made zero difference with the SnoopPrecompile issue. Using it only makes the precompile time go up but no speedup whatsoever.