Make GAP.jl usable as a Julia module with precompilation #249

sebasguts · 2019-06-12T21:30:39Z

Move *.jl files from JuliaInterface to module itself
Add GAPTypes.jl package as dependency
Replaced MPtr by GapObj
Removed unnecessary libgap.jl
Moved MPtr call definition into init, eval into Main to not eval into a closed module

Resolves #265

codecov · 2019-06-12T21:35:20Z

Codecov Report

Merging #249 into master will increase coverage by 5.76%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
+ Coverage   64.31%   70.07%   +5.76%     
==========================================
  Files          49       50       +1     
  Lines        3276     3262      -14     
==========================================
+ Hits         2107     2286     +179     
+ Misses       1169      976     -193

Impacted Files	Coverage Δ
test/conversion.jl	`100% <ø> (+100%)`	⬆️
src/macros.jl	`100% <ø> (ø)`
src/ccalls.jl	`98.38% <ø> (ø)`
deps/build.jl	`0% <ø> (ø)`	⬆️
pkg/GAPJulia/JuliaInterface/read.g	`100% <ø> (ø)`	⬆️
src/julia_to_gap.jl	`95.38% <0%> (ø)`
src/gap_to_julia.jl	`92.98% <100%> (ø)`
src/GAP.jl	`91.89% <100%> (+18.36%)`	⬆️
src/gap2.jl	`46% <92.85%> (ø)`
... and 10 more

pkg/GAPJulia/JuliaInterface/read.g

src/GAP.jl

src/ccalls.jl

test/basics.jl

src/julia_to_gap.jl

sebasguts · 2019-06-17T05:43:45Z

Sorry, I was afk for the weekend. Will continue now.

fingolfin · 2019-06-19T07:39:45Z

deps/build.jl

+println("DEPOT = ", DEPOT_PATH)
+
+println("................ENV.............")
+println(ENV)


Is this for debugging or is it meant to say here?

fingolfin

There are some kinks (see comments) but if you want, we can just merge this now and resolve those later (perhaps best to record them as issues, though)

fingolfin · 2019-06-19T07:41:15Z

deps/build.jl

+    ## Hack to make GAPTypes available in the global package environment:
+    ## For a moment, we set the Julia load path to the actual depot, i.e.,
+    ## the env GAP.jl is installed to, to install GAPTypes.jl globally
+    julia_load_path_save = ENV["JULIA_LOAD_PATH"]


Is ENV["JULIA_LOAD_PATH"] guaranteed (by documentation / specification) to be set at this point? If it is not, then this line could throw an exception.

Since this build script is (or better should, nobody is keeping you from shooting yourself in the foot) only executed by the package manager, yes, I am sure this is always set.

fingolfin · 2019-06-19T07:43:47Z

deps/build.jl

+    julia_load_path_save = ENV["JULIA_LOAD_PATH"]
+    pop!( ENV, "JULIA_LOAD_PATH" )
+    julia_executable = joinpath(julia_binary, "julia")
+    run(`$(julia_executable) -e 'import Pkg; Pkg.add("GAPTypes")'`)


Wow, that is a gross hack. I am also not quite sure why it is needed? Surely, GAP's configure also takes care of it? OK, perhaps the problem is that it inherits the JULIA_LOAD_PATH? But then at the very least, the comment above should explain so.

But I also wonder (very naively, so there is probably an easy answer why this won't work): couldn't we make the code in GAP more flexible, and ask it to first check if GAP.GAPTypes is available, then use that; if not, try to use GAPTypes directly?

Yeah, I am painfully aware that this is gross, and should not be done this way. On the other hand, the Julia package manager sets this environment variable, too, which is also not very nice of it.

Actually, patching GAP this way would be possible, although not for the 4.10.2 release I guess. But then we can at least remove that hack afterwards.

I will leave one more comment. The problem is that for the configure script, this somehow failed to give me the right responses when we asked for flags. I have no real idea why yet. This solution is merely the result of a lot of try-and-error.

src/GAP.jl

* Move *.jl files from JuliaInterface to module itself * Add GAPTypes.jl package as dependency * Replaced MPtr by GapObj * Removed unnecessary libgap.jl * Moved MPtr call definition into init, eval into Main to not eval into a closed module

mohamed-barakat · 2019-06-20T08:48:46Z

I get the following error:

julia> using GAP
[ Info: Recompiling stale cache file ~/.julia/compiled/v1.1/GAP/YqHod.ji for GAP [c863536a-3901-11e9-33e7-d5cd0df7b904]
Adding path /Users/mo/.julia/dev/GAP/gap/.libs to DL_LOAD_PATH
 ┌───────┐   GAP 4.10dev-2052-gf30c1e4 of today
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-apple-darwin18.2.0-julia64-kv6
 Configuration:  gmp 6.1.2, Julia GC, Julia 1.1.0, readline
 Loading the library and packages ...
 Packages:   GAPDoc 1.6.2, PrimGrp 3.3.2, SmallGrp 1.3, TransGrp 2.0.4
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
Error, MethodError: Cannot `convert` an object of type Main.ForeignGAP.MPtr to an object of type GAPTypes.GapObj
Closest candidates are:
  convert(::Type{T}, !Matched::T) where T at essentials.jl:154
ERROR: InitError: Error thrown by GAP
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] error_handler() at /Users/mo/.julia/dev/GAP/src/GAP.jl:47
 [3] initialize(::Array{String,1}, ::Array{String,1}, ::Ptr{Nothing}) at /Users/mo/.julia/dev/GAP/src/GAP.jl:67
 [4] (::getfield(GAP, Symbol("##3#4")))(::String, ::Ptr{Nothing}) at /Users/mo/.julia/dev/GAP/src/GAP.jl:97
 [5] __init__() at /Users/mo/.julia/dev/GAP/src/GAP.jl:115
 [6] _include_from_serialized(::String, ::Array{Any,1}) at ./loading.jl:633
 [7] _require_from_serialized(::String) at ./loading.jl:684
 [8] _require(::Base.PkgId) at ./loading.jl:967
 [9] require(::Base.PkgId) at ./loading.jl:858
 [10] require(::Module, ::Symbol) at ./loading.jl:853
during initialization of module GAP

sebasguts · 2019-06-20T08:54:20Z

Please do not put bug reports in closed PR's, but create an issue instead. Anyway, will investigate.

mohamed-barakat · 2019-06-20T08:55:45Z

I was about to do that. This is just for reference.

sebasguts · 2019-06-20T08:58:09Z

I have to admit, I cannot reproduce this. Did you update everything? Including GAP itself?

mohamed-barakat · 2019-06-20T09:25:38Z

As you suggest, I deleted GAP.jl/gap for

pkg> build GAP

to work. Now using GAP works. Thanks.

sebasguts requested a review from fingolfin June 12, 2019 21:30

sebasguts force-pushed the enable_precompilation branch from 20a9a35 to 08ddcdd Compare June 13, 2019 08:47

fingolfin reviewed Jun 14, 2019

View reviewed changes

sebasguts force-pushed the enable_precompilation branch 2 times, most recently from c8e491f to d88b5bd Compare June 14, 2019 14:14

fingolfin mentioned this pull request Jun 16, 2019

Misc changes, preparation for PR #249 #255

Merged

fingolfin force-pushed the enable_precompilation branch from d88b5bd to 8c067bc Compare June 16, 2019 21:22

fingolfin mentioned this pull request Jun 16, 2019

JuliaInterface: move macros to macros.jl #257

Merged

fingolfin force-pushed the enable_precompilation branch 2 times, most recently from 7e4cfd1 to fb95746 Compare June 17, 2019 15:33

sebasguts force-pushed the enable_precompilation branch 4 times, most recently from 8293594 to 0b3e664 Compare June 18, 2019 15:22

This was referenced Jun 19, 2019

Added optional parameter to LoadPackageAndExposeGlobals #264

Merged

Make GAPTypes.jl a dependency in the Project.toml #265

Closed

fingolfin reviewed Jun 19, 2019

View reviewed changes

fingolfin approved these changes Jun 19, 2019

View reviewed changes

fingolfin mentioned this pull request Jun 19, 2019

show GAP help in Julia with 'GAP.show_GAP_help' and via ? #251

Merged

sebasguts force-pushed the enable_precompilation branch from 85ddeb5 to 5a1572f Compare June 19, 2019 09:45

sebasguts merged commit 5e104af into oscar-system:master Jun 19, 2019

sebasguts deleted the enable_precompilation branch June 19, 2019 14:27

sebasguts restored the enable_precompilation branch June 20, 2019 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make GAP.jl usable as a Julia module with precompilation #249

Make GAP.jl usable as a Julia module with precompilation #249

sebasguts commented Jun 12, 2019 •

edited by fingolfin

Loading

codecov bot commented Jun 12, 2019 •

edited

Loading

sebasguts commented Jun 17, 2019

fingolfin Jun 19, 2019

fingolfin left a comment

fingolfin Jun 19, 2019

sebasguts Jun 19, 2019

fingolfin Jun 19, 2019

sebasguts Jun 19, 2019

sebasguts Jun 19, 2019

mohamed-barakat commented Jun 20, 2019

sebasguts commented Jun 20, 2019

mohamed-barakat commented Jun 20, 2019

sebasguts commented Jun 20, 2019

mohamed-barakat commented Jun 20, 2019

Make GAP.jl usable as a Julia module with precompilation #249

Make GAP.jl usable as a Julia module with precompilation #249

Conversation

sebasguts commented Jun 12, 2019 • edited by fingolfin Loading

codecov bot commented Jun 12, 2019 • edited Loading

Codecov Report

sebasguts commented Jun 17, 2019

fingolfin Jun 19, 2019

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Jun 19, 2019

Choose a reason for hiding this comment

sebasguts Jun 19, 2019

Choose a reason for hiding this comment

fingolfin Jun 19, 2019

Choose a reason for hiding this comment

sebasguts Jun 19, 2019

Choose a reason for hiding this comment

sebasguts Jun 19, 2019

Choose a reason for hiding this comment

mohamed-barakat commented Jun 20, 2019

sebasguts commented Jun 20, 2019

mohamed-barakat commented Jun 20, 2019

sebasguts commented Jun 20, 2019

mohamed-barakat commented Jun 20, 2019

sebasguts commented Jun 12, 2019 •

edited by fingolfin

Loading

codecov bot commented Jun 12, 2019 •

edited

Loading