Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#28958 closed enhancement (fixed)

spkg-configure.m4 for nauty

Reported by: mjo Owned by:
Priority: major Milestone: sage-9.1
Component: build: configure Keywords:
Cc: embray, dimpase Merged in:
Authors: Michael Orlitzky, Dima Pasechnik Reviewers: Isuru Fernando
Report Upstream: N/A Work issues:
Branch: 0c17402 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

The nauty package has no dependencies within sage, and we only communicate with it via popen() for now, so this should be relatively easy to pick up from the system. As far as I can tell, the following three files are the only places where a nauty executable is called:

  1. src/sage/graphs/digraph_generators.py (directg, gentourng)
  2. src/sage/graphs/graph_generators.py (geng)
  3. src/sage/graphs/hypergraph_generators.py (genbg)

Change History (41)

comment:1 Changed 2 years ago by mjo

  • Authors set to Michael Orlitzky
  • Branch set to u/mjo/ticket/28958
  • Commit set to c22ee36e1b42b9ed5e187f4b0db304ad4bbaffe0

New commits:

ba33bd1Trac #28958: allow tests to pass with nauty-2.7.x.
c22ee36Trac #28958: spkg-configure.m4 for nauty.

comment:2 Changed 2 years ago by mjo

  • Cc embray dimpase added
  • Status changed from new to needs_review

I've left out the header/library check because the library situation in ticket 27952 is still a mess, and nothing else uses the library right now. If this gets merged we can leave a note on that ticket that spkg-configure.m4 for nauty needs to be updated along with normaliz.

comment:3 Changed 2 years ago by isuruf

This needs a version check, otherwise the configure script might find an old nauty version. Recent versions of nauty has a -version argument which we can use for checking the nauty version.

comment:4 Changed 2 years ago by mjo

If anything, we should be doing feature tests and not version tests. Do we use any features of nauty that aren't present in a version that still exists somewhere? Debian only goes back to v2.5, but I suppose sage could be using a feature that was introduced between 2.5 and 2.6.

comment:5 Changed 2 years ago by mjo

The last time we upgraded Nauty was four years ago in #19919. That ticket didn't modify any of these source files, so the API we use hasn't changed in a meaningful way for even longer than that.

comment:6 Changed 2 years ago by isuruf

  • Reviewers set to Isuru Fernando
  • Status changed from needs_review to positive_review

comment:7 Changed 2 years ago by dimpase

This seems to be too distro-specific. Note that e.g. on Debian and Ubuntu the executabes in question get prefix nauty-, cf. https://packages.debian.org/buster/amd64/nauty/filelist

One can e.g. test for these, and install prefix-less links into $SAGE_LOCAL/bin.

Last edited 2 years ago by dimpase (previous) (diff)

comment:8 follow-ups: Changed 2 years ago by dimpase

by the way, how about writing a loop? :-)

SAGE_SPKG_CONFIGURE([nauty], [
  m4_foreach([nautyprog], [directg, gentourng, geng, genbg], [
    AC_PATH_PROG(nautyprog[_CHECK],[nautyprog])
    AS_IF([test x"[]$nautyprog[_CHECK]" = x],[sage_spkg_install_nauty=yes])
  ])
])

comment:9 in reply to: ↑ 8 Changed 2 years ago by mjo

Replying to dimpase:

by the way, how about writing a loop? :-)

AS_IF([test x"[]$nautyprog[_CHECK]" = x],[sage_spkg_install_nauty=yes])

The loop is a little more succinct, but I have no idea what that line actually does =)

Why the empty brackets before the variable name? I tried to get away with something similar, but without different variables for geng_CHECK, directg_CHECK, etc. it doesn't work. The configure script incorrectly caches the check result based on the variable name:

configure: === checking whether to install the nauty SPKG ===
checking for directg... yes
checking for gentourng... (cached) yes
checking for geng... (cached) yes
checking for genbg... (cached) yes

Since the checks are presently very simple, I don't think it's worth increasing the magic level tenfold to eliminate a few easy-to-read lines. But maybe that equation changes when the test becomes more complicated. See my next comment...

comment:11 follow-up: Changed 2 years ago by mjo

Replying to dimpase:

This seems to be too distro-specific. Note that e.g. on Debian and Ubuntu the executabes in question get prefix nauty-, cf. https://packages.debian.org/buster/amd64/nauty/filelist

One can e.g. test for these, and install prefix-less links into $SAGE_LOCAL/bin.

The usual way to handle this is to test for both names, e.g. "directg" and "nauty-directg", and then to put the resulting path into a variable like NAUTY_DIRECTG. You then tell autoconf to substitute that value into a file,

AC_OUTPUT([ ... src/sage/graphs/digraph_generators.py ])

rename that file to src/sage/graphs/digraph_generators.py.in, and finally replace the hard-coded executable name with the variable:

sub = subprocess.Popen('@NAUTY_DIRECTG@ {0}'.format(options),

It would be a little ugly to separate those file names (e.g. src/sage/graphs/digraph_generators.py) from the rest of spkg-configure.m4, since they need to go in the top-level configure.ac, but I don't foresee any other technical problems if this sounds acceptable.

comment:12 in reply to: ↑ 8 Changed 2 years ago by dimpase

Replying to dimpase:

by the way, how about writing a loop? :-)

SAGE_SPKG_CONFIGURE([nauty], [
  m4_foreach([nautyprog], [directg, gentourng, geng, genbg], [
    AC_PATH_PROG(nautyprog[_CHECK],[nautyprog])
    AS_IF([test x"[]$nautyprog[_CHECK]" = x],[sage_spkg_install_nauty=yes])
  ])
])

the bonus question is to explain quoting in AC_PATH_PROG call. I must say I don't quite know the answer.

Last edited 2 years ago by dimpase (previous) (diff)

comment:13 Changed 2 years ago by dimpase

Change the AS_IF call to to AS_IF([test x"[]$n... does work too.

comment:14 in reply to: ↑ 11 Changed 2 years ago by dimpase

Replying to mjo:

Replying to dimpase:

This seems to be too distro-specific. Note that e.g. on Debian and Ubuntu the executabes in question get prefix nauty-, cf. https://packages.debian.org/buster/amd64/nauty/filelist

One can e.g. test for these, and install prefix-less links into $SAGE_LOCAL/bin.

The usual way to handle this is to test for both names, e.g. "directg" and "nauty-directg", and then to put the resulting path into a variable like NAUTY_DIRECTG. You then tell autoconf to substitute that value into a file,

AC_OUTPUT([ ... src/sage/graphs/digraph_generators.py ])

rename that file to src/sage/graphs/digraph_generators.py.in, and finally replace the hard-coded executable name with the variable:

sub = subprocess.Popen('@NAUTY_DIRECTG@ {0}'.format(options),

It would be a little ugly to separate those file names (e.g. src/sage/graphs/digraph_generators.py) from the rest of spkg-configure.m4, since they need to go in the top-level configure.ac, but I don't foresee any other technical problems if this sounds acceptable.

There is considerable resistance by (binary) distro people to this sort of solution. I'd rather add an env. var. named like SAGE_NAUTY_BIN_PREFIX to src/bin/sage-env-config, which would be either '' or 'nauty-' and use it to compose names of executables.

comment:15 follow-up: Changed 2 years ago by dimpase

  • Status changed from positive_review to needs_work

I'm testing a branch with necessary for Debian/Fedora etc adjustments for nauty- prefix

comment:16 Changed 2 years ago by dimpase

  • Authors changed from Michael Orlitzky to Michael Orlitzky, Dima Pasechnik
  • Branch changed from u/mjo/ticket/28958 to u/dimpase/packages/nautyconfig
  • Commit changed from c22ee36e1b42b9ed5e187f4b0db304ad4bbaffe0 to 2800b65e699624ec92eb526cb2035330101fe0a2
  • Status changed from needs_work to needs_review

here it is, please have a look


New commits:

45e35cdspkg-configure for nauty
2800b65prefixes for calls to nauty routines, adjust tests

comment:17 in reply to: ↑ 15 ; follow-up: Changed 2 years ago by mjo

Replying to dimpase:

I'm testing a branch with necessary for Debian/Fedora etc adjustments for nauty- prefix

Thanks, I was busy with symmetrica all day...

There is considerable resistance by (binary) distro people to this sort of solution. I'd rather add an env. var.

Which part is objectionable? If one env var is OK, how about four env vars?

That would let us get away with only performing autoconf substitution in a single file (sage-env-config.in), but would result in a simpler implementation and would be more flexible if we ever meet a distro that uses different prefixes for different executables (e.g. if they only rename the ones that clash).

Aside from that, your version looks like it works but we wind up checking for geng twice:

configure: === checking whether to install the nauty SPKG ===
checking for geng... /usr/bin/geng
checking for directg... no
checking for gentourng... /usr/bin/gentourng
checking for geng... /usr/bin/geng
checking for genbg... /usr/bin/genbg

comment:18 in reply to: ↑ 17 Changed 2 years ago by dimpase

Replying to mjo:

Replying to dimpase:

I'm testing a branch with necessary for Debian/Fedora etc adjustments for nauty- prefix

Thanks, I was busy with symmetrica all day...

There is considerable resistance by (binary) distro people to this sort of solution. I'd rather add an env. var.

Which part is objectionable? If one env var is OK, how about four env vars?

they say they don't use ./configure to configure Sage. This means they are OK with env vars, but not OK with src/sage/foo/bar.py.in files with templates.

That would let us get away with only performing autoconf substitution in a single file (sage-env-config.in), but would result in a simpler implementation and would be more flexible if we ever meet a distro that uses different prefixes for different executables (e.g. if they only rename the ones that clash).

Aside from that, your version looks like it works but we wind up checking for geng twice:

yes, the 1st check is to figure out whether the nauty- prefix is there, or not. I can remove geng from the list from the foreach loop, but this will make the whole thing a bit more cryptic.

configure: === checking whether to install the nauty SPKG ===
checking for geng... /usr/bin/geng
checking for directg... no
checking for gentourng... /usr/bin/gentourng
checking for geng... /usr/bin/geng
checking for genbg... /usr/bin/genbg

comment:19 Changed 2 years ago by mjo

For what it's worth, here's what the spkg-configure.m4 looks like with four environment variables instead of one:

SAGE_SPKG_CONFIGURE([nauty], [

  AC_PATH_PROGS([DIRECTG_PATH],[directg nauty-directg])
  AS_IF([test x"$DIRECTG_PATH" = "x"],
        [sage_spkg_install_nauty=yes DIRECTG_PATH=directg])
  AC_SUBST(DIRECTG_PATH)

  AC_PATH_PROGS([GENTOURNG_PATH],[gentourng nauty-gentourng])
  AS_IF([test x"$GENTOURNG_PATH" = "x"],
        [sage_spkg_install_nauty=yes GENTOURNG_PATH=gentourng])
  AC_SUBST(GENTOURNG_PATH)

  AC_PATH_PROGS([GENG_PATH],[geng nauty-geng])
  AS_IF([test x"$GENG_PATH" = "x"],
        [sage_spkg_install_nauty=yes GENG_PATH=geng])
  AC_SUBST(GENG_PATH)

  AC_PATH_PROGS([GENBG_PATH],[genbg nauty-genbg])
  AS_IF([test x"$GENBG_PATH" = "x"],
        [sage_spkg_install_nauty=yes GENBG_PATH=genbg])
  AC_SUBST(GENBG_PATH)

])

The rest of the changes are straight-forward; you'd just import the paths instead of the prefix and execute them directly.

comment:20 Changed 2 years ago by dimpase

This is not complete - it does not take into account the situation where nothing in the 2nd parameter of SAGE_SPKG_CONFIGURE is run, something that happens if ./configure is run with the option --without-system-nauty.

So one needs four AC_SUBST calls in the 5th parameter of SAGE_SPKG_CONFIGURE, as well.

There is also a possibility that some of programs in question will be prefixed, and some will be not, potentially adding to confusion...

comment:21 follow-up: Changed 2 years ago by dimpase

Also, imagine people writing Sage/Python code using other nauty utilities, not covered by the list of four. With your proposal, they'd be for a nasty surprise of having to modify not only Python, but also m4 macros...

comment:22 in reply to: ↑ 21 ; follow-up: Changed 2 years ago by mjo

This one should work, although I haven't actually tried to run Sage after building it (it takes 8+ hours on my workstation):

https://git.sagemath.org/sage.git/diff?id=u/mjo/ticket/28958&id2=develop

That supports mismatched prefixes, and --with-system-nauty=no.

Replying to dimpase:

Also, imagine people writing Sage/Python code using other nauty utilities, not covered by the list of four. With your proposal, they'd be for a nasty surprise of having to modify not only Python, but also m4 macros...

Those four are the only ones used by the Sage library, which is the only thing we're testing for here (whether or not we're going to use the system copy of nauty from within the sage library). If you popen() another path in your own program (from nauty or otherwise), then it's up to you to ensure that something lives there.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 2 years ago by dimpase

Replying to mjo:

Replying to dimpase:

Also, imagine people writing Sage/Python code using other nauty utilities, not covered by the list of four. With your proposal, they'd be for a nasty surprise of having to modify not only Python, but also m4 macros...

Those four are the only ones used by the Sage library, which is the only thing we're testing for here (whether or not we're going to use the system copy of nauty from within the sage library). If you popen() another path in your own program (from nauty or otherwise), then it's up to you to ensure that something lives there.

from point of view of Sage the distribution, Sage provides a working copy of nauty, not just these four programs. Your approach breaks more of nauty than mine.

comment:24 in reply to: ↑ 23 Changed 2 years ago by mjo

Replying to dimpase:

from point of view of Sage the distribution, Sage provides a working copy of nauty, not just these four programs. Your approach breaks more of nauty than mine.

With either the system copy or the spkg, users get a completely functional copy of nauty, don't they? It was not my intention to break that. Can you give me an example of what goes wrong in my approach that doesn't with yours?

comment:25 follow-up: Changed 2 years ago by dimpase

Suppose one wrote some Sage code using nauty's amtog script, calling it in subprocess.Popen("amtog...,or whatever. Now, this will break on Debian's nauty, as it is called nauty-amtog there.

With my patch, the problem can be fixed in Python by mimicking what's done in, say, src/sage/graphs/digraph_generators.py :

from sage.env import SAGE_NAUTY_BINS_PREFIX as nautyprefix
...
subprocess.Popen(nautyprefix+"amtog...

With your patch, one would need either to do executable discovery in Python (ugly), extract the correct prefix from, say, DIRECTG_PATH (cursing the patch's author) and use it as above, or modify your patch by adding amtog to the list of four (tricky and might just be infeasible, as it modifies Sage sources).

comment:26 in reply to: ↑ 25 Changed 2 years ago by mjo

Replying to dimpase:

Suppose one wrote some Sage code using nauty's amtog script, calling it in subprocess.Popen("amtog...,or whatever. Now, this will break on Debian's nauty, as it is called nauty-amtog there.

With my patch, the problem can be fixed in Python by mimicking what's done

Relying on these dependencies to be satisfied transitively can't possible work in sage-the-distribution, or any other linuxy distribution. The way we design these spkg-configure tests is to test for only the features used by the sage library. With nauty, we test for the four programs the sage library uses. With e.g. zlib, we test for a version of zlib that works with the sage library.

If someone wants to write a program that works "in sage" but which uses nauty or zlib, they can't depend on a particular interface being presented by those two package just because they "come from sage." Consider that,

  1. Multiple versions of nauty and zlib (and everything else) can wind up installed to satisfy the sage dependency. New executables get added to nauty in new versions, and our configure check will match any version. If the user wants to ensure that an executable is there with a specific name, that's too much to ask -- he can't even be sure that the executable is there at all! Likewise if you want to use the public API of the zlib that comes with sage. If you want to use a newer function, you can't rely on sage's zlib to provide it, because sage will happily detect and use an old version of zlib.
  2. You can't fix the first problem even if you detect an exact version in spkg-configure. Different versions of sage install different versions of its dependencies anyway, so if you build against "sage", then you're back to the problem you started with: now you need to know what version of sage is installed as a proxy for the version of the library/programs that are installed.

This is no different than it is for any other linux distribution that provides a bunch of software. If you code against "debian", and if you use a feature in a library on debian, your program is still responsible for testing the existence of that feature. There's nothing debian can do for you to present an immutable API, and sage is the same.

With your patch, one would need either to do executable discovery in Python (ugly), extract the correct prefix from, say, DIRECTG_PATH (cursing the patch's author) and use it as above, or modify your patch by adding amtog to the list of four (tricky and might just be infeasible, as it modifies Sage sources).

You've left out the best option =)

If someone writes a program against sagelib and against features of nauty that aren't a part of sagelib, then their build system has to ensure that those features are present. This is no different than writing any program that's supposed to work on any other distribution. If your code uses amtog, your build system should check for amtog.

With all that said... I don't really care if we promise more than we can deliver. I don't like that it makes the test a lot uglier than it has to be, and that we check for geng twice, but if it works (I'll build your branch tonight) I'm willing to thumbs-up it.

comment:27 Changed 2 years ago by dimpase

As I already said, removing geng, from the 2nd line of spkg-configure.m4 will remove double-testing for geng. But it's a minor point.

comment:28 Changed 2 years ago by mjo

  • Status changed from needs_review to positive_review

God help the next person that needs to debug that m4 file, but it seems to work =P

comment:29 Changed 2 years ago by fbissey

Clever, but not helpful with distros (non debian) where SAGE_NAUTY_BINS_PREFIX is most likely undefined [in gentoo I got rid of sage-env altogether and I fell much better for it]. It being undefined means that in env.py

var('SAGE_NAUTY_BINS_PREFIX')

sets it to None, which is not a string which has the following kind of consequences

sage -t --long /usr/lib/python3.7/site-packages/sage/graphs/line_graph.pyx
**********************************************************************
File "/usr/lib/python3.7/site-packages/sage/graphs/line_graph.pyx", line 435, in sage.graphs.line_graph.root_graph
Failed example:
    for i,g in enumerate(graphs(6)): # long time
      if not g.is_connected():       # long time
        continue                     # long time
      test(g)                        # long time
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.7/site-packages/sage/doctest/forker.py", line 681, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.7/site-packages/sage/doctest/forker.py", line 1123, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.graphs.line_graph.root_graph[2]>", line 1, in <module>
        for i,g in enumerate(graphs(Integer(6))): # long time
      File "/usr/lib/python3.7/site-packages/sage/graphs/graph_generators.py", line 739, in __call__
        for g in graphs.nauty_geng(vertices):
      File "/usr/lib/python3.7/site-packages/sage/graphs/graph_generators.py", line 926, in nauty_geng
        sp = subprocess.Popen(nautyprefix+"geng {0}".format(options), shell=True,
    TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'
**********************************************************************

I guess defaulting to an empty string in env.py would do the trick.

I am happy for such correction to be in a follow up ticket.

comment:30 follow-up: Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

I'm seeing various failures on buildbots (where nauty is not system-wide installed)

comment:31 in reply to: ↑ 30 Changed 2 years ago by fbissey

Replying to vbraun:

I'm seeing various failures on buildbots (where nauty is not system-wide installed)

I think that make my argument for a default in env.py stronger. Using

var('SAGE_NAUTY_BINS_PREFIX',    '')

is enough to fix the issue for me and I am sure it would make Volker's bots happy.

comment:32 follow-up: Changed 2 years ago by vbraun

sage: from sage.env import SAGE_NAUTY_BINS_PREFIX as nautyprefix
sage: nautyprefix
'@SAGE_NAUTY_BINS_PREFIX@'

comment:33 in reply to: ↑ 32 Changed 2 years ago by dimpase

Replying to vbraun:

sage: from sage.env import SAGE_NAUTY_BINS_PREFIX as nautyprefix
sage: nautyprefix
'@SAGE_NAUTY_BINS_PREFIX@'

this looks like an issue with improperly updated ./configure.

comment:34 Changed 2 years ago by fbissey

OK, a default in env.py won't fix that :) I am not even sure how that happen. An improperly updated configure could do that I guess.

comment:35 Changed 2 years ago by git

  • Commit changed from 2800b65e699624ec92eb526cb2035330101fe0a2 to 0c17402e3a28298137b03ed61e129ac512ebfb45

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

0a281d4spkg-configure for nauty
6c240a7prefixes for calls to nauty routines, adjust tests
0c17402allow undefined SAGE_NAUTY_BINS_PREFIX

comment:36 Changed 2 years ago by dimpase

  • Status changed from needs_work to positive_review

rebased over the latest beta, and added the fix from comment:31

modulo a possible release process issue wrt ./configure all is fine.

Last edited 2 years ago by dimpase (previous) (diff)

comment:37 Changed 2 years ago by vbraun

  • Branch changed from u/dimpase/packages/nautyconfig to 0c17402e3a28298137b03ed61e129ac512ebfb45
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:38 follow-ups: Changed 2 years ago by mkoeppe

  • Commit 0c17402e3a28298137b03ed61e129ac512ebfb45 deleted

New versions of normaliz (#27952) use libnauty as well. Provided by libnauty-dev on Debian. Not sure which other distributions provide the library too.

Should we be adding a test for libnauty to build/pkgs/nauty/spkg-configure.m4? Alternatively, I could imagine creating a script package libnauty (depending on nauty) with its own spkg-configure.m4. Thoughts?

comment:39 in reply to: ↑ 38 ; follow-up: Changed 2 years ago by mjo

Replying to mkoeppe:

New versions of normaliz (#27952) use libnauty as well. Provided by libnauty-dev on Debian. Not sure which other distributions provide the library too.

Should we be adding a test for libnauty to build/pkgs/nauty/spkg-configure.m4?

That sounds right to me. There's even a pkg-config file if you're lucky.

comment:40 in reply to: ↑ 39 Changed 2 years ago by mkoeppe

Replying to mjo:

There's even a pkg-config file if you're lucky.

No such luck. Upstream nauty does not even have a proper build system; Debian replaces it with autotools, but does not provide .pc.

comment:41 in reply to: ↑ 38 Changed 2 years ago by dimpase

Replying to mkoeppe:

New versions of normaliz (#27952) use libnauty as well. Provided by libnauty-dev on Debian. Not sure which other distributions provide the library too.

Fedora & Co do, see https://rpmfind.net/linux/rpm2html/search.php?query=libnauty

Should we be adding a test for libnauty to build/pkgs/nauty/spkg-configure.m4? Alternatively, I could imagine creating a script package libnauty (depending on nauty) with its own spkg-configure.m4. Thoughts?

yes, this seems to be more reasonable, as libnauty is only used in optional packages

Note: See TracTickets for help on using tickets.