Opened 16 months ago

Closed 6 months ago

#14349 closed defect (fixed)

The cliquer spkg is patching upstream

Reported by: Snark Owned by: tbd
Priority: major Milestone: sage-6.2
Component: packages: standard Keywords:
Cc: fbissey Merged in:
Authors: Felix Salfelder Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: public/14349 (Commits) Commit: 625306835f2033c96a982a3b4cee0b87b708f8e6
Dependencies: #14892 Stopgaps:

Description (last modified by jdemeyer)

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.

Attachments (1)

0001-reduce-cliquer-patching.patch (3.5 KB) - added by felixs 14 months ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 16 months ago by fbissey

  • Cc fbissey added

comment:2 Changed 14 months ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 14 months ago by jdemeyer

  • Description modified (diff)

comment:4 in reply to: ↑ description Changed 14 months ago by jdemeyer

Replying to Snark:

  1. 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.

comment:5 Changed 14 months ago by Snark

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.)

comment:6 Changed 14 months ago by jdemeyer

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

Changed 14 months ago by felixs

comment:7 Changed 14 months ago by felixs

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

comment:8 follow-up: Changed 13 months ago by 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.

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

comment:9 in reply to: ↑ 8 Changed 13 months ago by felixs

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.

comment:10 Changed 13 months ago by vbraun

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

comment:11 Changed 13 months ago by felixs

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)

comment:12 follow-up: Changed 13 months ago by fbissey

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

comment:13 in reply to: ↑ 12 Changed 13 months ago by felixs

Replying to fbissey:

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

I have created #14887.

comment:14 Changed 13 months ago by vbraun

  • Dependencies set to #14892

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.

comment:15 Changed 12 months ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:16 Changed 11 months ago by felixs

  • Branch set to u/felixs/14349
  • Commit set to aecdf29f39b29c5b7438939a08f54b6ff18fc347
  • Status changed from new to needs_review

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

comment:17 Changed 8 months ago by ncohen

  • Dependencies changed from #14892 to #14892, #15410
  • Status changed from needs_review to needs_info

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

comment:18 Changed 8 months ago by vbraun

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/....

comment:19 Changed 8 months ago by ncohen

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

comment:20 follow-up: Changed 8 months ago by vbraun

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.

comment:21 in reply to: ↑ 20 Changed 8 months ago by ncohen

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

comment:22 Changed 8 months ago by ncohen

  • Status changed from needs_info to needs_review

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

comment:23 Changed 7 months ago by ncohen

ping ?

comment:24 Changed 7 months ago by ncohen

  • Branch changed from u/felixs/14349 to public/14349
  • Commit aecdf29f39b29c5b7438939a08f54b6ff18fc347 deleted
  • Dependencies changed from #14892, #15410 to #14892

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

comment:25 Changed 7 months ago by git

  • Commit set to 625306835f2033c96a982a3b4cee0b87b708f8e6

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

f55a304python-sage work without cliquer patching (#14349)
aecdf29module_list: add cl.c to cliquer.so sources (#14349)
25edb2ftrac #14349: rebase on 6.1.beta4
6253068trac #14349: move cl.* to cliquer/

comment:26 Changed 7 months ago by felixs

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

comment:27 Changed 7 months ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

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

comment:28 Changed 7 months ago by jdemeyer

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

comment:29 Changed 6 months ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:30 Changed 6 months ago by vbraun

  • Authors set to Felix Salfelder

comment:31 Changed 6 months ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.