Opened 6 years ago

Closed 6 years ago

#18425 closed enhancement (fixed)

new-style Nauty package

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.7
Component: packages: optional Keywords:
Cc: tmonteil, vdelecroix, dcoudert, azi, borassi Merged in:
Authors: François Bissey, Thierry Monteil Reviewers: Thierry Monteil, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 42404ed (Commits, GitHub, GitLab) Commit: 42404ed0991dbc67bab2eeaf80925c02204c61fb
Dependencies: Stopgaps:

Status badges

Description (last modified by fbissey)

New style spkg for Nauty. The code has been updated to the latest upstream version available.

Renamed upstream tarball http://www.lmona.de/files/sage/nauty-25r9.tar.gz

Change History (56)

comment:1 Changed 6 years ago by ncohen

  • Branch set to u/ncohen/18425
  • Commit set to c2d4c898e6cfab4e1e6f8cf88eb11d39d9ebbbb1
  • Status changed from new to needs_review

New commits:

c2d4c89trac #18425: new-style Nauty package

comment:2 Changed 6 years ago by tmonteil

I would prefer us to ship the unmodified upstream tarball which can be found at http://cs.anu.edu.au/~bdm/nauty/nauty25r9.tar.gz since the gain in the recompression is less than 100K, see for example #17529. I will upload a patch while ptestlong is finishing.

comment:3 Changed 6 years ago by tmonteil

  • Branch changed from u/ncohen/18425 to u/tmonteil/18425

comment:4 Changed 6 years ago by git

  • Commit changed from c2d4c898e6cfab4e1e6f8cf88eb11d39d9ebbbb1 to e23db009d555c3cb3019e560ccd67bc4f5dd32de

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

247b6a6#18425 : cd into the correct source directory.
e23db00#18425 tag doctests involving nauty.

comment:5 Changed 6 years ago by tmonteil

  • Reviewers set to Thierry Monteil

I modified the install script to allow building with an unmodified tarball. Also, the # optional - nauty flag should be applied on a whole doctest, to allow sage -t --optional=nauty work (my computer is currently very slow hence i can not run with sage -t --optional=sage,nauty right now, only test nauty-specific doctests).

Tell if it is OK like that.

comment:6 Changed 6 years ago by git

  • Commit changed from e23db009d555c3cb3019e560ccd67bc4f5dd32de to 57c992259a44fcd7de1889d8626b026424faa9ab

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

57c9922#18425 : remove old gtools-h.in patch.

comment:7 follow-up: Changed 6 years ago by tmonteil

According to the mercurial history of nauty-25.spkg, the "patch" (actually a file to overwrite the existing one) gtools-h.in was introduced in 2010 for version 2.4 of nauty in order "to compile on Ubuntu and other systems". Inbetween, the file gtools-h.in has evolved, so keeping it does not let us benefit from the evolution, hence i propose to simply remove it and test to see if some problem appears on some machines.

Note that nauty builds and passes tests without the patch.

comment:8 Changed 6 years ago by git

  • Commit changed from 57c992259a44fcd7de1889d8626b026424faa9ab to 34a829c83d7f1014daf1f74c0e05cecbf9839354

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

34a829c#18425 : add a self-check file since some self-tests are available.

comment:9 follow-up: Changed 6 years ago by ncohen

  • Branch changed from u/tmonteil/18425 to u/ncohen/18425
  • Commit changed from 34a829c83d7f1014daf1f74c0e05cecbf9839354 to c2d4c898e6cfab4e1e6f8cf88eb11d39d9ebbbb1

I disagree with the change you made to keep the original tarball. I initially did the same, then decided against it when I noticed the directory name problem that you avoided with:

+NAUTY_VERSION="$(cat ${SAGE_ROOT}/build/pkgs/nauty/package-version.txt)"
+SRC="nauty${NAUTY_VERSION}"

I do not think that it is a good idea to do this, as we could very well imagine that other scripts be written that apply to the packages, and those would expect a correctly named package.

Furthermore, I consider it very bad manners to go on somebody else's ticket, change the branch name and change the commits without even getting the ticket's author's agreement. Please do not do this again.

I set the ticket to its original content. If you have requests to make as a reviewer, please do.

Nathann

comment:10 in reply to: ↑ 9 ; follow-up: Changed 6 years ago by tmonteil

Replying to ncohen:

Furthermore, I consider it very bad manners to go on somebody else's ticket, change the branch name and change the commits without even getting the ticket's author's agreement.

What does "Tell if it is OK like that." mean ?

I don't understand your point at all. I did not remove your branch, i just propose some modifications for discussion. I guess in that case it is easier to propose changes that people could download to test (e.g. does nauty still work without the old patch), than asking the author of the previous commits to remove the patch themselves so that i can download their changes to see what happens. Same with requesting a spkg-check, then have to inspect it to filnally request some output parsing because it may fail silently if coded as the other similar files.

I understand that trac Branch field is not well designed since it does not allow to show various branches, but i do not see what can i do else than git trac push to show the proposed modifications on the ticket.

More generally, i do not understand why the people that opens a ticket owns it, Sage is a common goods that people try to improve collectively, not a staked territory. If you want to own a ticket, please write it explicitely by fillind the related field, i will not bother you on those.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 6 years ago by ncohen

I don't understand your point at all.

If you want to propose a commit as a reviewer, please push them on a new branch. Add a comment to tell the other guy what you did and where to find the branch, and that he can add them to the ticket's branch if he agrees.

More generally, i do not understand why the people that opens a ticket owns it,

Just picture several guys talking around a table. Guy '1' has an idea, and begins to explain it. Midway, you tell him to shut up and you explain to the others what you think that his idea is. That's how I see it.

If you want to own a ticket, please write it explicitely by fillind the related field, i will not bother you on those.

I did. The branch is named u/ncohen/18425, to which you have no write access. When I create branches to which anybody is welcome to contribute (e.g. reorganization of the doc, which cover all domains at once), I use a branch named public/something.

Nathann

comment:12 Changed 6 years ago by fbissey

Can someone explain to me why all the installed programs have been prefixed with nauty-. What's the big idea? Does it clash with something? If you cannot justify it, I will remove it and patch every place that call a nauty-* program (and I already know where those are) as a review.

comment:13 Changed 6 years ago by ncohen

probably because the executables' names are not very meaningful if you don't know that they are nauty routines? I don't see any objection to the removal of that 'nauty-' in front of them, but then they should probably be in a nauty subfolder.

Nathann

comment:14 follow-up: Changed 6 years ago by fbissey

Very little is meaningful if you don't know what it is in advance on a unix system. I don't see why nauty routines should have a special treatment.

comment:15 in reply to: ↑ 14 Changed 6 years ago by ncohen

Very little is meaningful if you don't know what it is in advance on a unix system. I don't see why nauty routines should have a special treatment.

Sooo... your goal is to lose information for the sake of consistency?

comment:16 Changed 6 years ago by fbissey

There are two points:

1) if you would like the shipped binaries to be prefixed like other nice software like sage, complain upstream 2) if the information you are worried about is the content of the nauty package, its binaries in particular, that's a defect of the sage packaging system that it doesn't have a database of the files installed by its various packages. This is a more advanced feature that has always been out of scope.

And about consistency, it is actually a good thing. Suppose that someone has some program or script that uses nauty and wants to use them in sage. I think they will be surprised when their scripts/programs don't work out of the box after installing sage's nauty package. It may actually take them time to figure out that they have to prefix everything.

Adding a prefix specifically in sage goes against the principle of least surprise because you modify what is expected from upstream. The only case where it is justified is a clash in name (in which case it is proper to involve both upstreams if possible).

comment:17 Changed 6 years ago by fbissey

And unfortunately for my last argument because nauty has been in sage prefixed for so long, we may have old user expecting it prefixed. So now we need to have both prefixed and non prefixed binaries. At the minimum the prefixed one should link to the non-prefixed ones. Better would be to have the prefixed one link to a wrapper that would execute the appropriate binary and output a deprecation message. But we shouldn't use prefixed version in sage in the future.

comment:18 Changed 6 years ago by ncohen

I still believe that a nauty/ folder would respect the original filenames wihout being less informative. If you must do these changes, however, please open a ticket depending on this one.

I only want to make nauty a new-style package because Thierry needed one.

Nathann

comment:19 Changed 6 years ago by fbissey

I accept to do it in a separate ticket once this is one done.

comment:20 in reply to: ↑ 7 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Use patch to patch packages instead of copying files.

comment:21 Changed 6 years ago by jdemeyer

Also, the license should be clarified: it's not just a military restriction:

*   Nauty is copyright (1984-2013) Brendan McKay.  All rights reserved.      *
*   Permission 
*   is hereby given for use and/or distribution with the exception of        *
*   sale for profit or application with nontrivial military significance.    *
*   You must not remove this copyright notice, and you must document any     *
*   changes that you make to this program.                                   *
*   This software is subject to this copyright only, irrespective of         *
*   any copyright attached to any package of which this is a part.           *
*                                                                            *
*   This program is only provided "as is".  No responsibility will be taken  *
*   by the author, his employer or his pet rabbit* for any misfortune which  *
*   befalls you because of its use.  I don't think it will delete all your   *
*   files, burn down your computer room or turn your children against you,   *
*   but if it does: stiff cheddar.  On the other hand, I very much welcome   *
*   bug reports, or at least I would if there were any bugs.                 *
*                                                       * RIP, 1989          *
*   Traces is copyright Adolfo Piperno (2011-).                              *

comment:22 Changed 6 years ago by jdemeyer

Use $MAKE instead of make.

comment:23 Changed 6 years ago by jdemeyer

I also agree that the prefixing is bad: just install stuff as-is in $SAGE_LOCAL/bin.

comment:24 Changed 6 years ago by jdemeyer

Then a final minor thing: it's best if you use indents of 4 spaces also in shell scripts.

comment:25 Changed 6 years ago by git

  • Commit changed from c2d4c898e6cfab4e1e6f8cf88eb11d39d9ebbbb1 to eb3432b3864ebc39f5a0522e3d9af7f57f951034

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

cdb6e6atrac #18425: Merged with 6.7
eb3432btrac #18425: License

comment:26 follow-up: Changed 6 years ago by ncohen

  • Status changed from needs_work to needs_review

I added a commit that updates the "license" section of SPKG.txt in a more faithful way.

This being said, I opened this ticket to turn the old spkg into a new-style package, and this is what it does: nothing more. If you have style issues related to tabulations, $MAKE instead of make or the name of packages I obviously cannot blame you, but please do not abuse your position as a reviewer to make me implement them for you.

This ticket addresses and problem and fixes it. If you see something wrong which I did not introduce, add a commit or open a ticket.

Nathann

Last edited 6 years ago by ncohen (previous) (diff)

comment:27 in reply to: ↑ 26 ; follow-up: Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_info

These two statements are inconsistent:

I opened this ticket to turn the old spkg into a new-style package, and this is what it does: nothing more.

The code is also updated to the latest release.

Now which one is true? (if the second is true, I certainly have some right to complain about spkg-install, especially the patching)

comment:28 in reply to: ↑ 27 Changed 6 years ago by ncohen

Now which one is true?

The tarball contains the latest release. That does not involve anything related to the branch. The patching is what the current spkg does.

(if the second is true, I certainly have some right to complain about spkg-install, especially the patching)

You certainly have the right to complain. You can also add a commit if that bothers you.

Nathann

comment:29 Changed 6 years ago by ncohen

  • Status changed from needs_info to needs_review

comment:30 in reply to: ↑ 11 Changed 6 years ago by tmonteil

Replying to ncohen:

I don't understand your point at all.

If you want to propose a commit as a reviewer, please push them on a new branch.

This is precisely what i did, i created u/tmonteil/18425. I did not touch the branch u/ncohen/18425, it is still here, unmodified.

Add a comment to tell the other guy what you did and where to find the branch, and that he can add them to the ticket's branch if he agrees.

Again, which word didn't you understand in "Tell if it is OK like that." ?

I still do not get your point about detached branches, there is zero benefits from detached branches over attached ones provided by git trac push. The git trac push command pushes the branch to git and let trac know about this, this is why both git and trac are on the same host, so that there is an additional information that links a branch to a particular ticket. The branch u/tmonteil/18425 is about this ticket, so git trac push let trac know about it. In particular, the commits and their description will appear on the trac ticket as cliquable links so that people regisered on the ticket will know about it, and anyone can easily browse them. Please read http://doc.sagemath.org/html/en/developer/git_trac.html

More generally, i do not understand why the people that opens a ticket owns it,

Just picture several guys talking around a table. Guy '1' has an idea, and begins to explain it. Midway, you tell him to shut up and you explain to the others what you think that his idea is. That's how I see it.

I am very sorry about your interpretation, here is how things happened actually:

  • we were discussing the opportunity to remove the old-stlye bitrotting spkgs, and the difficulty to know which of them could be safely removed, and i was mentioning that nauty was the only one that i still use to build Sage Debian Live,
  • you asked me to send you nauty-25.spkg because you could not find it elsewhere,
  • i looked for it, and find it on the remote server that was used to build SDL and sent you the file,
  • you copy/paste its content into a commit on this ticket without looking further, set the ticket to needs_review and added myself and four other people in CC.

So, what picture talking around a table should i interpret with what happened ?

  1. you put a raw copy of an old unmaintained spkg on the git to move the process from discussion to action, and added all of us in CC so that we could work together on the migration, since Sage has some coding standards that were not satisfied by the bitrotting sage-25.spkg.
  1. you did not read the developer manual and are not able to write packages stasfying medium standards, you do not care if custom files coming from an older version of the package (2.4) overwrite the newer ones, you do not care to implement self-tests while they exist, you consider the raw copy/paste as a perfectly valid commit ready for review.
  1. you knows all those standards very well, but prefers the other to do all the checks during the review, but at the same time require them not to implement the changes on the ticket, but on an hidden branch.

I am very sorry that i interpreted what happened in the first constructive collective way and could not even think of some other possibilities, i will be far more careful next time and think that 2. or 3. could happen, but then my comment would have been "please do not waste reviewer's time by letting far-below-standards packaging into needs_rewiew". Or perhaps should i open a new ticket with the same purpose and a cleaner package so that the territories are respected ?

comment:31 Changed 6 years ago by ncohen

  • Authors Nathann Cohen deleted
  • Branch u/ncohen/18425 deleted
  • Commit eb3432b3864ebc39f5a0522e3d9af7f57f951034 deleted
  • Status changed from needs_review to needs_work

Okay guys. This ticket is yours, I am out. I just wanted to help, but this is getting very out-of-hands for an initially straightforward change.

Nathann

comment:32 Changed 6 years ago by fbissey

OK, Nathann took his marbles (literally) and walked away. While the package obviously needed some love I was partisan not to alienate him. But never mind I will re-start off from Thierry's branch.

comment:33 follow-up: Changed 6 years ago by fbissey

  • Authors set to François Bissey
  • Branch set to u/fbissey/trac18425
  • Commit set to e8ead70b82394d24adf01a6c1b8f96a04d53795e
  • Status changed from needs_work to needs_review

New branch but same tarball. Upstream tarball unfortunately has the bad taste of not having a - between the name and the version number which confuses sage-fix-pkg-cheksums. So I used the repackaged tarball. I am quite open to improvement in this area. I used $MAKE and removed the prefix from the binaries. Inserted a few comments in spkg-install and checked it passed its own tests.


New commits:

d65e799Initial import of nauty as a new style spkg
e8ead70removing the nauty- prefix from the calls to nauty binaries.

comment:34 follow-up: Changed 6 years ago by tmonteil

  • Authors changed from François Bissey to François Bissey, Thierry Monteil
  • Status changed from needs_review to needs_work

I don't know how you did to import my branch (e.g. self-check), but you forgot commit e23db009d555c3cb3019e560ccd67bc4f5dd32de that adds a missing # optional - nauty so that we can do sage -t --optional=nauty.

Concerning upstream tarball, the less we touch upstream source, the best it is. Here, we have the possibility to keep it intact (as done in 247b6a6d17aa9db788e73ab27f5966f3c08e0666). It is much less intrusive to just rename nauty25r9.tar.gz into nauty-25r9.tar.gz (the file itself is untouched and we get the same hash than upstream) than to unpack it, change internal directory, and recompress it (which should require an additional spkg-src). See http://doc.sagemath.org/html/en/developer/packaging.html#modified-tarballs

comment:35 in reply to: ↑ 34 Changed 6 years ago by jdemeyer

Replying to tmonteil:

so that we can do sage -t --optional=nauty.

Why would you want to do that? I disagree with adding # optional tags unless strictly required.

comment:36 in reply to: ↑ 33 Changed 6 years ago by jdemeyer

Replying to fbissey:

I used $MAKE

You should do this also in spkg-check.

comment:37 Changed 6 years ago by fbissey

You are right about the re-packing. It would need a spkg-src, a renaming probably can just be a mention in SPKG.txt.

I cannot believe I left a make out there. I guess I was annoyed by something else will I was finishing the branch.

comment:38 Changed 6 years ago by jdemeyer

Shell tip: instead of running runalltests and parsing the output, you can instead source ./runalltests and just read the variable $fails.

comment:39 Changed 6 years ago by jdemeyer

And in spkg-check, this can be removed:

echo "Tests suite for nauty passed without errors."
exit 0

comment:40 Changed 6 years ago by jdemeyer

For installing, I would use cp -p instead of plain cp.

comment:41 follow-up: Changed 6 years ago by fbissey

@tmontiel I did some copying to rebase stuff on top of 6.7 quick and dirty and I completely missed that you touched other files. I am not quite sure how to see the details of that commit from the command line.

@jeroen I'll have a quick look to see if I can add an install target, that would care of the permissions.

comment:42 Changed 6 years ago by jdemeyer

That's all my comments for now. If you make the edits, I will do a final review.

And I don't care much about renaming/repacking/changing the tarball, just document what you did.

comment:43 in reply to: ↑ 41 Changed 6 years ago by jdemeyer

Replying to fbissey:

@jeroen I'll have a quick look to see if I can add an install target, that would care of the permissions.

I don't think that's needed, just copying with cp -p in spkg-install should work.

comment:44 follow-up: Changed 6 years ago by fbissey

And should I do something to put it in conformance with #18431?

comment:45 in reply to: ↑ 44 Changed 6 years ago by jdemeyer

Replying to fbissey:

And should I do something to put it in conformance with #18431?

Ah yes, I think you need to add a file build/pkgs/nauty/type containing optional (although this might not be strictly required)

comment:46 Changed 6 years ago by git

  • Commit changed from e8ead70b82394d24adf01a6c1b8f96a04d53795e to 0d4e4207a30ab51b83b7fdf9968e1a7385508d9f

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

0d4e420Incorporated all of Jeroen's comment. Reverted to a renamed upstream tarball and documented packaging.

comment:47 Changed 6 years ago by fbissey

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:48 Changed 6 years ago by jdemeyer

Replace the arcane cd $SRC stuff by a much simpler cd nauty*.

And $SAGE_LOCAL -> "$SAGE_LOCAL" in shell scripts.

comment:49 Changed 6 years ago by jdemeyer

And obviously, this comment needs updating:

# Unfortunately, runalltests script does not exit with a nonzero status if some
# tests failed, so we parse the last line of the log to decide whether all
# tests passed or not, in order to exit with the correct status.

comment:50 Changed 6 years ago by jdemeyer

  • Reviewers changed from Thierry Monteil to Thierry Monteil, Jeroen Demeyer
  • Status changed from needs_review to needs_work

comment:51 follow-up: Changed 6 years ago by git

  • Commit changed from 0d4e4207a30ab51b83b7fdf9968e1a7385508d9f to 032648d0008a1f81181f9c5900e4e546a1f83a95

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

032648dFurther simplification and sliming

comment:52 in reply to: ↑ 51 Changed 6 years ago by jdemeyer

Replying to git:

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

032648dFurther simplification and sliming

sliming???

comment:53 Changed 6 years ago by git

  • Commit changed from 032648d0008a1f81181f9c5900e4e546a1f83a95 to 42404ed0991dbc67bab2eeaf80925c02204c61fb

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

42404edPut some comments back in spkg-check

comment:54 Changed 6 years ago by fbissey

  • Status changed from needs_work to needs_review

I'd say that was too much sliming :) - OK I'll remember that should be slimming for next time (may be, I have always been terrible at spelling).

I didn't know you could do the cd nauty* thing.


New commits:

42404edPut some comments back in spkg-check

comment:55 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:56 Changed 6 years ago by vbraun

  • Branch changed from u/fbissey/trac18425 to 42404ed0991dbc67bab2eeaf80925c02204c61fb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.