WIP: Pkg Artifacts with legacy mode #4
Conversation
|
We're only setting the gmp functions in the unlikely event of having to load gmp ourselves. If gmp is already there, the memory functions are not touched. (This assumes that Julia is doing it) |
|
There is now a Flint_jll (see JuliaPackaging/Yggdrasil#661) thanks to @benlorenz , yay! |
|
So, how important is support for Julia 1.0-1.2 to us? We can't support them in Oscar.jl anyway, because Gap.jl needs Julia 1.1 at the least (and I've gone full in with 1.3 otherwise since we declared elsewhere that'll be the minimum for Oscar overall). I mean, I don't mind adding in backwards compatibility shims if that helps (and somebody else does it ;-) ), I just wonder if it's really worth the effort? |
|
On Fri, Apr 03, 2020 at 05:53:04PM -0700, Max Horn wrote:
So, how important is support for Julia 1.0-1.2 to us? We can't support them in Oscar.jl anyway, because Gap.jl needs Julia 1.1 at the least (and I've gone full in with 1.3 otherwise since we declared elsewhere that'll be the minimum for Oscar overall).
I mean, I don't mind adding in backwards compatibility shims if that
helps (and somebody else does it ;-) ), I just wonder if it's really
worth the effort?
Bill will kill anyone if Nemo is no-longer running in 1.0 unneccessary.
Oscar is 1.3 and above, but as long as Nemo is independently useful,
this will be difficult to argue....
What are the advantages to us of using the flint_jll at this point over
the binary builder?
… --
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#4 (comment)
|
benlorenz
force-pushed
the
artifactswithlegacy
branch
from
df22334
to
1bf4ac5
Compare
|
The main reason I am wondering is that using |
|
At the moment it seems that with And just to be clear: the jll packages are what BinaryBuilder now produces, and BinaryProvider (which is used for he build_*.jl files) will be deprecated at some point. |
benlorenz
force-pushed
the
artifactswithlegacy
branch
6 times, most recently
from
9431d87
to
462a3fc
Compare
|
On Sat, Apr 04, 2020 at 02:53:17AM -0700, Max Horn wrote:
The main reason I am wondering is that using `GMP_jll` and `Zlib_jll`
is considerably more convenient for me than using their "old-style"
binary builder counterparts; yet I am afraid that mixing the two will
again trigger the warning in LoadFlint about multiple GMP being
loaded... For this reason, I also think we should disable that error.
It's OK for Flint, but for GMP, it just is quite likely that this will
be triggered eventually by other packages outside our control.
The problem is that, once we have multiple gmp, the system is from our
point of view non-deterministic and will crash - unless all gmps are
- binary compatible
- using compatible allocaters
The crashing code is always an idirect call to gmp. flint -> gmp
While I can control the flint that I get (setting absolute path names) I
cannot control which gmp flint is using. As far as I can say not even
that subsequent calls to different flint functions will call the same
gmp all the time. This is likely to be the case, and might be able to
be enforced by some flags to dlopen, but I don't know.
At this point, having multiple gmps means that I can no longer guarantee
that pairs of alloc/free will be resolved in the same library using the
same underlying memory manager. Possibly there are other, better means
to enforce this, but I don't know how.
A slightly different solution we might try - also awkward - is to patch
flint to use the gmp allocators:
flint_set_memory_functions_to_gmp()
and hope that within flint all calls are resolved to the same gmp...
Even worse we have more complicated c-graphs
libantic -> gmp
-> flint -> gmp
-> mpfr -> gmp
arb -> gmp
mpfr -> gmp
and those gmp should line up...
… --
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#4 (comment)
|
|
On Sat, Apr 04, 2020 at 02:53:17AM -0700, Max Horn wrote:
The main reason I am wondering is that using `GMP_jll` and `Zlib_jll`
is considerably more convenient for me than using their "old-style"
binary builder counterparts; yet I am afraid that mixing the two will
again trigger the warning in LoadFlint about multiple GMP being
loaded... For this reason, I also think we should disable that error.
It's OK for Flint, but for GMP, it just is quite likely that this will
be triggered eventually by other packages outside our control.
I don't mind giving up in 1.0, for Oscar we're already past that
decision. But as long as Nemo is independent and Julia does not commit
to a new LTS we're screwed
… --
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#4 (comment)
|
|
Sure, I am fine with using the old-style BinaryBuilder / BinaryProvider stuff here, resp. keep compatibility with Julia 1.0, if that's necessary for Nemo / to make Bill happy ;-). As to multiple GMP versions being loaded: My point is that we can't prevent that anyway: if In the meantime, the check causes an annoyance to people who would otherwise be happily using our stuff. So I would suggest that we downgrade it to a (big and fat) warning in the meantime... Independent of that, I'll work on fixing GAP.jl to not load a new GMP (it's going to be a bit slow today without the babysitter, but I'll manage ;-) ) |
|
On Mon, Apr 06, 2020 at 01:36:20AM -0700, Max Horn wrote:
Sure, I am fine with using the old-style BinaryBuilder /
BinaryProvider stuff here, resp. keep compatibility with Julia 1.0, if
that's necessary for Nemo / to make Bill happy ;-).
As to multiple GMP versions being loaded: My point is that we can't
prevent that anyway: if `LoadFlint` is loaded first then it doesn't
see if a dozen copies of libgmp get loaded afterwards. There are Julia
package using `GMP_jll` already anyway, and people may load them after
Oscar...
LoadFlint will, currently, neer actually load gmp as it is already
present...
I see your point and maybe that is s.th. we need to into an issue for
Julia: having multiple copies of any library and non-julia dependencies
between packages is dangerous.
In the meantime, the check causes an annoyance to people who would
otherwise be happily using our stuff. So I would suggest that we
downgrade it to a (big and fat) warning in the meantime... Independent
Sure - however, it will produce random crashes...
As long as it is clear that we cannot help others that is fine.
of that, I'll work on fixing GAP.jl to not load a new GMP (it's going
to be a bit slow today without the babysitter, but I'll manage ;-) )
All the best!
…
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#4 (comment)
|
|
I've just pushed and replaced the error by @warn
…On Mon, Apr 06, 2020 at 01:36:20AM -0700, Max Horn wrote:
Sure, I am fine with using the old-style BinaryBuilder / BinaryProvider stuff here, resp. keep compatibility with Julia 1.0, if that's necessary for Nemo / to make Bill happy ;-).
As to multiple GMP versions being loaded: My point is that we can't prevent that anyway: if `LoadFlint` is loaded first then it doesn't see if a dozen copies of libgmp get loaded afterwards. There are Julia package using `GMP_jll` already anyway, and people may load them after Oscar...
In the meantime, the check causes an annoyance to people who would otherwise be happily using our stuff. So I would suggest that we downgrade it to a (big and fat) warning in the meantime... Independent of that, I'll work on fixing GAP.jl to not load a new GMP (it's going to be a bit slow today without the babysitter, but I'll manage ;-) )
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#4 (comment)
|
|
Overall, I like this PR: It allows us to use @benlorenz What needs to be done with this PR to make it ready, from your perspective? The one thing I found by re-reading the discussion is that I am really interested in getting this in, as it would allow GAP.jl and Polymake.jl to switch to Jll packages, which will simplify all kinds of things down the road, I think. |
|
I think the second gmp from GMP_jll should not cause issues. At some point this might be resolved by julia itself using pkg artifacts for dependencies. I need to test the latest version with older julia versions again and check what downstream software needs in form of exported variables (libgmp path, libflint path, ..., maybe handles instead?) Also I wanted to provide a function for running the old build script manually which at the end shows what is needed to use the custom installation. I got distracted by trying to update polymake for the artifacts. |
|
Note that we use Travis to test this package against Julia 1.0, 1.3, 1.4 and nightly, on Linux and macOS. we can also add a test using Xcode 11.4; and we only dropped 1.1 and 1.2 to reduce the time the tests run, but we can of course add them back (this repo shouldn't get updates too often anyway). Anyway, no pressure, we all are busy with many other things, too :-). I just got reminded of this because of oscar-system/GAP.jl#415 :-). Thank you for working on this. |
add legacy mode for old julia version via build.jl remove source build FLINT_PATH for custom flint installations with julia 1.2 or older dont touch gmp memory functions
benlorenz
force-pushed
the
artifactswithlegacy
branch
2 times, most recently
from
442df11
to
214aeb9
Compare
benlorenz
force-pushed
the
artifactswithlegacy
branch
2 times, most recently
from
444e9ef
to
a871563
Compare
benlorenz
force-pushed
the
artifactswithlegacy
branch
from
a871563
to
e7dbb47
Compare
There was a problem hiding this comment.
@benlorenz the error on Travis you are seeing is about fmpz_poly_is_x -- this is a genuine error, as that function does not exist anymore in Flint 2.6.0, it was renamed. For compatibility, there is only this left:
fmpq_poly.h:260:#define fmpq_poly_is_x fmpq_poly_is_gen
fmpz_mod_poly.h:219:#define fmpz_mod_poly_is_x fmpz_mod_poly_is_gen
fmpz_poly.h:282:#define fmpz_poly_is_x fmpz_poly_is_gen
But of course that means there is no symbol we can ccall anymore. Ways to resolve this:
- We need to adjust Nemo.jl in lock step with the switch to Flint 2.6.0. If we also switch to JLLs at the same time, this might require a bit of a leap of faith.
- Or we first merge PR Just to test the new flint alpha #5 by @thofma, then update Nemo.jl, then update and merge this PR
- Or @wbhart releases Flint 2.6.1 with actual binary stubs for
fmpz_poly_is_xand friends, then we can delay updating Nemo.jl
There are probably other approaches, we just need to pick one and execute on it.
deps/build.jl
Outdated
| end | ||
| println("DONE") | ||
| # GMP and MPFR might not be needed on unix as julia should have those loaded already | ||
| # but on windows flint will not load without them so we put leave them here for simplicity |
There was a problem hiding this comment.
| # but on windows flint will not load without them so we put leave them here for simplicity | |
| # but on windows flint will not load without them so we leave them here for simplicity |
|
(#5 was only there to test the alpha release.) Does this still allow to build flint locally? I mean, using the package manager and everything linked properly. At the moment I can do Also, Nemo will require corresponding |
|
The change has to be made in Nemo at some point. The intention was that it would happen immediately, which is why I didn't retain the symbol in the library. |
|
In the meeting I was referring to other builds with weird errors: The first one seems to appear only with a fresh deps directory and also goes away when using an absolute path instead of |
|
Regarding source build: There exist two mechanisms two provide a custom flint:
Maybe Nemo can have a script or function that does a source build in a custom directory, independent of the julia build command. At the end of the build this could print the required Overrides.toml entries / environment variables. Something like the following made up output: For julia 1.0 we could print |
Maybe Nemo could do |
This partially reverts commit 56e89f4, to switch to the older flint for testing. Also add compat to use old flint_jll.
benlorenz
force-pushed
the
artifactswithlegacy
branch
from
890a249
to
dcd6d55
Compare
benlorenz
force-pushed
the
artifactswithlegacy
branch
from
b2e7cf3
to
cbc18e3
Compare
|
I have switched this back to the FLINT_jll from before 2.6 which was tagged as 0.0.1 in Yggdrasil. This is compatible with the current Nemo release and the also the latest polymake release. We can do the bump to 2.6 in a later pull request together with a version bump to 0.2.0 when Nemo is fixed to work with 2.6 (or can work with both flint versions). |
|
Thanks Benjamin. How about merging your PR and tagging it as 0.2.0? Then we can play with it on the Nemo side (without automatically using it) and see if everything works fine. I know that the reverse dependency tests should catch any such error, but I want to make sure :) |
|
Note that both Polymake.jl and Nemo.jl currently only use LoadFlint 0.1, so they won't see or use 0.2.0... So what is the plan forward here? To update Nemo.jl and Polymake.jl to use LoadFlint.jl 0.2.0? Then release LoadFlint 0.3.0 which uses FLINT_jll 2.6.0 (which will of course not be able to pass the "reverse" tests), then afterwards update Nemo.jl and Polymake.jl once more. (BTW, all these issue would be easy to resolve if we were using a monorepo ... :-) |
|
Yes. We should actually use something newer than 2.6.0. There has been some important bugfixes since then. Nemo is basically good to go with Artifacts and legacy mode, see Nemocas/Nemo.jl#845. |
This is still work in progress
Switch to Pkg Artifacts for dependencies with a legacy mode for Julia 1.0 - 1.2.
Legacy mode is using a generated build_flint.jl which uses the tarballs from the artifacts.
To use the artifacts one has to add the OscarRegistry.
I have removed the source build as in #3, for now I have only added a FLINT_PATH environment variable to specify custom
libflint.$dlextfor julia 1.2 or older.For julia 1.3 one can add the following to
~/.julia/artifacts/Overrides.toml:where
/home/me/flint-installshould contain alibdirectory containinglibflint.soI removed the setting of gmp memory functions. I dont think we should touch this, also julia is doing this by itself already and we should be using the julia-libgmp.
I added a small ccall test using
flint_mallocandflint_free.Issues / Todo: