GAPTypes are added to the main environment

[kalmarek]

build.jl should not add packages to the global environment as it does now

[fingolfin]

So first of, I agree with this in principle, and would love to get rid of it, and @sebasguts had a hard time to convince me to add it in the first place; but back then we couldn't come up with a better solution to the problem(s) this is meant to help work around, and frankly needed to make progress, so went with the ugly hack in the end. It certainly is possible that one can come up with a better solution if one has a better understanding of Julia package management; I am hoping it is!

Before I go on, let me point out that also GAP's configure script also tries to install GAPTypes.jl into the global environment, for similar reasons, and ideally that then should also be "fixed", somehow.

I'll now try to explain from the top of my head why I think we do this. However, it's been some time, and while I asked @sebasguts to please write down the reasoning somewhere for future discussions like this one, I don't think it ever happened :-/ (but if it did and I just can't find it, then hopefully Sebastian will let us know ;-).

The start of it all is that we wanted to enable precompilation of (at least parts of) GAP.jl resp of packages depending on GAP.jl. But for that one needs stable types, but for that code depending on GAP.jl should see a fixed type defined in Julia, not a type defined only at runtime from our C code (i.e., ForeignGAP.MPtr) via some low-level Julia APIs (jl_new_foreign_type -- an API we got the Julia people to add, @rbehrends wrote it). And so https://github.com/oscar-system/GAPTypes.jl was born, which defines a type GapObj, which is now the supertype of ForeignGAP.MPtr.

OK problem solved, except we have a new one: now the GAP kernel code, which defines ForeignGAP.MPtr needs to know about GAPTypes.GapObj, so that it can set it as parent for ForeignGAP.MPtr. OK, that's easy enough -- well, as long as GAPTypes.jl is loaded. Fine, no problem, because GAPTypes.jl is a dependency of GAP.jl anyway, right? Well yes, almost...

Except: remember that GAP.jl actually can be used in two completely different settings:

1. From Julia: load GAP.jl, this "launches" GAP, then loads JuliaInterface inside GAP.
2. From GAP linked against Julia: load JuliaInterface, this then loads GAP.jl into Julia.

And what I just described only was concerned with setting 1. In setting 2, we now face a chicken-and-egg problem: GAP needs to initialize on the C level before GAP.jl can possibly be loaded (because GAP.jl needs to either load GAP, or be run in an already fully loaded GAP). Hence the GAPTypes.jl "belonging" to GAP.jl is not loaded yet. So as HACK to work around this, we try to load GAPTypes.jl directly, from within the GAP C initialization code (the ability to do that was actually also a reason to add the separate package GAPTypes.jl). Here's a code snippet:

    else {
        // GAP.jl is not yet loaded, i.e., this is a standalone GAP; so try to
        // load GAPTypes.jl directly and into the main module
        parent_module = jl_main_module;
        jl_eval_string("import GAPTypes");
        if (jl_exception_occurred()) {
            Panic("could not import GAPTypes module into Julia");
        }
    }

So this literally evaluates the expression import GAPTypes... but of course that gives us a "global" GAPTypes, and only works if one is present...

At this point, you may wonder: "So, why care about this, I only want to use Julia from GAP". Well, yeah, but if you want to use any of the GAP packages that need compilation, chances are that their build systems need to invoke gap directly at some point; and doing that requires GAP to define its ForeignGAP.MPtr Julia type; though it need not load JuliaInterface, GAP.jl etc. Of course we could simply not use a supertype for ForeignGAP.MPtr in this case, but GAP has no way of knowing whether the user may want to load JuliaInterface later on or not. OK, I guess technically we could then add such a way, say via some command line flag; but I really think that in order to convince the broader GAP community to take GAP.jl and more broadly OSCAR seriously, we need to support this "load Julia from GAP" mode well...

Iit's unclear to me how to resolve this conundrum. One crazy idea I should try would be to simply declare ForeignGAP.MPtr without a supertype at first in this case; then load GAP.jl as soon as possible (i.e. after the GAP init has completed), but make it trigger a special callback into GAP immediately after it has loaded GAPTypes.jl; that callback then modifies ForeignGAP.MPtr in the background to set its supertype.... I have no idea whether that would work, not work, or call down the fury of hell on us...

There are probably some things I missed to explain, or forgot, or got wrong, but I think at least the broad issue should be clearer.

[kalmarek]

I understand, but the actual reported thing is that local installation of GAP.jl modifies the global/main environment. I'm totally fine with it adding GAPTypes to local environment.

One potential solution is to create (mutable) struct GAPObj{T} ptr::T end as a thin wrapper around MPtr or whatever else that comes from GAP. How many subtypes of GAPObj do we have? How many there are subtypes of GAPObj right now? Is GAPObj used purely for dispatch?

[fingolfin]

OK, installing into the local environment sounds like it might lead to a workable solution (at least of part of the problem). But to be honest, I've very little experience using environments; I will work on changing that; in the meantime, if you have a pointer for me what to change in build.jl to achieve that, I'll look into it. As mentioned, there will be work needed on the GAP side, too, but I can take care of that: I'll have to ensure GAP knows which Julia environment it is installed in, so that it can also find and reference the right GAPTypes.jl

Hm, there is another weird thing in build.jl: we create a symlink to the GAP directory in ~/.julia/. I vaguely remember @sebasguts telling me why he thought this was needed or useful, but I can't remember it. It doesn't make sense to me anyway, so I think I'll drop it and wait if anybody complaints subsequently about a regression; then we can look into fixing that properly (and if there are no complaints: even better)

[fingolfin]

Ah, I think it may have been in there to make it easy to locate the GAP executable when running on Travis. But there are of course better ways to do that.

[kalmarek]

well, the other thing is to have exactly one version of GAP tree for julia ;)
I'm looking at those lines in 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")'`)
    ENV["JULIA_LOAD_PATH"] = julia_load_path_save

and I'm asking myself why isn't it just Pkg.add("GAPTypes") ? I'm not sure I understand the dance with load_paths though...

[fingolfin]

"the other thing is to have exactly one version of GAP tree for julia" -> but that's not what that did or was about. It just was a symlink which pointed to the last gap clone made by any version of GAP.jl

There are issue with sharing a single GAP tree: at the very least, we need to compile GAP once for each for version of Julia (so for 1.1, 1.2, 1.3). GAP actually allows multiple out-of-tree build dirs from a single source, so that could even be done with a single GAP source dir (but emphasis on could -- "somebody" would have to code the required logic. Shouldn't be too hard, but requires non-zero effort).

Next, as I said, in the future the plan is not to clone a git repo, but rather use a fixed version of GAP (I'd assume that's what e.g. Polymake.jl also does, doesn't it?). And then different versions of GAP.jl may require different versions of GAP, in addition to different versions of Julia requiring GAP to be recompiled. So one has to be really careful.

[fingolfin]

Regarding that JULIA_LOAD_PATH: Doesn't the comment above that snippet you pasted explain it? That code is explicitly meant to ensure GAPTypes is installed in global package environment. Otherwise, unless the other manually installed GAPTypes.jl globally, one cannot start gap, as it'll fail to load GAPTypes.jl, as explained in my length comment above

[kalmarek]

so I guess the problem is gap always looking for GAPTypes in the global env?

In Polymake only the thin CxxWrapper is built against libjulia. Other than this we use fixed build by BinaryBuilder, which handles those automatically (or I think so).

Lets meet on slack/skype and discuss why GAPObj needs to be an abstract type, shall we?

[fingolfin]

In the past, there was no GapObj and no GAPTypes.jl, and none of these problems existed; I could easily revert things back to that. But that also meant that everything was using ForeignGAP.Mptr directly, which, as I understand it, made precompilation of the Julia code in GAP.jl impossible, and also in any other project that might use GAP.jl.

I am happy to discuss other ways to solve this, and be super glad if we could get rid of GAPTypes.jl.

[fingolfin]

See also PR #322, PR #327 and https://discourse.julialang.org/t/swizzling-the-super-type-of-a-foreign-julia-type-or-how-evil-must-i-be/33733