Opened 5 years ago

Closed 4 years ago

#25273 closed enhancement (invalid)

Unprefixed libGAP interface

Reported by: Jeroen Demeyer Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: interfaces Keywords:
Cc: Samuel Lelièvre Merged in:
Authors: Jeroen Demeyer Reviewers:
Report Upstream: N/A Work issues:
Branch: u/dimpase/unprefixed_libgap_interface (Commits, GitHub, GitLab) Commit: 48e4f7b9ec98c13acf6345689bea442b30ac316f
Dependencies: Stopgaps:

Status badges

Description (last modified by Samuel Lelièvre)

Use Cython cname declarations to use unprefixed libGAP names in Sage. The libGAP package is not changed at all, only the Sage interface.

Obvious exceptions:

  • libGAP_True
  • libGAP_False

Change History (32)

comment:1 Changed 5 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 5 years ago by Jeroen Demeyer

Description: modified (diff)

comment:3 Changed 5 years ago by Jeroen Demeyer

Dependencies: #25274

comment:4 Changed 5 years ago by Dima Pasechnik

Dependencies: #25274

Does this mean that libgap.Blah syntax won't be needed, too?

comment:5 Changed 5 years ago by Dima Pasechnik

Dependencies: #25274

comment:6 Changed 5 years ago by Jeroen Demeyer

Branch: u/jdemeyer/unprefixed_libgap_interface

comment:7 Changed 5 years ago by Jeroen Demeyer

Commit: ea2e39c7374b03b97366f264c89a0f4c75a7c172
Status: newneeds_review

New commits:

b8c3c12Clean up cimports in libgap
ea2e39cUnprefixed libGAP interface

comment:8 Changed 5 years ago by Jeroen Demeyer

Description: modified (diff)

comment:9 Changed 5 years ago by Dima Pasechnik

Description: modified (diff)

In libgap.pyx you have

The ``foobar`` methods are
the original GAP methods simply prefixed with the string
````.

which looks like an artefact of non-interactive editing...

I presume there is no prefix any more.

comment:10 Changed 5 years ago by git

Commit: ea2e39c7374b03b97366f264c89a0f4c75a7c17272b5066bb6b1606bef5d7e88c1a8d5fab62b5192

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

72b5066Unprefixed libGAP interface

comment:11 Changed 5 years ago by git

Commit: 72b5066bb6b1606bef5d7e88c1a8d5fab62b5192d28b5757ad45cd54ae70adf25c72fcefef8b1a01

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d28b575Unprefixed libGAP interface

comment:12 Changed 5 years ago by Dima Pasechnik

The only libGAP_ things left are libGAP_True/False. There are also libgap_-things, but this is another story. The docs about libGAP_ ought to be more explicit about this.

comment:13 Changed 5 years ago by Samuel Lelièvre

Cc: Samuel Lelièvre added
Description: modified (diff)

comment:14 Changed 5 years ago by Jeroen Demeyer

Milestone: sage-8.3sage-pending

I'm not sure anymore that we should do this because it's not at all clear what upstream GAP is going to do. Now that merging libGAP upstream looks further away, they might be convinced to change (some of) these names.

comment:15 Changed 5 years ago by Dima Pasechnik

But it's just two names that need special treatment - I don't see why you want to postpone this.

As well, I think that if one tells GAP people that a couple of macros need to be renamed to be able to inter-operate with Python etc, they would be happy with such a change.

comment:16 Changed 4 years ago by Dima Pasechnik

Branch: u/jdemeyer/unprefixed_libgap_interfaceu/dimpase/unprefixed_libgap_interface
Commit: d28b5757ad45cd54ae70adf25c72fcefef8b1a0148e4f7b9ec98c13acf6345689bea442b30ac316f
Dependencies: #25274

rebased over the latest beta, fixed bug. We could perhaps finish and merge this, as an intermediate step towards #22626.


New commits:

f9bdacdrebased over 8.4.b4
12cc10cMerge branch 'develop' of trac.sagemath.org:sage into HEAD
48e4f7bcorrecting a typo (?)

comment:17 Changed 4 years ago by Jeroen Demeyer

Is it clear what upstream's policy on the naming and prefixing is? See https://trac.sagemath.org/ticket/22626#comment:66

comment:18 Changed 4 years ago by Jeroen Demeyer

Status: needs_reviewneeds_info

comment:19 Changed 4 years ago by Dima Pasechnik

Well, GAP people are reluctant to change names; in particular T_NUM is merely a C enum, and we don't even need to use it (certainly not with the new, since GAP 4.9, functions).

comment:20 in reply to:  19 Changed 4 years ago by Jeroen Demeyer

Replying to dimpase:

Well, GAP people are reluctant to change names

Did anybody actually try to convince them? Ideally, they would change their names to reduce changes of name collisions. Since this is C, this matters regardless of whether we actually use those names.

comment:21 Changed 4 years ago by Dima Pasechnik

I am at https://www.gapdays.de/gapdays2018-fall/ and spent some time yesterday and today talking about this issue with Markus and Max. They say that as they have no control over their packages, changing API is tricky.

If you can come up with a convincing argument, please do, I'd do my best to communicate it while here. Are there any name clashes in exported symbols in their new libgap-API? IMHO there are none.

comment:22 Changed 4 years ago by Dima Pasechnik

Milestone: sage-pendingsage-8.4
Status: needs_infoneeds_review

While we at it, are libgap_enter() and libgap_exit() needed? GAP people told me that they are not. I tried removing them and running ptestlong (with gap_packages installed for a better coverage), no errors (apart from the expected one in libs/gap/util.pyx).

I suggest we proceed with this ticket - it does not look as if #22626 will be ready soon, perhaps we should rather try rebasing libGAP to GAP 4.10, which looks a less daunting task now than a total replacement with "native" GAP's libgap.

comment:23 in reply to:  21 Changed 4 years ago by Jeroen Demeyer

(sorry I seem to have missed your previous comment)

Replying to dimpase:

They say that as they have no control over their packages, changing API is tricky.

Changing API should not be tricky if you add a compatibility layer changing the new names to the old names. My proposal should literally be 100% API-compatible for GAP packages.

Are there any name clashes in exported symbols in their new libgap-API?

I haven't tried the new libgap-API, so I cannot tell. But it's not only about exported symbols, it's about anything defined in the GAP header files. In particular T_INT is problematic that way (assuming that it still exists in the new libgap-API).

comment:24 Changed 4 years ago by Dima Pasechnik

What I did on #22626 is just simply not use T_INT (this is a constant equal to 0), I just used 0 instead.

comment:25 in reply to:  24 Changed 4 years ago by Jeroen Demeyer

Replying to dimpase:

What I did on #22626 is just simply not use T_INT (this is a constant equal to 0), I just used 0 instead.

First of all, I'd consider that very bad style. For example, what if GAP suddenly decided to change the value of T_INT?

Second, the question is not whether T_INT is used. The question is whether it's defined in a header file.

comment:26 Changed 4 years ago by Dima Pasechnik

T_INT won't change "suddenly", for sure, as these T_* things are heavily used in GAP's low-level type system.

As I said, as I didn't understand the problem fully (and still don't), I wasn't able to convince GAP devs that something needs to be done about it.

comment:27 Changed 4 years ago by Dima Pasechnik

For the record, in GAP T_INT is a part of enum type TNUM in src/objects.h, i.e. one sees there

enum TNUM {
 ....
 T_INT;
 ...
};

comment:28 Changed 4 years ago by Jeroen Demeyer

The problem is that the Python header structmember.h file also does

#define T_INT       1

You can hope that the GAP headers won't be included after that Python header, but that seems dangerous to me especially because we cannot really control the code that Cython generates.

comment:29 in reply to:  28 Changed 4 years ago by Dima Pasechnik

Replying to jdemeyer:

The problem is that the Python header structmember.h file also does

#define T_INT       1

You can hope that the GAP headers won't be included after that Python header, but that seems dangerous to me especially because we cannot really control the code that Cython generates.

If GAP's headers are included after Python's, you'd get a compilation error. Indeed

#include <stdio.h>
#define T_INT 42
enum TNUM { T_INT, T_FOO };
int main(){
    int i;
    i = T_INT;
    printf("%d\n", i);
    return 0;}

and

$ gcc t.c 
t.c:2:15: error: expected identifier before numeric constant
 #define T_INT 42
               ^~
t.c:3:13: note: in expansion of macro ‘T_INT’
 enum TNUM { T_INT, T_FOO };
             ^~~~~

But if you swap the inclusion order, then yes, everything compiles, and overrides the enum. Indeed

#include <stdio.h>
enum TNUM { T_INT, T_FOO };
#define T_INT 42
int main(){
    int i;
    i = T_INT;
    printf("%d\n", i);
    return 0;}

compiles and outputs 42.

Not sure whether and how this could affect libgap...

Last edited 4 years ago by Dima Pasechnik (previous) (diff)

comment:30 Changed 4 years ago by Jeroen Demeyer

I'd rather not make the compilation of Sage depend on a specific order of includes.

comment:31 Changed 4 years ago by Dima Pasechnik

I'd be more concerned about Python's #define shadowing T_INT: a compilation error is much easier to deal with than a vague runtime one...

I'd also say that here Python is the bad guy, not using enum for its T_* constants. (I'd say it should rather be using enum, no?)

comment:32 Changed 4 years ago by Jeroen Demeyer

Milestone: sage-8.4sage-duplicate/invalid/wontfix
Resolution: invalid
Status: needs_reviewclosed

Obsolete by #22626

Note: See TracTickets for help on using tickets.