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: |
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
- Branch set to u/dimpase/packages/palpconfig
- Cc mjo mkoeppe enriqueartal isuruf added
- Commit set to 882b9b8aef3f6e129e812cc17dab60dd5fe04de4
- Status changed from new to needs_review
comment:2 Changed 2 years ago by
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
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: ↓ 5 Changed 2 years ago by
Debian has essentially the same naming scheme for palp's programs in its package.
comment:5 in reply to: ↑ 4 Changed 2 years ago by
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
yes, assuming upstream is sensible. I spent a lot of time convincing upstreams to accept patches, it is often frustrating.
comment:7 follow-up: ↓ 14 Changed 2 years ago by
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
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
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: ↓ 13 Changed 2 years ago by
- 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
- 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
- Cc vbraun added; volker removed
comment:13 in reply to: ↑ 10 Changed 2 years ago by
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 forclass-4d.x
and substitutesclass.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
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: ↓ 16 Changed 2 years ago by
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
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
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
one way to test palp executables is to look up examples in https://arxiv.org/abs/math/0204356
comment:19 Changed 2 years ago by
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
- Commit changed from 882b9b8aef3f6e129e812cc17dab60dd5fe04de4 to c79fc1b959060ab8f07fe5eb2526249e028f7244
comment:21 Changed 2 years ago by
- Commit changed from c79fc1b959060ab8f07fe5eb2526249e028f7244 to 4a6ec7012007e4417801e73666631f572ac107ff
Branch pushed to git repo; I updated commit sha1. New commits:
4a6ec70 | getting quoting right everywhere
|
comment:22 Changed 2 years ago by
- 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: ↓ 25 Changed 2 years ago by
- 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
- Commit changed from 4a6ec7012007e4417801e73666631f572ac107ff to 926c9f32b7bcfcf673c92c3fa88cdfda5fb38197
Branch pushed to git repo; I updated commit sha1. New commits:
926c9f3 | added quotes
|
comment:25 in reply to: ↑ 23 Changed 2 years ago by
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
- Status changed from needs_review to positive_review
Ok, works now.
comment:27 Changed 2 years ago by
- Branch changed from u/dimpase/packages/palpconfig to 926c9f32b7bcfcf673c92c3fa88cdfda5fb38197
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
spkg-configure for palp