Opened 10 years ago
Closed 9 years 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, GitHub, GitLab) | Commit: | 625306835f2033c96a982a3b4cee0b87b708f8e6 |
Dependencies: | #14892 | Stopgaps: |
Description (last modified by )
This spkg does the following:
- 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!)
- it modifies the api by making parse_input exported (non-static) -- was this forwarded upstream?
- it adds new functions, only for sage
- it uses
make
instead of$MAKE
. - it has testing code inside
spkg-install
instead ofspkg-check
.
Attachments (1)
Change History (32)
comment:1 Changed 10 years ago by
Cc: | fbissey added |
---|
comment:2 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:4 Changed 10 years ago by
comment:5 Changed 10 years ago by
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.)
Changed 10 years ago by
Attachment: | 0001-reduce-cliquer-patching.patch added |
---|
comment:7 Changed 10 years ago by
Would somebody please help getting rid of the other one (cl.c.patch)?
comment:8 follow-up: 9 Changed 10 years ago by
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 Changed 10 years ago by
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 10 years ago by
Felix, are you still working on this ticket? We need to resolve this ticket one way or another for #14781
comment:11 Changed 10 years ago by
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: 13 Changed 10 years ago by
What it your problem with gap? Does it have ticket number?
comment:13 Changed 10 years ago by
comment:14 Changed 10 years ago by
Dependencies: | → #14892 |
---|
comment:15 Changed 9 years ago by
Milestone: | sage-5.11 → sage-5.12 |
---|
comment:16 Changed 9 years ago by
Branch: | → u/felixs/14349 |
---|---|
Commit: | → aecdf29f39b29c5b7438939a08f54b6ff18fc347 |
Status: | new → 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 9 years ago by
Dependencies: | #14892 → #14892, #15410 |
---|---|
Status: | needs_review → 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 9 years ago by
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 9 years ago by
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: 21 Changed 9 years ago by
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 Changed 9 years ago by
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 9 years ago by
Status: | needs_info → 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:24 Changed 9 years ago by
Branch: | u/felixs/14349 → public/14349 |
---|---|
Commit: | aecdf29f39b29c5b7438939a08f54b6ff18fc347 |
Dependencies: | #14892, #15410 → #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 9 years ago by
Commit: | → 625306835f2033c96a982a3b4cee0b87b708f8e6 |
---|
comment:26 Changed 9 years ago by
i'd prefer fewer directories. but i agree with 625306835f2033c96a982a3b4cee0b87b708f8e6. consider these changes positively reviewed.
comment:27 Changed 9 years ago by
Reviewers: | → Nathann Cohen |
---|---|
Status: | needs_review → 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:29 Changed 9 years ago by
Milestone: | sage-6.1 → sage-6.2 |
---|
comment:30 Changed 9 years ago by
Authors: | → Felix Salfelder |
---|
comment:31 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | positive_review → closed |
Replying to Snark:
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.