Migration of the code in here to FLINT_jll

[fingolfin]

BinaryBuilder now has support for inserting code into the __init__ function of JLLs using the init_block kwarg (see JuliaPackaging/BinaryBuilder.jl#791). This also available on Yggdrasil (see JuliaPackaging/Yggdrasil#1260).

With this, in principle we should now be able to move the initialization code in LoadFlint.jl directly into FLINT_jll; fewer moving parts should then make things easier for us.

Right now, though, I have some doubt about how the initialization should look. We do this in LoadFlint.jl right now:

  if !Sys.iswindows() && !__isthreaded[]
    #to match the global gmp ones
    fm = dlsym(libflint_handle, :__flint_set_memory_functions)
    ccall(fm, Nothing,
      (Ptr{Nothing},Ptr{Nothing},Ptr{Nothing},Ptr{Nothing}),
        cglobal(:jl_malloc),
        cglobal(:jl_calloc),
        cglobal(:jl_realloc),
        cglobal(:jl_free))
  end      

What I don't understand is why Windows builds and threaded mode are treated differently. Note that jl_malloc and friends ought to be thread safe (they just wrap malloc etc., plus some code which modifies TLS data).

The threaded check was added by @thofma in Nemocas/Nemo.jl@96663d9

The windows check was added by @wbhart in Nemocas/Nemo.jl@77e6667

Unfortunately in both cases it is not clear what the issues were, so...

[thofma]

As I wrote in Nemocas/Nemo.jl#604, jl_malloc has not been thread safe for quite a long time. Given that JuliaLang/julia#33284 has been merged, this should not be necessary anymore, at least on julia >= 1.3. Unfortunately this has not made it into LTS yet (JuliaLang/julia#34011). But I think given that this is a experimental feature, we can safely remove the !__isthreaded[] condition and tell users who want to try it to use a newer julia version.

I don't know about Windows.

[fingolfin]

@thofma thanks for the clarification. Ah yes, I was only looking at Julia 1.4, and JuliaLang/julia#33284 only appeared starting with that. So it'll be in the next LTS (1.6), good.

[fingolfin]

So I don't mind keeping the __isthreaded check, but it should then probably be restricted to Julia <= 1.3, and augmented by a comment explaining it. No problem.

[thofma]

If I understand it correctly, this init block will work only if the library is provided by the jll files. So it is limited to julia >= 1.3, right?

[thofma]

Since Polymake & Singular require julia >= 1.3, we can just build new binaries on Yggdrasil with the adjusted init part and close LoadFlint down. For julia < 1.3, Nemo will do the init itself without LoadFlint, but this won't effect anyone.

Of course, for this we have to wait until Polymake works with flint 2.6.0.

[benlorenz]

polymake itself does work fine with flint 2.6.0, but unfortunately we cannot easily switch to jll dependencies right now because this breaks compilation of wrappers at run-time. So we are still using a legacy build_something.jl system and rely on LoadFlint to set the memory functions. (But I changed all the dependencies to the jll based binaries)

The current master of Polymake.jl uses LoadFlint 0.2.1 (with FLINT-v0.0.2+0) binaries, but changing that to some LoadFlint 0.3 with FLINT 2.6 would be fine.

[thofma]

So, even if we move the init code to FLINT_jll, we would still need to have LoadFlint around? OK. I will prepare the PR bumping LoadFlint. Do you mind if we set the FLINT_jll version in the Project.toml as "= v2.6.0", that is, we pin the exact version of the package.?It does not seem that flint follows semver, so doing "v.2.6.0" (which is the same es [2.6.0 - 2..]) seems a bit risky.

[benlorenz]

So, even if we move the init code to FLINT_jll, we would still need to have LoadFlint around? OK.

Yes, until I can come up with a good solution for our compilation problem.

I will prepare the PR bumping LoadFlint.

Thanks

Do you mind if we set the FLINT_jll version in the Project.toml as "= v2.6.0", that is, we pin the exact version of the package.?It does not seem that flint follows semver, so doing "v.2.6.0" (which is the same es [2.6.0 - 2..]) seems a bit risky.

I am fine with fixing it to 2.6.0, probably having [2.6.0 - 2.6._] would also be fine (and reduce the number of version bumps that we need to do)?

Alternatively, any packages depending on LoadFlint could also add an extra dependency on FLINT_jll with a compat entry to restrict the version as needed. And in LoadFlint we put a very wide compat entry (or even none at all) as it does not really depend on the flint version.
I think this would make it a lot easier to keep track of the versions.

[thofma]

Alternatively, any packages depending on LoadFlint could also add an extra dependency on FLINT_jll with a compat entry to restrict the version as needed. And in LoadFlint we put a very wide compat entry (or even none at all) as it does not really depend on the flint version.
I think this would make it a lot easier to keep track of the versions.

Yes, I like that solution. I will use a very wide compat entry for FLINT_jll here. Then the upstream packages can take care of the compability themselves.

[thofma]

There is a minor issue that deps.jl must include the right build_FLINT_*.jl. This was easy when we fixed the FLINT_jll version, but now this is in flux. So we must ship all build_FLINT_*.jl files and deps.jl must include the right one ...

I will monkey patch it somehow by looking up the FLINT_jll version at build time.

Alternatively, we fix the FLINT_jll version, again.