Opened 4 years ago

Closed 4 years ago

#18681 closed enhancement (fixed)

Separate the rankwidth library into a standard package

Reported by: mjo Owned by:
Priority: major Milestone: sage-6.8
Component: graph theory Keywords:
Cc: jdemeyer, ncohen Merged in:
Authors: Nathann Cohen Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 3681e06 (Commits) Commit: 3681e06d8de78f2e65b73500ae1dfc6d8893a5e5
Dependencies: Stopgaps:

Description (last modified by ncohen)

This is a follow-up to,

https://groups.google.com/forum/#!topic/sage-devel/HF52TdkH0fw

The author of the rankwidth (rw) package was very helpful in getting the library/executable packaged with autotools. The new release is available here:

http://pholia.tdi.informatik.uni-frankfurt.de/~philipp/software/rw-0.7.tar.gz

The first big change is that it uses autotools, obviously. So the standard ./configure && make should work. But more importantly, we can now compile the shared library without the executable, and without the resulting dependency on igraph. If you use the default ./configure:

$ ./configure
...
configure: error: Package requirements (igraph >= 0.6) were not met:

No package 'igraph' found

But if you pass the --enable-executable=no flag,

$ ./configure --enable-executable=no

it will succeed without igraph and build the library.

Change History (25)

comment:1 Changed 4 years ago by mjo

  • Cc jdemeyer added; jdemeyed removed

comment:2 follow-up: Changed 4 years ago by ncohen

Have you started turning it into a Sage package?

comment:3 in reply to: ↑ 2 Changed 4 years ago by mjo

Replying to ncohen:

Have you started turning it into a Sage package?

Nope, no work on that yet.

comment:4 Changed 4 years ago by ncohen

Okay, I'll do it tomorrow then.

comment:5 Changed 4 years ago by ncohen

  • Authors set to Nathann Cohen
  • Branch set to public/18681
  • Commit set to 938002493f84cee8e56e7e0a3ae504a1b8a60874
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

9380024trac #18681: 'rw' as a package

comment:6 Changed 4 years ago by git

  • Commit changed from 938002493f84cee8e56e7e0a3ae504a1b8a60874 to 687888df22d84d548e1668062a9717e068b6d1b1

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

687888dtrac #18681: 'rw' as a package

comment:7 Changed 4 years ago by jdemeyer

The license is GPL version 2 or later.

comment:8 Changed 4 years ago by jdemeyer

comment:9 Changed 4 years ago by jdemeyer

These lines should be removed from the .pxd file, since they are not in the API of rw:

subset_t *slots

uint_fast8_t num_vertices

Move this inside the cdef extern from "rankwidth_c/rw.h": block (removing the stuff in quotes and uint_fast8):

    ctypedef int uint_fast8_t
    ctypedef int uint_fast32_t
    ctypedef int subset_t

comment:10 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:11 Changed 4 years ago by jdemeyer

And this is clearly a private function, this shouldn't be in the pxd either:

cdef void set_am(int i, int j, int val)

comment:12 Changed 4 years ago by git

  • Commit changed from 687888df22d84d548e1668062a9717e068b6d1b1 to 8002dc43a5f94ce4a584dae56631ecb9d161ebc9

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

8002dc4trac #18681: Reviewer's comments

comment:13 follow-up: Changed 4 years ago by ncohen

  • Status changed from needs_work to needs_review

I made all modifications except the one about the licence: where did you see that? I said 'GPL2' in SPKG.txt because that's what the COPYING file contains.

Nathann

comment:14 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to ncohen:

I made all modifications except the one about the licence: where did you see that? I said 'GPL2' in SPKG.txt because that's what the COPYING file contains.

Look in the code (rw.c for example).

Note that Sage is released under GPL version 3 (the only license which is compatible with all standard packages), so if it really was GPL version 2 only, that would be a problem.

comment:15 Changed 4 years ago by jdemeyer

Do you really need this stuff in quotes:

"uint_fast8_t"

(same for the other two)

comment:16 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by ncohen

Yo !

Look in the code (rw.c for example).

Well, that's two contradicting sources. Should we ask upstream to use the same in both places?

Note that Sage is released under GPL version 3 (the only license which is compatible with all standard packages), so if it really was GPL version 2 only, that would be a problem.

Sigh... T_T

Nathann

comment:17 Changed 4 years ago by git

  • Commit changed from 8002dc43a5f94ce4a584dae56631ecb9d161ebc9 to f0556317ae56404b1171b4d990be769599a0af63

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

f055631trac #18681: Reviewer's comments

comment:18 Changed 4 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:19 in reply to: ↑ 16 Changed 4 years ago by jdemeyer

Replying to ncohen:

Well, that's two contradicting sources.

It's not contradicting. The code is what matters. Essentially it says "this code is released until GPL v2 or later, see the file COPYING for the GPL v2 text"

comment:20 follow-up: Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

rw should be a dependency of the Sage library in build/deps.

comment:21 in reply to: ↑ 20 Changed 4 years ago by ncohen

  • Status changed from needs_work to needs_review

rw should be a dependency of the Sage library in build/deps.

Arg... True :-/

Can you think of a way to make this automatic?

Nathann

comment:22 Changed 4 years ago by git

  • Commit changed from f0556317ae56404b1171b4d990be769599a0af63 to 3681e06d8de78f2e65b73500ae1dfc6d8893a5e5

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

3681e06trac #18681: rw is a dependency of Sage

comment:23 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:24 Changed 4 years ago by ncohen

Thanks!

comment:25 Changed 4 years ago by vbraun

  • Branch changed from public/18681 to 3681e06d8de78f2e65b73500ae1dfc6d8893a5e5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.