The cliquer spkg is patching upstream

[SnarkBoojum]

This spkg does the following:

1. it compiles cliquer not only as an executable, but also as a library -- here I can't help but notice that the patches do to likewise in debian look more complete see here (and remember than when the header of a debian patch doesn't mention forwarding to upstream -- that means it was!)
2. it modifies the api by making parse_input exported (non-static) -- was this forwarded upstream?
3. it adds new functions, only for sage
4. it uses make instead of $MAKE.
5. it has testing code inside spkg-install instead of spkg-check.

Depends on #14892

CC: @kiwifb

Component: packages: standard

Author: Felix Salfelder

Branch/Commit: public/14349 @ 6253068

Reviewer: Nathann Cohen

Issue created by migration from https://trac.sagemath.org/ticket/14349

[jdemeyer]

Replying to @SnarkBoojum:

3. it adds new functions, only for sage

Please clarify how you would add these functions to Sage? They need access to the cliquer internals, so they cannot simply be added to Sage.

[SnarkBoojum]

comment:5

I don't remember the details, but I think it was a case of bad use of static variables ; so probably helping upstream get around that is a better route.

(That involves putting those variables in a params structure which gets passed around. Perhaps with some getter/setter api for the new structure.)

[jdemeyer]

comment:6

Good luck convincing upstream, I'm not going to do it...

[sagetrac-felixs]

Attachment: 0001-reduce-cliquer-patching.patch.gz

[sagetrac-felixs]

comment:7

Would somebody please help getting rid of the other one (cl.c.patch)?

[vbraun]

comment:8

Most of the "global variables" aren't global but local to the compilation unit of the command line parser and only used to keep track of command line options. Its likely that one could just move the additional sage_... functions to an auxiliary source file in the Sage library.

Maybe we should get a GSoC student working on the build system to clean that up? :-P

Unless there is some action on this ticket soon I'll just take the current patch, add all currently untracked files to the repo, and use that for #14781

[sagetrac-felixs]

comment:9

Replying to @vbraun:

Most of the "global variables" aren't global but local to the compilation unit of the command line parser and only used to keep track of command line options. Its likely that one could just move the additional sage_... functions to an auxiliary source file in the Sage library.

seems to work. i'll test and upload the patch as soon as i'm done with gap.

[vbraun]

comment:10

Felix, are you still working on this ticket? We need to resolve this ticket one way or another for #14781

[sagetrac-felixs]

comment:11

Still stuck with gap. My cliquer patch needs to be sorted out/rebased/ported, I don't know whether that makes much sense right now. please do with this tiket whatever seems appropriate.

(sorry i don't understand #14781)

[kiwifb]

comment:12

What it your problem with gap? Does it have ticket number?

[sagetrac-felixs]

comment:13

Replying to @kiwifb:

What it your problem with gap? Does it have ticket number?

I have created #14887.

[vbraun]

comment:14

I've moved the 0001-reduce-cliquer-patching.patch to #14892. This ticket still has enough improvement suggestions for cliquer. Note how issue #9870 is related to this ticket.

[vbraun]

Dependencies: #14892

[sagetrac-felixs]

Commit: aecdf29

[sagetrac-felixs]

comment:16

A patch removing the patches is now ready on top of sage 5.12beta3 (build_system). It does not address #9870.

[sagetrac-felixs]

Branch: u/felixs/14349

[nathanncohen]

Changed dependencies from #14892 to #14892, #15410

[nathanncohen]

comment:17

Hellooooooooo !!!

This patch probably needs to be rebased atop the nice additions of #15410, but would it also be possible to not create those two cl.c and cl.h files right inside of the graphs/ folder ? Could it be put inside of a cliquer/ folder, and couldn't it even stay inside of the spkg, even if it is not used to patch the sources but (nicely) imported at compile-time ?

The trick is nice and efficient, thanks for this patch :-)

Nathann

[vbraun]

comment:18

I don't understand your question. In any case, upstream sources go into build/pkgs/cliquer, source files for the Sage library into src/sage/....

[nathanncohen]

comment:19

Well, then it would be cool if the cl.c and cl.h files could all be moved inside of a cliquer/ folder, for the graphs/ folder is already quite messy. Especially when these two cl.c and cl.h files are not documented in any way O_o

Would it make sense to get rid of the .c files automatically compiled by Cython for each .pyx file by the way ?

Nathann

[vbraun]

comment:20

You can move them into graphs/cliquer if thats what you are asking.

There is a ticket for in-place cythonization somewhere. Obviously the cython output c files must be somewhere for the compiler to find them.

[nathanncohen]

comment:21

You can move them into graphs/cliquer if thats what you are asking.

Yep, that would be cool.

There is a ticket for in-place cythonization somewhere. Obviously the cython output c files must be somewhere for the compiler to find them.

I guess, but they just don't have to stay there afterwards :-P

Nathann

[nathanncohen]

comment:22

At u/ncohen/14349 you will find three commits atop of this ticket's branch, which rebase it on top of #15410 and 5.13.beta4, plus moves cl.c and cl.h to a new cliquer/ directory.

Nathann

[nathanncohen]

comment:23

ping ?

[nathanncohen]

Changed branch from u/felixs/14349 to public/14349

[nathanncohen]

comment:24

Okay, let's forget about #15410 for the moment as it is not in positive_review anymore. Here is a new commit added on top of Felixs' which moves the two files into a cliquer/ folder.

Nathann

[nathanncohen]

Changed commit from aecdf29 to none

[nathanncohen]

Changed dependencies from #14892, #15410 to #14892

[sagetrac-git]

Commit: 6253068

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

f55a304	python-sage work without cliquer patching (#14349)
aecdf29	module_list: add cl.c to cliquer.so sources (#14349)
25edb2f	trac #14349: rebase on 6.1.beta4
6253068	trac #14349: move cl.* to cliquer/

[sagetrac-felixs]

comment:26

i'd prefer fewer directories. but i agree with 6253068. consider these changes positively reviewed.

[nathanncohen]

Reviewer: Nathann Cohen

[nathanncohen]

comment:27

Well.. I prefer to have the least amount of mess in the graph/ folder. And cl.c/cl.h files do not fit in the picture :-P

Thank you fo the review !

Nathann

[jdemeyer]

comment:28

Further clean-up of cliquer at #9870...

[vbraun]

Author: Felix Salfelder