Opened 2 years ago

Closed 2 years ago

#29672 closed enhancement (fixed)

spkg-configure.m4 for palp

Reported by: dimpase Owned by:
Priority: major Milestone: sage-9.2
Component: build: configure Keywords:
Cc: mjo, mkoeppe, enriqueartal, isuruf, vbraun Merged in:
Authors: Dima Pasechnik Reviewers: Michael Orlitzky
Report Upstream: N/A Work issues:
Branch: 926c9f3 (Commits, GitHub, GitLab) Commit: 926c9f32b7bcfcf673c92c3fa88cdfda5fb38197
Dependencies: Stopgaps:

Status badges

Description

it should be easy - it's a stable package, with just binary executables and data.

In debian it is called palp

Change History (27)

comment:1 Changed 2 years ago by dimpase

  • Authors set to Dima Pasechnik
  • Branch set to u/dimpase/packages/palpconfig
  • Cc mjo mkoeppe enriqueartal isuruf added
  • Commit set to 882b9b8aef3f6e129e812cc17dab60dd5fe04de4
  • Status changed from new to needs_review

New commits:

882b9b8spkg-configure for palp

comment:2 Changed 2 years ago by mjo

The reason I haven't included this in Gentoo yet is because sage builds optimized versions of the class.x, cws.x, nef.x, and poly.x executables -- and then installs them with nonstandard names.

Editing Global.h to define POLY_Dmax is what upstream recommends, but the names of the resulting binaries are always e.g. class.x when you do that. Sage then renames them too e.g. class-4d.x. I don't want to include the nonstandard executables in a distro package just to make sage (which isn't even in the tree yet) happy.

Does anyone know if upstream is still interested in making source improvements? It might be easy to consolidate these into one executable by having each foo.x dispatch to one of several optimized functions rather than having the user (i.e. sage) dispatch to one of several different optimized executables. That depends it being easy to infer POLY_Dmax from the input, of course. Otherwise, we could at least ask that the build system standardize on these names and actually build the optimized executables.

comment:3 Changed 2 years ago by mjo

I think I forgot to write this: in sage/geometry/polyhedron/palp_database.py, we have

        return Popen(['class-4d.x', '-He',

so Sage does use at least use that nonstandard name, and this script needs to check for it. I'm not sure if that will affect any of the other distros.

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

Debian has essentially the same naming scheme for palp's programs in its package.

Fedora as well.

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

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

Replying to dimpase:

Debian has essentially the same naming scheme for palp's programs in its package.

Fedora as well.

If all of the major distros are using the same convoluted build process to achieve a consistent naming scheme, then to me that still suggests that we should upstream that build process. It's hard for me to justify building the same package four times and installing a bunch of executables with non-standard names just to make the test suite of another package that's not even in the tree pass.

In lieu of some agreement, I'll probably add a fallback to sagelib so that it uses class.x instead of class-4d.x when the latter isn't present (like we did with nauty). That will allow sage to keep working with the "out of the box" version of palp.

comment:6 Changed 2 years ago by dimpase

yes, assuming upstream is sensible. I spent a lot of time convincing upstreams to accept patches, it is often frustrating.

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

I sent the maintainer a cleanup patch for the makefile, so I'll get some sort of impression soon enough.

comment:8 Changed 2 years ago by mjo

It turns out that the only place class-4d.x is even used is in an optional test for a huge, old-style SPKG that hasn't been ported yet (#26029). The nonstandard names are thus quite likely to be completely unused.

comment:9 Changed 2 years ago by mjo

Just kidding, the names of the executables are mangled on the fly. In geometry/lattice_polytope.py,

def set_palp_dimension(d):
    ...
    global _palp_dimension
    _palp_dimension = d

def _palp(command, polytopes, reduce_dimension=False):
    ...
    if _palp_dimension is not None:
        dot = command.find(".")
        command = command[:dot] + "-%dd" % _palp_dimension + command[dot:]

I give up.

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

  • Status changed from needs_review to needs_work

In light of that, you should check all 16 combinations of executable names (poly, class, nef cws) x (4,5,6 11) in the spkg-configure.m4. I started a branch at u/mjo/ticket/29672 that checks for class-4d.x and substitutes class.x in its place, but I'm not going to spend three days doing it for the other 15 names.

comment:11 Changed 2 years ago by dimpase

  • Cc volker added

Volker, you're a palp user - could you comment on the naming schemes for palp executables?

comment:12 Changed 2 years ago by dimpase

  • Cc vbraun added; volker removed

comment:13 in reply to: ↑ 10 Changed 2 years ago by dimpase

Replying to mjo:

In light of that, you should check all 16 combinations of executable names (poly, class, nef cws) x (4,5,6 11) in the spkg-configure.m4. I started a branch at u/mjo/ticket/29672 that checks for class-4d.x and substitutes class.x in its place, but I'm not going to spend three days doing it for the other 15 names.

OK, I'll try my hand in nested m4 loops :-)

comment:14 in reply to: ↑ 7 Changed 2 years ago by mjo

Replying to mjo:

I sent the maintainer a cleanup patch for the makefile, so I'll get some sort of impression soon enough.

Harald Skarke just merged these patches. It probably wouldn't be hard to fix the naming issue if everyone can agree on a solution.

I think having one executable (e.g. class.x) dispatch to the proper routine is clearly preferable, if the dimension can be determined quickly enough to do so. This avoids invalidating the examples in the paper (https://arxiv.org/abs/math/0204356) unnecessarily, and doesn't pollute /usr/bin with four copies of the same programs.

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

So, what's going to change once these merged upstream patches are in distros?

comment:16 in reply to: ↑ 15 Changed 2 years ago by mjo

Replying to dimpase:

So, what's going to change once these merged upstream patches are in distros?

Probably nothing, the series I sent only ensures that CC, LDFLAGS, CFLAGS, CPPFLAGS are supported out of the box.

comment:17 Changed 2 years ago by gh-thierry-FreeBSD

I just made a port of palm for FreeBSD, based upon build/pkgs/palp/spkg-install.in, but I do not see any data in it. The resulting MANIFEST (relative to $PREFIX) is:

bin/class-11d.x
bin/class-4d.x
bin/class-5d.x
bin/class-6d.x
bin/class.x
bin/cws-11d.x
bin/cws-4d.x
bin/cws-5d.x
bin/cws-6d.x
bin/cws.x
bin/mori-11d.x
bin/mori-4d.x
bin/mori-5d.x
bin/mori-6d.x
bin/mori.x
bin/nef-11d.x
bin/nef-4d.x
bin/nef-5d.x
bin/nef-6d.x
bin/nef.x
bin/poly-11d.x
bin/poly-4d.x
bin/poly-5d.x
bin/poly-6d.x
bin/poly.x

comment:18 Changed 2 years ago by dimpase

one way to test palp executables is to look up examples from https://arxiv.org/abs/math/0204356

Version 0, edited 2 years ago by dimpase (next)

comment:19 Changed 2 years ago by gh-thierry-FreeBSD

Yes, it works!

But my question was about the description of this ticket "it's a stable package, with just binary executables and data", since I do not see any datafiles.

comment:20 Changed 2 years ago by git

  • Commit changed from 882b9b8aef3f6e129e812cc17dab60dd5fe04de4 to c79fc1b959060ab8f07fe5eb2526249e028f7244

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

700ce3bspkg-configure for palp
c79fc1binner loop macro

comment:21 Changed 2 years ago by git

  • Commit changed from c79fc1b959060ab8f07fe5eb2526249e028f7244 to 4a6ec7012007e4417801e73666631f572ac107ff

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

4a6ec70getting quoting right everywhere

comment:22 Changed 2 years ago by dimpase

  • Status changed from needs_work to needs_review

OK, so as requested, checking that every executable is there. Needs review (you can just look in the config.log to see it does the right thing)

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

  • Reviewers set to Michael Orlitzky

The logic is OK but the variable tests need quotes otherwise they crash if I do something stupid like put the executables in /home/mjo/My Programs.

comment:24 Changed 2 years ago by git

  • Commit changed from 4a6ec7012007e4417801e73666631f572ac107ff to 926c9f32b7bcfcf673c92c3fa88cdfda5fb38197

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

926c9f3added quotes

comment:25 in reply to: ↑ 23 Changed 2 years ago by dimpase

Replying to mjo:

The logic is OK but the variable tests need quotes otherwise they crash if I do something stupid like put the executables in /home/mjo/My Programs.

OK, sure, done.

comment:26 Changed 2 years ago by mjo

  • Status changed from needs_review to positive_review

Ok, works now.

comment:27 Changed 2 years ago by vbraun

  • Branch changed from u/dimpase/packages/palpconfig to 926c9f32b7bcfcf673c92c3fa88cdfda5fb38197
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.