Opened 4 years ago
Closed 4 years ago
#19919 closed enhancement (fixed)
upgrade nauty to version 26 and make it standard
Reported by:  dimpase  Owned by:  

Priority:  major  Milestone:  sage7.1 
Component:  packages: standard  Keywords:  
Cc:  jmantysalo, ncohen, jdemeyer  Merged in:  
Authors:  Dima Pasechnik  Reviewers:  François Bissey, Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  56815d4 (Commits)  Commit:  56815d4da72ff750813797d9158a82b1de9cd199 
Dependencies:  Stopgaps: 
Description (last modified by )
We can now make nauty standard; tarball can be obtained from http://users.ox.ac.uk/~coml0531/sage/nauty26r1.tar.gz
(this is just renamed upstream tarball, inserted '' in the name)
Change History (44)
comment:1 Changed 4 years ago by
 Description modified (diff)
 Summary changed from upgrade nasty to version 26 and make it standard to upgrade nauty to version 26 and make it standard
comment:2 Changed 4 years ago by
 Cc jmantysalo ncohen jdemeyer added
 Status changed from new to needs_review
comment:3 followup: ↓ 5 Changed 4 years ago by
comment:4 Changed 4 years ago by
 Commit changed from 3a2555bd253634c9cee57a146ae81c1d703ff829 to 076a8b5ceb7df52d62bccc09ed71a3bb099e248d
Branch pushed to git repo; I updated commit sha1. New commits:
076a8b5  package type and source repo

comment:5 in reply to: ↑ 3 Changed 4 years ago by
Replying to vbraun:
This branch doesn't make it standard, we should have a formal vote for that as well I guess.
fixed and called up for vote...
A spkgcheck script that runs runalltests would be nice.
but that's what spkgcheck already does, no?
#!/usr/bin/env bash if [ "$SAGE_LOCAL" = "" ]; then echo >&2 "SAGE_LOCAL undefined ... exiting"; echo >&2 "Maybe run 'sage sh'?" exit 1 fi cd nauty* # runalltests doesn't exit with a zero status. # So we check $fails instead. $MAKE checks source ./runalltests if [ "${fails}" ne "0" ]; then echo >&2 "Error checking nauty." exit 1 fi
comment:6 followup: ↓ 11 Changed 4 years ago by
 Status changed from needs_review to needs_info
Why fork nauty?
comment:7 followup: ↓ 8 Changed 4 years ago by
And where does this version "26b17" come from?
comment:8 in reply to: ↑ 7 ; followup: ↓ 10 Changed 4 years ago by
Replying to jdemeyer:
And where does this version "26b17" come from?
https://github.com/dimpase/nauty_sagemath/blob/master/This_is_nauty26b17.
so this is still a beta, but we'll update as soon as we get a release
comment:9 Changed 4 years ago by
Typo: distrubting
comment:10 in reply to: ↑ 8 ; followup: ↓ 12 Changed 4 years ago by
Replying to dimpase:
https://github.com/dimpase/nauty_sagemath/blob/master/This_is_nauty26b17
404 page not found...
comment:11 in reply to: ↑ 6 ; followups: ↓ 13 ↓ 14 Changed 4 years ago by
Replying to jdemeyer:
Why fork nauty?
cause that's what Brendan would like  he want to keep releasing with his original license, I suppose.
comment:12 in reply to: ↑ 10 Changed 4 years ago by
Replying to jdemeyer:
Replying to dimpase:
https://github.com/dimpase/nauty_sagemath/blob/master/This_is_nauty26b17.
404 page not found...
the dot at the end is part of the URL
comment:13 in reply to: ↑ 11 ; followup: ↓ 15 Changed 4 years ago by
Replying to dimpase:
Replying to jdemeyer:
Why fork nauty?
cause that's what Brendan would like  he want to keep releasing with his original license, I suppose.
I don't think that justifies forking nauty. Why not just ship the official tarball and add Brendan's permission as a separate file in the repo (say, build/pkgs/nauty/LICENSE
).
comment:14 in reply to: ↑ 11 ; followup: ↓ 16 Changed 4 years ago by
Replying to dimpase:
he want to keep releasing with his original license, I suppose.
For the record, does he know that is completely pointless since the nautyinSage gives everybody permission to copy the code under the weaker license?
comment:15 in reply to: ↑ 13 ; followup: ↓ 17 Changed 4 years ago by
Replying to jdemeyer:
Replying to dimpase:
Replying to jdemeyer:
Why fork nauty?
cause that's what Brendan would like  he want to keep releasing with his original license, I suppose.
I don't think that justifies forking nauty. Why not just ship the official tarball and add Brendan's permission as a separate file in the repo (say,
build/pkgs/nauty/LICENSE
).
there is no official tarball of this version available (we got the tarball by email), and the licence is in a .in file to be run with the tarball.
comment:16 in reply to: ↑ 14 Changed 4 years ago by
Replying to jdemeyer:
Replying to dimpase:
he want to keep releasing with his original license, I suppose.
For the record, does he know that is completely pointless since the nautyinSage gives everybody permission to copy the code under the weaker license?
I don't read his mind, but I imagine that a professor of CS in one of the best universities in Australia knows what he is doing.
comment:17 in reply to: ↑ 15 ; followup: ↓ 19 Changed 4 years ago by
Replying to dimpase:
there is no official tarball of this version available (we got the tarball by email)
That's a bit fishy. Can't you use the official tarball (+ patches if needed)?
comment:18 Changed 4 years ago by
 Commit changed from 076a8b5ceb7df52d62bccc09ed71a3bb099e248d to 400bc1114bf6a21f421534a7f40c52a395c9cbdf
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
400bc11  package type and source repo, and a typo fix

comment:19 in reply to: ↑ 17 ; followup: ↓ 21 Changed 4 years ago by
Replying to jdemeyer:
Replying to dimpase:
there is no official tarball of this version available (we got the tarball by email)
That's a bit fishy. Can't you use the official tarball (+ patches if needed)?
Now the gihub repo has the 26b17 updates over the version 25r9 we have (and which is the current publicly available version) You can take a look at a long diff...
comment:20 Changed 4 years ago by
 Commit changed from 400bc1114bf6a21f421534a7f40c52a395c9cbdf to 2a690f8d9ad22a4483d871ace7ec8b57ee2764f8
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2a690f8  package type and source repo, and a typo fix

comment:21 in reply to: ↑ 19 ; followup: ↓ 22 Changed 4 years ago by
Replying to dimpase:
Now the gihub repo has the 26b17 updates over the version 25r9 we have (and which is the current publicly available version) You can take a look at a long diff...
I've tried. The relevant diffs would be between 26b17 and your repository, but I can't figure out how to do that in github (the main problem is that 26b17 doesn't seem to be available in your repository).
I think the reason why Jeroen is a little hesitant of using a fork of nauty (it's certainly why I am) is maintainability. What process for upgrading to the next version of nauty do you propose?
The usual way is to start with "pristine" upstream and patch that. It may be doable to do this with git instead, but in that case I think the procedure would be to rebase your changes to 26b17, not merge 26b17 into your branch (which seems to be what happened now). In that case you'd have to ship your (limited) git history in the tarball as a certificate of what changes have been made relative to upstream, and to give maintainers in the future the means to perform an upgrade in a relatively automated fashion.
comment:22 in reply to: ↑ 21 Changed 4 years ago by
Replying to nbruin:
Replying to dimpase:
Now the gihub repo has the 26b17 updates over the version 25r9 we have (and which is the current publicly available version) You can take a look at a long diff...
I've tried. The relevant diffs would be between 26b17 and your repository, but I can't figure out how to do that in github (the main problem is that 26b17 doesn't seem to be available in your repository).
26b17 is the 2nd commit, 25r9 is the 1st commit.
Diff is too big to be viewed online, thus check the repo out and do git diff HEAD^
I'll mention this in SPKG.txt
.
I think the reason why Jeroen is a little hesitant of using a fork of nauty (it's certainly why I am) is maintainability. What process for upgrading to the next version of nauty do you propose?
the relevant license file is in nautyh.in
.
Just make sure that whenever you pour over the next nauty release, the relevant part of the file containing the license stays intact.
Suggestions how to make this foolproof are welcome. But releases are not frequent at all, so I am not worried.
The usual way is to start with "pristine" upstream and patch that. It may be doable to do this with git instead, but in that case I think the procedure would be to rebase your changes to 26b17, not merge 26b17 into your branch (which seems to be what happened now). In that case you'd have to ship your (limited) git history in the tarball as a certificate of what changes have been made relative to upstream, and to give maintainers in the future the means to perform an upgrade in a relatively automated fashion.
there is no "my" branch. The repo is the released version 25r9 patched by 26b17.
comment:23 Changed 4 years ago by
The git repo in question should be in https://github.com/sagemath and not in https://github.com/dimpase
If we agree that this should go forward at all then I'll do this.
comment:24 Changed 4 years ago by
 Commit changed from 2a690f8d9ad22a4483d871ace7ec8b57ee2764f8 to 5b17a29fa79743c1eaa6548332a20a2f4a3efe77
comment:25 Changed 4 years ago by
 Description modified (diff)
 Status changed from needs_info to needs_review
good to go, I think.
comment:26 Changed 4 years ago by
 Description modified (diff)
comment:27 followup: ↓ 29 Changed 4 years ago by
Should you install the new tools from this version? May not be useful in sage but as a general matter unless there is a reason I think we should install everything. This is my list
copyg listg labelg dretog amtog geng complg showg NRswitchg \ biplabg addedgeg deledgeg countg pickg genrang newedgeg catg genbg \ directg gentreeg genquarticg \ ranlabg multig planarg gentourng linegraphg watercluster2 dretodot \ subdivideg vcolg delptg cubhamg twohamg hamheuristic converseg \ genspecialg genbgL \ copyg listg labelg dretog amtog geng complg showg NRswitchg \ dreadnaut
I'll sort it at some point.
comment:28 Changed 4 years ago by
 Status changed from needs_review to needs_work
 Work issues set to remove "#optional  nauty" tags
comment:29 in reply to: ↑ 27 Changed 4 years ago by
Replying to fbissey:
Should you install the new tools from this version? May not be useful in sage but as a general matter unless there is a reason I think we should install everything. This is my list
copyg listg labelg dretog amtog geng complg showg NRswitchg \ biplabg addedgeg deledgeg countg pickg genrang newedgeg catg genbg \ directg gentreeg genquarticg \ ranlabg multig planarg gentourng linegraphg watercluster2 dretodot \ subdivideg vcolg delptg cubhamg twohamg hamheuristic converseg \ genspecialg genbgL \ copyg listg labelg dretog amtog geng complg showg NRswitchg \ dreadnautI'll sort it at some point.
it's quite a handful; I should have read the announcement more carefully.
comment:30 Changed 4 years ago by
I had to go to the makefile to make a proper list. And it looks like I got duplicates in the previous one so care is needed there.
all : nauty gtools ; nauty : dreadnaut nauty.a nauty1.a nautyW.a nautyW1.a nautyL.a nautyL1.a; gtools : copyg listg labelg dretog amtog geng complg showg NRswitchg \ biplabg addedgeg deledgeg countg pickg genrang newedgeg catg genbg \ directg gentreeg genquarticg \ ranlabg multig planarg gentourng linegraphg watercluster2 dretodot \ subdivideg vcolg delptg cubhamg twohamg hamheuristic converseg \ genspecialg genbgL shortg ;
comment:31 Changed 4 years ago by
 Commit changed from 5b17a29fa79743c1eaa6548332a20a2f4a3efe77 to a64cc23fa0ead449d41b6c158bc98d922df75eb5
comment:32 Changed 4 years ago by
 Status changed from needs_work to needs_review
 Work issues remove "#optional  nauty" tags deleted
OK, all fixed, thanks for pointing these issues out.
comment:33 Changed 4 years ago by
 Reviewers set to François Bissey
Apart from the issue to moving to standard it is all ok from me, I am ready to put in positive review if we have a clear signal on "standard".
comment:34 Changed 4 years ago by
 Status changed from needs_review to needs_work
 Work issues set to HTTP 404
Not Found The requested URL /~coml0531/sage/nauty26r1.tar.bz2 was not found on this server.
comment:35 Changed 4 years ago by
One detail: you don't need to copy the whole license to SPKG.txt
, just a link should be sufficient, perhaps mention that it is GPLcompatible.
comment:36 Changed 4 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
oops, wrong suffix in the URL. Fixed now.
comment:37 Changed 4 years ago by
 Status changed from needs_review to needs_work
 Work issues HTTP 404 deleted
In src/sage/graphs/graph_generators.py
:
Also: see the use of the optional nauty package for generating graphs at the :meth:`nauty_geng` method.
comment:38 Changed 4 years ago by
Author name...
comment:39 Changed 4 years ago by
 Commit changed from a64cc23fa0ead449d41b6c158bc98d922df75eb5 to 74d5ab954bcfe870db8b588a2309db1dd27e6f50
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
a5536bb  Merge branch 'develop' of trac.sagemath.org:sage into develop

a1b0953  Merge branch 'develop' of trac.sagemath.org:sage into develop

0c0e046  Merge branch 'develop' of trac.sagemath.org:sage into develop

39e8d33  Merge branch 'develop' of trac.sagemath.org:sage into develop

a3e0d4c  Merge branch 'develop' of trac.sagemath.org:sage into develop

3c9e0aa  Merge branch 'develop' of trac.sagemath.org:sage into develop

97723cf  Merge branch 'develop' of trac.sagemath.org:sage into develop

c1bcd55  Merge branch 'develop' of trac.sagemath.org:sage into develop

96156e8  Merge remotetracking branch 'trac/public/nauty26' into nau

74d5ab9  addressed the reviewer's questions

comment:40 Changed 4 years ago by
comment:41 Changed 4 years ago by
 Commit changed from 74d5ab954bcfe870db8b588a2309db1dd27e6f50 to 56815d4da72ff750813797d9158a82b1de9cd199
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
56815d4  addressed the reviewer's questions

comment:42 Changed 4 years ago by
 Status changed from needs_work to needs_review
OK, done (and then fixed spurrious merge commits that were not needed...)
comment:43 Changed 4 years ago by
 Reviewers changed from François Bissey to François Bissey, Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:44 Changed 4 years ago by
 Branch changed from public/nauty26 to 56815d4da72ff750813797d9158a82b1de9cd199
 Resolution set to fixed
 Status changed from positive_review to closed
This branch doesn't make it standard, we should have a formal vote for that as well I guess.
A spkgcheck script that runs runalltests would be nice.