Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Doctest failure with nauty-gentreeg #34133

Open
mkoeppe opened this issue Jul 8, 2022 · 14 comments
Open

Doctest failure with nauty-gentreeg #34133

mkoeppe opened this issue Jul 8, 2022 · 14 comments
Labels
c: graph theory p: major / 3 t: bug

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Jul 8, 2022

with system nauty 2.6r7+ds-1 on debian-stretch (https://github.com/sagemath/sage/runs/7243040063?check_suite_focus=true)

The interface and doctest were added in #33670

CC: @dcoudert @dimpase

Component: graph theory

Issue created by migration from https://trac.sagemath.org/ticket/34133
@mkoeppe mkoeppe added this to the sage-9.7 milestone Jul 8, 2022
@mkoeppe

This comment has been minimized.

Sign in to view
@mkoeppe

This comment has been minimized.

Sign in to view
@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 8, 2022

comment:2

No failure on debian-buster-standard with system nauty 2.6r10+ds-1

@dcoudert
Copy link
Contributor

dcoudert commented Jul 9, 2022

comment:3

I have no clue what's happening here.

@dimpase
Copy link
Member

dimpase commented Jul 9, 2022

comment:4

debian stretch is past EOL (and past LTS support too). Are we dropping it?
https://wiki.debian.org/LTS

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 9, 2022

comment:5

Replying to @dcoudert:

I have no clue what's happening here.

This system can be inspected using docker run -it docker.pkg.github.com/sagemath/sage/sage-docker-debian-stretch-standard-with-targets-optional:9.7.beta4 ./sage

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 9, 2022

comment:6

Replying to @dimpase:

debian stretch is past EOL (and past LTS support too). Are we dropping it?
https://wiki.debian.org/LTS

I don't think there's any urgency to drop it.
I'd prefer that we detect bad nauty versions in configure

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Sep 19, 2022
@mkoeppe mkoeppe removed this from the sage-9.8 milestone Jan 29, 2023
@mkoeppe mkoeppe mentioned this issue Feb 18, 2023
Closed
2 tasks
@dcoudert
Copy link
Contributor

This issue is similar to #35157. The only solution I see is to be more restrictive on version numbers.

@GMS103
Copy link
Member

GMS103 commented Feb 20, 2023

This issue is similar to #35157. The only solution I see is to be more restrictive on version numbers.

How can that be done? I have tried modifying $SAGE_ROOT/build/pkgs/nauty/spkg-configure.m4 (which seems a bit old) using PKG_CHECK_MODULES with >= and < without success (but then I am only a novice).

@dcoudert
Copy link
Contributor

I don't know how to modify this file. May be Matthias can help ?

@GMS103
Copy link
Member

GMS103 commented Mar 8, 2023

Thanks to Dima, Homebrew's nauty has been patched and now it works (but the test files must be updated).

@GMS103
Copy link
Member

GMS103 commented Mar 8, 2023

For other people arriving here, I quote Dima:

Actually, one can patch 2.8.6, as reported by upstream: https://mailman.anu.edu.au/pipermail/nauty/2023-January.txt

@tornaria tornaria mentioned this issue Mar 8, 2023
Merged
2 tasks
@tornaria
Copy link
Contributor

tornaria commented Mar 8, 2023

This seems a dupe of #35157. The doctest failures for a patched nauty 2.8.6 are addressed by #35250 but checking system nauty for the bug at configure time is still open (I won't work on that since I'm no longer using sage-the-distro).

vbraun pushed a commit that referenced this issue Apr 1, 2023
gh-35250: Fix doctests for nauty 2.8.6 output changes
dc05f71 
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes #1234" use "Introduce new method to
calculate 1+1"
-->
### 📚 Description

See #35157 and #34133.

This changes doctests to accomodate to changes in nauty output (which
are only used for debug purposes).

OTOH, there is a bug in the released nauty 2.8.6 which causes another
doctest failure reported in #35157. On a system with patched nauty 2.8.6
the current PR is good enough.

Leave #35157 open. To fix it a good option might be running `gentreeg 2`
at configure time and check the bug is not present so it's possible to
use 2.8.6 from system if it's been patched. In case it's useful, the
patch I'm using is https://gitweb.gentoo.org/repo/gentoo.git/plain/sci-
mathematics/nauty/files/nauty-2.8.6-gentreeg-gentourng.patch

<!-- Describe your changes here in detail -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If it resolves an open issue, please link to the issue here. For
example "Closes #1337" -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

- [x] I have made sure that the title is self-explanatory and the
description concisely explains the PR.
- [x] I have linked an issue or discussion.

### ⌛ Dependencies
<!-- List all open pull requests that this PR logically depends on -->
<!--
- #xyz: short description why this is a dependency
- #abc: ...
-->
    
URL: #35250
Reported by: Gonzalo Tornaría
Reviewer(s): David Coudert
@dcoudert
Copy link
Contributor

Do we still have this issue after the last update of nauty (#36774) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: graph theory p: major / 3 t: bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@dimpase @tornaria @mkoeppe @dcoudert @GMS103