Add GAP kernel API (aka libgap) #2702

markuspf · 2018-08-15T09:36:22Z

This pull request is the next logical step in providing GAP as a dynamically loadable library (known by some as LibGAP).

I squashed a whole history of commits developed by @sebasguts, @fingolfin, and me into a single commit because it's not that big anymore. If there's certain bits you'd like committed separately, speak now or forever hold your peace.

It would be good if @sebasguts (for @oscar-system) and @nthiery (for SageMath) as the main "clients" could have a look to see whether this is going towards what they are hoping for.

For the future there needs to be more documentation, and more thorough testing, and a more comprehensive API.

For a future pull-request I am thinking about making a command-line-switch --disable-shell which disables the repl (basically removing the need for IsLIBGAP, at least right now, and I can't think of another usecase for this particular constant.

codecov · 2018-08-15T10:15:22Z

Codecov Report

Merging #2702 into master will increase coverage by <.01%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #2702      +/-   ##
==========================================
+ Coverage   75.79%   75.79%   +<.01%     
==========================================
  Files         479      480       +1     
  Lines      241471   241483      +12     
==========================================
+ Hits       183022   183044      +22     
+ Misses      58449    58439      -10

Impacted Files	Coverage Δ
lib/streams.gi	`40.59% <ø> (+0.25%)`	⬆️
src/libgap-api.c	`93.33% <93.33%> (ø)`
src/hpc/threadapi.c	`43.48% <0%> (-0.11%)`	⬇️
src/system.c	`70.18% <0%> (+0.31%)`	⬆️
src/gasman.c	`88.06% <0%> (+0.33%)`	⬆️
hpcgap/lib/hpc/stdtasks.g	`64.96% <0%> (+1.53%)`	⬆️

fingolfin · 2018-08-15T10:15:53Z

src/libgap-api.c

+/*** Integer *************************************************************/
+/*************************************************************************/
+
+Obj GAP_IntObj_Int( Int value )


clang-format all new files?

fingolfin · 2018-08-15T10:21:16Z

src/libgap-api.c

+
+void GAP_SetElmPList( Obj list, Int pos, Obj elem ){
+    SET_ELM_PLIST( list, pos, elem );
+}


Honest open question: I wonder whether there is an actual reason (e.g. extreme performance needs?) for why this "API" exposes potentially dangerous low-level functions like SET_ELM_PLIST, or even AssPlist -- wouldn't it be more useful and safer to expose ASS_LIST, ELM0_LIST, LEN_LIST, etc.?

I just had a look at the sage code, and also at https://hackmd.io/emNi76svSWCh1fBeLKqPdA# (which @nthiery and me created together). Some notes:

LEN_PLIST is used once in SageMath -- incorrectly, though, as it is also called on ranges, blists, strings -> should really use LEN_LIST instead

as far as I can tell, no other GAP function with the string plist (upper or lower case) is used by SageMath, although a few more are imported (but then not used)

Perhaps @sebasguts "needs" some of them? Is there perhaps a code repository somewhere to look at?

Finding what the correct API is is, in my opinon, the name of the game for the immediate future of "LibGAP".

Fort that its important to know use-cases. I wasnt aware of https://hackmd.io/emNi76svSWCh1fBeLKqPdA#, and as a non-user of this library of course completely oblivious to what people want to do.

fingolfin · 2018-08-15T10:25:52Z

src/libgap-api.c

+
+void GAP_Finalize(void)
+{
+    FinishBags();


What's the intention behind GAP_Finalize? It makes GAP impossible to use afterwards, but is far from deinitializing it completely; e.g. it does not close file descriptors, nor does it call finalizers ("free functions") for any bags, hence it won't free external data allocated by external modules like NormalizInterface. It doesn't terminate child processes, etc.

So, all in all, this function is currently of rather limited use. Hence I would suggest to either remove it; or else at least add a comment explaining its incomplete state.

Oh, and it won't work with Julia GC or Boehm, but that holds for other things in this PR, too (though most other stuff should be easy to adjust).

The intention is to finish the GAP "session", and be able to GAP_Initialize again, having a fresh GAP session.

Of course better yet, in future, if we have all state encapsulated, we'd be able to run multiple GAPs in one process.

That said, if noone sees a use for this, we can just leave it out for now.

I do not think we would need this in OSCAR for now, so I think it could be removed.

I'd quite like to be able to finalize GAP for my racket bindings, otherwise using libgap in drracket is a bit of a pain, as it crashes on reload. I might be able to address this issue by unloading the .so file, but have to check whether that's possible.

Either way, its for me to implement if noone else wants it.

As I wrote, I am neutral on whether to keep it or drop it; but if kept, I still recommend to add a warning comment, like // TODO: this function does not yet free all state held by GAP, e.g. it does not close any open file handles or kill child processes. But in the end ... shrug

fingolfin · 2018-08-15T10:27:22Z

src/gasman.h

+ * If not 0 this function will be called in
+ * CollectBags to allow users of libgap to mark bags
+ */
+extern TNumExtraMarkFuncBags ExtraMarkFuncBags;


I'd strongly prefer if we did not expose a global variable like this, but instead provided a SetExtraMarkFuncBags(TNumExtraMarkFuncBags func) setter function.

fingolfin · 2018-08-15T10:29:35Z

src/libgap-api.c

+#include "plist.h"
+#include "stringobj.h"
+
+Obj FuncREAD_ALL_COMMANDS(Obj self, Obj instream, Obj echo, Obj capture, Obj resultCallback);


This must go into a header file which is included in both this file here, and the file which declares FuncREAD_ALL_COMMANDS.

fingolfin · 2018-08-15T11:04:55Z

tst/testlibgap/0001.c

+    printf("Input:\n%s", cmd);
+    res = GAP_EvalString(cmd);
+    rc = GAP_LenPList(res);
+    for(i=1;i<=rc;i++) {


clang-format?

fingolfin · 2018-08-15T11:05:57Z

tst/testlibgap/0001.c

+        ires = GAP_ElmPList(res, i);
+        if( GAP_ElmPList(ires, 1) == True )
+            printf("Output:\n%s\n", CSTR_STRING(GAP_ElmPList(ires, 5)));
+    }


Would it perhaps be helpful to print the output in the same format as a .tst file?
(Fine by me to leave it as is, just wondering)

fingolfin · 2018-08-15T11:20:42Z

Makefile.rules

@@ -995,6 +1002,14 @@ testbugfix: all
          ReadGapRoot( "tst/testbugfix.g" );' | $(TESTGAP) | \
            tee `date -u +dev/log/testbugfix2_%Y-%m-%d-%H-%M` )

+obj/libgap-test-0001.lo: $(top_srcdir)/tst/testlibgap/0001.c
+	$(QUIET_CC)$(COMPILE) $(DEPFLAGS) $(GAP_CFLAGS) $(CC_EXTRA_FLAGS) $(GAP_CPPFLAGS) -c $< -o $@


What is CC_EXTRA_FLAGS? Is this an accidental left-over, or meant to introduce C flags just for testlibgap?

Other than that, with PR #2704 this build rule could go, and the general *.c build rule would take of compiling this C file

This is, I believe, an accidental leftover from the history of trying to get libgap into master

With PR #2704 merged, I think this rule can just be removed (after a rebase, I mean)

Makefile.rules

+obj/libgap-test-0001.lo: $(top_srcdir)/tst/testlibgap/0001.c
+	$(QUIET_CC)$(COMPILE) $(DEPFLAGS) $(GAP_CFLAGS) $(CC_EXTRA_FLAGS) $(GAP_CPPFLAGS) -c $< -o $@
+
+testlibgap: libgap.la obj/libgap-test-0001.lo 


fingolfin · 2018-08-15T11:35:09Z

Makefile.rules

+	$(QUIET_CC)$(COMPILE) $(DEPFLAGS) $(GAP_CFLAGS) $(CC_EXTRA_FLAGS) $(GAP_CPPFLAGS) -c $< -o $@
+
+testlibgap: libgap.la obj/libgap-test-0001.lo 
+	$(QUIET_LINK)$(LINK) obj/libgap-test-0001.lo libgap.la -o test-libgap


I would recommend to always use $<, $@ etc. instead of duplicating the names of inputs and outputs, for the usual reasons why one should avoid code duplication. So here that could become:

testlibgap: libgap.la obj/libgap-test-0001.lo $(QUIET_LINK)$(LINK) $+ -o test-libgap ...

Additional thought: all other files related to this test are named 0001.FOO, would you mind if the executable then was named 0001 (or 0001.exe or 0001.prog or whatever)? That would then suggest a clear why how to generalize this to more tests (i.e. just provide a list of stem names for tests; then for each such name "FOO", compile FOO.c into FOO (or FOO.prog or whatever), run it with input file FOO.in (if present), send output to FOO.out and diff it against FOO.expect.

And most importantly, it would enable us to resolve issue #1917, i.e., start providing kernel based tests for some features which are otherwise difficult to test (such as ObjInt_Int and its various siblings).

I can make these changes later, of course, simply want to hear what others think of the general idea.

markuspf · 2018-08-16T10:26:21Z

Should we get rid of the "--enable-libgap" configure switch? It currently (as far as I can tell) serves no useful purpose other than to confuse people (me); a dynamically linkable object could be created either way; we could just link the libgap-api.o to the dynamic object (but not the executable) for now.

markuspf · 2018-08-16T10:34:35Z

To add some more detail: the GAP_Initialize function could set the constant IsLIBGAP, or have a parameter that starts a shell.

(I am not sure about the usefulness of the IsLIBGAP constant right now as I still can't see a usecase; GAP code should not behave differently just because its run "as a library", even less so if we start implementing alternative repls)

fingolfin · 2018-08-16T11:20:48Z

I'd be fine with removing the --enable-libgap switch. As to IsLIBGAP: I am neutral about this -- perhaps @sebasguts has an opinion, though?

src/libgap-api.c

+{
+    InitializeGap(&argc, argv, env);
+    SetExtraMarkFuncBags(markBagsCallback);
+    STATE(JumpToCatchCallback) = errorCallback;


markuspf · 2018-08-17T07:23:58Z

Only marginally related: Where the heck does the spurious ^M come from? GAP seems to be printing it under some circumstances (not on travis, but in my terminal).

fingolfin · 2018-08-17T08:51:41Z

src/libgap-api.c

+
+void GAP_SetElmPList(Obj list, Int pos, Obj elem)
+{
+    SET_ELM_PLIST(list, pos, elem);


I wrote the following before, but due to updates to this PR, it is now hidden; but I still would like a reply by @sebasguts on this:

Honest open question: I wonder whether there is an actual reason (e.g. extreme performance needs?) for why this "API" exposes potentially dangerous low-level functions like SET_ELM_PLIST, or even AssPlist -- wouldn't it be more useful and safer to expose ASS_LIST, ELM0_LIST, LEN_LIST, etc.?

I just had a look at the sage code, and also at https://hackmd.io/emNi76svSWCh1fBeLKqPdA# (which @nthiery and me created together). Some notes:

LEN_PLIST is used once in SageMath -- incorrectly, though, as it is also called on ranges, blists, strings -> should really use LEN_LIST instead

as far as I can tell, no other GAP function with the string plist (upper or lower case) is used by SageMath, although a few more are imported (but then not used)

Perhaps @sebasguts "needs" some of them? Is there perhaps a code repository somewhere to look at?

Also, @markuspf replied:

Finding what the correct API is is, in my opinon, the name of the game for the immediate future of "LibGAP".

I have a clear opinion on this: such an API should overall aim to provide high-level APIs at first; and only add lower-level APIs if there is a strong justification that make the higher level APIs unsuitable (and even then, I'd first investigate making the higher level APIs better). In this case, I think it's a mistake to export all those plist accessors, when we already have generic list accessors. The only reason to use the plist ones is raw performance, but a sizeable portion of that is sacrificed by these non-inlined wrappers anyway. There may be other reasons to export plist-only APIs, but these then should be conscious decisions (and ideally explained in comments).

That said, if you guys really, really want to expose these plist APIs (even though e.g. GAP_SetElmPList is impossible to use correctly via the provided API GAP_SetElmPList, since CHANGED_BAG is not exposed), fine, go ahead -- but I think it runs totally against the idea of creating a long-term stable API, as naturally lower level interfaces are much more likely to change than higher level ones.

One more note regarding the symbols used by SageMath: a lot of the low level ones come from the function gap_eval. My suggestion here would be to try and replace most of it by using READ_ALL_COMMANDS, or a similar new function. To get somewhere quickly, I'd not worry about making the ideal generic function we all want in the long run, but just something that does precisely what SageMath needs; we then may have to touch this again later down the road, but I think that'll be much easier once we on the GAP side have clear insights on what you need. Perhaps @nthiery has thoughts on this, too?

This is precisely what GAP_EvalString does (or, well, is supposed to do!)

@fingolfin said

Also, @markuspf replied:

Finding what the correct API is is, in my opinon, the name of the game for the immediate future of "LibGAP".

I have a clear opinion on this: such an API should overall aim to provide high-level APIs at first; and only add lower-level APIs if there is a strong justification that make the higher level APIs unsuitable (and even then, I'd first investigate making the higher level APIs better). In this case, I think it's a mistake to export all those plist accessors, when we already have generic list accessors. The only reason to use the plist ones is raw performance, but a sizeable portion of that is sacrificed by these non-inlined wrappers anyway. There may be other reasons to export plist-only APIs, but these then should be conscious decisions (and ideally explained in comments).

I completely agree. The API that is in libgap-api.c right now is what I stole from @sebasguts' code, the only "high level" function I added is GAP_EvalString.

I will update this PR with a basic API which I think is useful (informed by the hackmd document you hinted at, and an effort of mine of tying GAP into racket and idris. Needless to say that feedback from serious users of the API (i.e. SageMath and Oscar) would be incredibly useful.

Sorry for being late to the show. I would be perfectly happy with the lowercase-functions, actually. I am not very picky here, we would need to do the input checks done by the lowercase versions for OSCAR anyway. I doubt there is a speed difference when doing it from julia or from GAP. Code to look at can be found here: https://github.com/oscar-system/LibGAP.jl/blob/master/src/libgap.jl

Furthermore, I also think the way @markuspf did the new version is also good!

fingolfin · 2018-08-17T08:56:56Z

src/libgap-api.c

+
+Obj GAP_CallFuncList(Obj func, Obj arg_list)
+{
+    return CallFuncList(func, arg_list);


arg_list -> argList? (to follow our general naming conventions)

fingolfin · 2018-08-17T08:58:08Z

src/libgap-api.h

+/*************************************************************************/
+
+// CallFuncList
+Obj GAP_CallFuncList(Obj, Obj);


Repeating more now-hidden comments:

Personally, I don't like that all argument names are omitted, esp. as there is virtually no documentation; so one has to look at the implementation to figure out what the various functions here do -- also, curiously, in some cases (GAP_AssPList, GAP_ValGVar, ....) the argument name was left in.

That's not a blocker, but since you announced that I may not complain about anything that I don't mention upfront, I'll point out every little detail.

fingolfin · 2018-08-17T08:58:50Z

tst/testlibgap/basic.c

+    printf("gap> %s\n", cmd);
+    res = GAP_EvalString(cmd);
+    rc = GAP_LenPList(res);
+    for(i=1;i<=rc;i++) {


clang-format?

fingolfin · 2018-08-17T09:00:01Z

tst/testlibgap/basic.c

+{
+    printf("Initializing GAP...\n");
+    GAP_Initialize(argc, argv, environ, 0L, 0L);
+    printf("done\n");


Optional: Maybe change to

printf("# Initializing GAP...\n"); GAP_Initialize(argc, argv, environ, 0L, 0L); printf("# done\n");

so that the output file becomes a valid .tst file?

fingolfin · 2018-08-17T09:02:16Z

The spurious ^M comes from the Browse package, see issue #430

fingolfin · 2018-08-17T09:03:24Z

(So you can avoid it if you e.g. start gap with the -A option)

markuspf · 2018-08-17T11:49:03Z

Just to note: I did not intend to hide the comments (I even copied them into a textfile on my machine to not forget about the points raised), but I am fiddling on this PR to move it closer to mergeability (or being obsoleted by smaller PRs, whichever comes first)

stevelinton · 2018-08-18T10:04:45Z

Generally this looks good to me.

On the "plist" question. I agree with the general sentiment of not exposing ELM_PLIST, SET_ELM_PLIST, etc. Way back when Martin Schoenert suggested that kernel modules that worked with lists they were given should go through the generic list interface in almost all cases. The PLIST Interface was primarily for functions that needed to create a new list.

In this context, the use case would be a libGAP client that needed to put some objects in a list to pass to some GAP function. In that context it is natural and safe to use

l  = NEW_PLIST( type, len);
for (i = 1; i <= len; i++) {
  SET_ELM_PLIST(l, i, thing);
  CHANGED_BAG(l);  # unless you know for sure that thing is kept alive
}
SET_LEN_PLIST(l);

I don't know what the performance penalty fro going through ASS_LIST instead would be.

I wonder if it might be appropriate to add an API function that simply does this whole job -- takes a C array (or C++ vector) of GAP objects and makes them into a GAP list. Less critically, one could do something similar for records.

fingolfin · 2018-08-18T15:50:00Z

While I agree that a conversion function that takes a C array of GAP refs and turns it into a plist, I would suggest to only add APIs they are explicitly requested.

ChrisJefferson · 2018-08-19T19:13:55Z

I also would want to think about a function that made a plist from an array, because I would worry about people mallocing that array, which would be bad (as the GC wouldn't see it while it was in malloced memory).

ChrisJefferson · 2018-08-19T19:16:28Z

With regards list functions, I would prefer to only share this like AssList, and maybe wrap new plist to not require the last option which requires a tnum.

My preferred design would be to have some way of marking functions which can cause crashes with UNSAFE, or some similar marker, and encourage people to mostly use functions which check their arguments. However some external users might think that wouldn't be fast enough (I think the they would be wrong)

ChrisJefferson · 2018-08-19T19:24:05Z

Not that this is not a requirement. Also we could "officially" mark the API as subject to change (by putting a if comment on the file /release notes), which would give us the chance to make changes with our first couple of users of the gap API.

markuspf · 2018-08-21T13:01:28Z

What would be that "first half" to be committed? Something like this?

Make a PR that allows disabling the REPL
Makefile updates (removing --enable-libgap)
libgap.{h,c} that only contain GAP_Initialize and GAP_EvalString? None of the rest is useful at this stage in my opinion.

Also, while I am not overly fussed with names, I have in the back of my head some opposition to calling this baby libgap. Can we (this is not "markus and max we", but "whoever wants to speak, we") please agree (on some level) on a name and then move on? I think we have now suitably nailed down what we're trying to achieve.

My (current) preferred name is GAP (or just API), and calling the includes either api.c and api.h, or gap-api.c and gap-api.h.

markuspf · 2018-08-21T13:04:42Z

@fingolfin you might notice that I indeed put "WIP" in the title to signify that this is work in progress, i.e. experimentation, but visible to interested parties so they can, if they want to, comment.

I also don't believe that this code, as is, compiles at all.

I am aware of the problems wrt bool, missing return values, and inconsistent design; that's because I am experimenting with writing the code at the moment (also to actually have something to discuss in a developer call, or on the mailing list, or a PR, for that matter).

fingolfin · 2018-08-21T14:06:50Z

Sorry, I missed the title change

markuspf · 2018-08-21T15:09:35Z

Updated to only contain the "first half" of the original PR; To make even testlibgap run, I added some functions for dealing with lists.
Of course, I can also "hide" this in tst/testlibgap/basic.c file by calling GAP kernel functions directly there to prevent bikeshedding right now.

I'd like to move towards merging this PR now, which means: please comment about things that need to be changed now to get this merged, and I'll open a new PR with a proposed API for more advanced work.

markuspf · 2018-08-22T08:48:54Z

Note that we need #2723 for this now to stop GAP from starting an invisible shell which blocks the process that calls GAP_Intitialize

fingolfin · 2018-08-25T12:49:00Z

.travis.yml

@@ -108,6 +108,9 @@ matrix:
    # test error reporting and compiling (quickest job in this test suite)
    - env: TEST_SUITES="testspecial test-compile"

+    # test libgap
+    - env: TEST_SUITES="testlibgap" CONFIGFLAGS="--enable-debug --enable-Werror"


Those CONFIGFLAGS can be dropped now, they are the default ones.

fingolfin · 2018-08-25T12:51:40Z

Makefile.rules

@@ -395,6 +397,8 @@ gap$(EXEEXT): $(OBJS) cnf/GAP-LDFLAGS cnf/GAP-LIBS cnf/GAP-OBJS

 endif

+libgap.so: $(OBJS)
+	$(QUIET_LINK)$(LINK) $(GAP_LDFLAGS) -shared $(OBJS) $(GAP_LIBS) -o $@


Hm... I just realized that the libgap target does not depend on libgap.so; and we already have a libgap.la; something does not quite add up here?!

fingolfin · 2018-08-25T12:53:29Z

src/libgap-api.c

+    UInt len;
+
+    if (IS_STRING(string)) {
+        copy = CopyToStringRep(string);


This creates a copy, even if the original string was already in string rep. Perhaps instead do this:

if (!IS_STRING_REP(string) string = CopyToStringRep(string);

and then use string instead of copy below?

fingolfin · 2018-08-25T12:56:30Z

src/libgap-api.h

+// Users of GAP as a library currently call directly into
+// GAP kernel functions. In the future we would like to hide
+// all GAP internal functions (in the .so/.dll file) and only
+// allow access to GAP via functions defined in this file.


Do "we"? This is a goal some of us might have; but it would also force all kernel extensions to be rewritten, for no clear gain, so even among us I am not sure this is universally supported; and there are several folks who have not chimed in on this at all yet.

Personally, I am not yet convinced this approach with a brand new API created for external consumption is viable. I remember similar approaches taken e.g. by OS vendors in the past; almost always people search for (and found) ways to work around this, because the "official" APIs just were missing too many useful or even crucial things.

Another problem with such external API is that it means GAP kernel developers won't "dogfood" them: I'll probably never use these APIs, thus won't be aware of issues with them, won't be in a good position to maintain them.

All in all, I really prefer what the Julia folks did, where they simply mark select functions as "official", by adding something like "GAP_EXPORT" for such symbols. The main (only?) issue people seem to have with that in GAP is namespacing, and the risk of name clashes, which Julia mostly avoids by adding the jl_ prefix to all identifiers. But there are ways around that, too; e.g. using #define's to rename all APIs in GAP transparently; or even just simply adding a GAP specific prefix.

Anyway, don't get me wrong: I am not opposed to merging this as-is for now, as long as people keep an open mind to revise and revisit this as needed; I'd hope that developing more actual "clients" for the API wil make it much clearer what works and what doesn't.

Well, I have to admit that I am losing patience with this as well. I am not (productively) using this API, and none of the "stakeholders" have chimed in either; yet "we" (mainly you and me) have sunk quite a bit of time dancing around this tree.

So unless we get feedback from @nthiery or @sebasguts I'll remove the custom API functions, I suggest we merge just the building of the dynamic object file and the GAP_EvaluateString, and leave it at that unless a demand shows up (together with a person who implements that).

The only additional thing I can suggest is emailing the GAP developers mailing list and see whether any feedback transpires there.

Outdated

nthiery · 2018-08-27T14:32:47Z

Well, I have to admit that I am losing patience with this as well. I am not (productively) using this API, and none of the "stakeholders" have chimed in either; yet "we" (mainly you and me) have sunk quite a bit of time dancing around this tree.

Yeah, I know; I am sorry the timing does not work right. It's frustrating for me as well not to be able to jump in the discussion at a time where you are available and investing a lot of efforts. I'd rather be playing with the new GAP API than taking care of reports as I have been doing in the last two weeks. Gosh, one more week to go; and then teaching starts ... Overall my impression is that, on Sage's side, we could adapt quite easily whatever the API is. From the discussions we had you know the kind of feature we need. So proceed to wherever feels best from GAP's perspective.

fingolfin · 2018-08-28T11:13:58Z

Makefile.rules

@@ -389,13 +388,16 @@ gap$(EXEEXT): libgap.la cnf/GAP-LDFLAGS cnf/GAP-LIBS cnf/GAP-OBJS

 else

+all: libgap.so
+libgap.so: symlinks libgap.la
+	$(QUIET_LINK)$(LINK) $(GAP_LDFLAGS) -shared $(OBJS) $(GAP_LIBS) -o $@


Disclaimer: the following is not meant to block this PR as such, just tries to clarify some background.

When I just looked at the above lines, I wondered again about the relation between libgap.so and libgap.la. There is now a dependency here -- but there seems no strict technical reason.

Moreover, on Mac OS X, the suffix .so is wrong, it should be .dylib. And of course on Windows one would expect gap.dll or so. But on Windows, we technically already build GAP into a DLL, as we need that, in order to be able to support kernel extensions at all.

However, all this has an "easy" solution: libgap.la already builds a shared library. That's what libtool .la files are for: they abstract over static/shared libraries in a portable way. Building libgap.la creates .libs/libgap.dylib for me on Mac OS X. Specifically:

$ ls .libs/ libgap.0.dylib libgap.dylib libgap.la libgap.lai

So it already takes care of things like file extensions.

We abuse this for the Cygwin builds:

bin/$(GAPARCH)/gap.dll: symlinks libgap.la cp .libs/cyggap-0.dll $@ # FIXME: HACK to support kernel extensions

But that' a bad HACK, which I only added to make things work quickly. In reality, one should not directly access files in .libs, but rather use the libtool script to use these files, e.g. to link against them, or to install them.

Indeed, installing a .la may cause libtool to relink it (because the rpaths and other settings that one needs to use the library may differ between when using it inside a build directory during development; and when using it as a system wide installed library).

This all means that this libgap.so target should be temporary at best. What we should do instead, though, depends a bit on how libgap.so is supposed to be used: are people right now linking against it by specifying -L/path/to/GAPROOT -lgap? If so, and if they are using GNU libtool as well, they are better of specifying /path/to/GAPROOT/libgap.la. If so, but they are not using GNU libtool, it becomes a bit tricky.
Of course, if instead one requires people to do a make install from within GAP (which is not fully implemented, in particular the libgap part...), then all that goes away...

All in all, the best long term solution probably is to finally implement make install, or at least enough of it to make libgap usable by clients. I always planned to work on that eventually (but demand wasn't exactly high), and would be happy to collaborate on it, too.

The way I use libgap at the moment is by

(define-ffi-definer define-gap (ffi-lib "/home/makx/git/gap/master/.libs/libgap.so"))

in Racket (well knowing that this is of course at best a hack, too). If libgap.so were installed, I'd probably be able to just use `ffi-lib "gap".

I do not require header files, but I have to give GAP_Initialize the path to the GAP library. I believe this is mostly how SageMath uses libgap as well.

The reason the .so target is there is because I apparently still did not understand libtool. I did remember that the .la file is all we need, and that the .so file just appeared for me in .libs.

Happy to learn how to do the make install so that enough people are happy (but for that we need people to use it).

fingolfin

I'd be OK with merging this as it currently is (not much is left... ;-). The issue with libgap.la vs. libgap.so can be resolved later and separately.

nthiery · 2018-08-28T11:35:04Z

What we should do instead, though, depends a bit on how libgap.so is supposed to be used: are people right now linking against it by specifying -L/path/to/GAPROOT -lgap? If so, and if they are using GNU libtool as well, they are better of specifying /path/to/GAPROOT/libgap.la. If so, but they are not using GNU libtool, it becomes a bit tricky. ... All in all, the best long term solution probably is to finally implement make install, or at least enough of it to make libgap usable by clients. I always planned to work on that eventually (but demand wasn't exactly high), and would be happy to collaborate on it, too.

From Sage's (and packagers I assume) perspective: make install would be great. But in the meantime, since we are using autotools in many other places anyway, it should be fine to use libtool. I would just need to double check our build rule since Cython is involved too, and I don't remember whether Cython does the linking too.

markuspf · 2018-08-29T07:59:24Z

Squashed and rebased on master. If tests pass I'll merge this.

markuspf added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: build system release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 15, 2018

markuspf force-pushed the libgap branch from 3a49cdb to b3a8203 Compare August 15, 2018 10:15

fingolfin mentioned this pull request Aug 15, 2018

buildsys: simplify C compilation build rule #2704

Merged

fingolfin previously requested changes Aug 15, 2018

View reviewed changes

markuspf mentioned this pull request Aug 15, 2018

Introduce SetExtraMarkFuncBags #2706

Merged

markuspf force-pushed the libgap branch from b3a8203 to 5739000 Compare August 15, 2018 12:57

markuspf mentioned this pull request Aug 15, 2018

Export READ_ALL_COMMANDS via header file #2707

Merged

markuspf force-pushed the libgap branch from 85b57c7 to 3f7dd1c Compare August 16, 2018 20:34

markuspf commented Aug 16, 2018

View reviewed changes

markuspf force-pushed the libgap branch from 01275c9 to f73a518 Compare August 17, 2018 04:53

fingolfin reviewed Aug 17, 2018

View reviewed changes

markuspf force-pushed the libgap branch from f73a518 to 579d428 Compare August 17, 2018 11:51

markuspf added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Aug 20, 2018

markuspf changed the title ~~Add GAP kernel API (aka libgap)~~ WIP: Add GAP kernel API (aka libgap) Aug 20, 2018

markuspf force-pushed the libgap branch from 033dfaf to e15a27f Compare August 21, 2018 14:29

markuspf mentioned this pull request Aug 21, 2018

Introduce command line options --norepl and --nointeract, remove IsLIBGAP constant #2723

Merged

markuspf force-pushed the libgap branch 2 times, most recently from f7cae72 to 4eb1b46 Compare August 21, 2018 14:54

markuspf changed the title ~~WIP: Add GAP kernel API (aka libgap)~~ Add GAP kernel API (aka libgap) Aug 21, 2018

markuspf removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Aug 21, 2018

markuspf force-pushed the libgap branch from 4eb1b46 to 533f865 Compare August 21, 2018 14:55

markuspf force-pushed the libgap branch 2 times, most recently from 054c98e to 5b21da2 Compare August 22, 2018 08:38

markuspf force-pushed the libgap branch 3 times, most recently from 7a5e877 to b084bea Compare August 24, 2018 18:50

fingolfin reviewed Aug 25, 2018

View reviewed changes

fingolfin reviewed Aug 28, 2018

View reviewed changes

fingolfin approved these changes Aug 28, 2018

View reviewed changes

Add GAP kernel API (aka libgap)

f8f85a0

markuspf force-pushed the libgap branch from 5f0d519 to f8f85a0 Compare August 28, 2018 21:51

markuspf merged commit 10de057 into gap-system:master Aug 29, 2018

markuspf mentioned this pull request Aug 29, 2018

Switch Travis to use Ubuntu 16.04 "xenial", and other CI tweaks #2759

Merged

fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Sep 12, 2018

Add GAP kernel API (aka libgap) #2702

Add GAP kernel API (aka libgap) #2702

Conversation

markuspf commented Aug 15, 2018 • edited Loading

codecov bot commented Aug 15, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

markuspf commented Aug 16, 2018 • edited Loading

markuspf commented Aug 16, 2018

fingolfin commented Aug 16, 2018

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

markuspf commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Aug 17, 2018

fingolfin commented Aug 17, 2018

markuspf commented Aug 17, 2018

stevelinton commented Aug 18, 2018

fingolfin commented Aug 18, 2018

ChrisJefferson commented Aug 19, 2018

ChrisJefferson commented Aug 19, 2018

ChrisJefferson commented Aug 19, 2018

markuspf commented Aug 21, 2018

markuspf commented Aug 21, 2018

fingolfin commented Aug 21, 2018

markuspf commented Aug 21, 2018

markuspf commented Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markuspf Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

nthiery commented Aug 27, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

nthiery commented Aug 28, 2018 via email

markuspf commented Aug 29, 2018

markuspf commented Aug 15, 2018 •

edited

Loading

codecov bot commented Aug 15, 2018 •

edited

Loading

markuspf commented Aug 16, 2018 •

edited

Loading

markuspf commented Aug 22, 2018 •

edited

Loading

markuspf Aug 27, 2018 •

edited

Loading