Opened 3 years ago

Closed 3 years ago

#25273 closed enhancement (invalid)

Unprefixed libGAP interface

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: interfaces Keywords:
Cc: slelievre 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 slelievre)

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 3 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 3 years ago by jdemeyer

  • Dependencies set to #25274

comment:4 Changed 3 years ago by dimpase

  • Dependencies #25274 deleted

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

comment:5 Changed 3 years ago by dimpase

  • Dependencies set to #25274

comment:6 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/unprefixed_libgap_interface

comment:7 Changed 3 years ago by jdemeyer

  • Commit set to ea2e39c7374b03b97366f264c89a0f4c75a7c172
  • Status changed from new to needs_review

New commits:

b8c3c12Clean up cimports in libgap
ea2e39cUnprefixed libGAP interface

comment:8 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:9 Changed 3 years ago by dimpase

  • 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 3 years ago by git

  • Commit changed from ea2e39c7374b03b97366f264c89a0f4c75a7c172 to 72b5066bb6b1606bef5d7e88c1a8d5fab62b5192

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

72b5066Unprefixed libGAP interface

comment:11 Changed 3 years ago by git

  • Commit changed from 72b5066bb6b1606bef5d7e88c1a8d5fab62b5192 to d28b5757ad45cd54ae70adf25c72fcefef8b1a01

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

d28b575Unprefixed libGAP interface

comment:12 Changed 3 years ago by dimpase

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 3 years ago by slelievre

  • Cc slelievre added
  • Description modified (diff)

comment:14 Changed 3 years ago by jdemeyer

  • Milestone changed from sage-8.3 to sage-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 3 years ago by dimpase

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 3 years ago by dimpase

  • Branch changed from u/jdemeyer/unprefixed_libgap_interface to u/dimpase/unprefixed_libgap_interface
  • Commit changed from d28b5757ad45cd54ae70adf25c72fcefef8b1a01 to 48e4f7b9ec98c13acf6345689bea442b30ac316f
  • Dependencies #25274 deleted

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 3 years ago by jdemeyer

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 3 years ago by jdemeyer

  • Status changed from needs_review to needs_info

comment:19 follow-up: Changed 3 years ago by dimpase

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 3 years ago by jdemeyer

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 follow-up: Changed 3 years ago by dimpase

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 3 years ago by dimpase

  • Milestone changed from sage-pending to sage-8.4
  • Status changed from needs_info to needs_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 3 years ago by jdemeyer

(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 follow-up: Changed 3 years ago by 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.

comment:25 in reply to: ↑ 24 Changed 3 years ago by jdemeyer

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 3 years ago by dimpase

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 3 years ago by dimpase

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 follow-up: Changed 3 years ago by 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.

comment:29 in reply to: ↑ 28 Changed 3 years ago by dimpase

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 3 years ago by dimpase (previous) (diff)

comment:30 Changed 3 years ago by jdemeyer

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

comment:31 Changed 3 years ago by dimpase

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 3 years ago by jdemeyer

  • Milestone changed from sage-8.4 to sage-duplicate/invalid/wontfix
  • Resolution set to invalid
  • Status changed from needs_review to closed

Obsolete by #22626

Note: See TracTickets for help on using tickets.