Opened 2 years ago

Closed 2 years ago

Last modified 23 months ago

#29673 closed enhancement (fixed)

spkg-configure.m4 for sympow

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

Status badges

Description

this should be easy, just binaries (and possibly data, tbd)

in Debian it's sympow and (maybe needed) sympow-data

Change History (14)

comment:1 Changed 2 years ago by dimpase

  • Authors set to Dima Pasechnik
  • Branch set to u/dimpase/packages/sympowconfig
  • Cc mjo mkoeppe added
  • Commit set to 2113187d27646192083d29925b2873bdee0e6096
  • Status changed from new to needs_review

New commits:

2113187spkg-configure for sympow

comment:2 Changed 2 years ago by dimpase

  • Cc isuruf added

comment:3 Changed 2 years ago by mjo

Works for me with the sympow fork. As of five minutes ago, this is sci-mathematics/sympow in Gentoo. Can you add that to gentoo.txt?

My only other question is why you settled on testing $ac_cv_path_SYMPOW instead of just $SYMPOW? The name of that cache variable is documented, so it should work just as well, but it was surprising to me.

comment:4 Changed 2 years ago by git

  • Commit changed from 2113187d27646192083d29925b2873bdee0e6096 to 99ebe047c653cccc930d667876f8c7782138d6eb

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

f36ccbaspkg-configure for sympow
99ebe04add gentoo package name

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

Added gentoo info.

Regarding SYMPOW vs ac_cv_path_SYMPOW, the latter seems to be what we normally use in many other spkg-configure.m4 files.

It appears that using SYMPOW is more hacky, e.g. if it's set then the test is not carried out, etc. E.g. if it's set to something not in PATH, it will probably not be found later on by sagelib.

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

Replying to dimpase:

It appears that using SYMPOW is more hacky, e.g. if it's set then the test is not carried out, etc. E.g. if it's set to something not in PATH, it will probably not be found later on by sagelib.

You mean if the user exports SYMPOW="junk or some non-PATH location"? I think the only reason ac_cv_path_SYMPOW avoids that same problem is because of the ugly variable name. Instead of SYMPOW we could use e.g. SAGE_SYMPOW_PATH or something like that to the same end.

In another life, we could make overrides work with e.g. --with-sympow=/home/mjo/bin/sympow but I think we all have better things to do right now.

comment:7 Changed 2 years ago by mjo

  • Reviewers set to Michael Orlitzky
  • Status changed from needs_review to positive_review

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

Replying to mjo:

Replying to dimpase:

It appears that using SYMPOW is more hacky, e.g. if it's set then the test is not carried out, etc. E.g. if it's set to something not in PATH, it will probably not be found later on by sagelib.

You mean if the user exports SYMPOW="junk or some non-PATH location"? I think the only reason ac_cv_path_SYMPOW avoids that same problem is because of the ugly variable name. Instead of SYMPOW we could use e.g. SAGE_SYMPOW_PATH or something like that to the same end.

Autoconf docs are silent on what happens if ac_cv_path_SYMPOW is pre-set, so it appears to be more robust.

Thanks for review!

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

Replying to dimpase:

Autoconf docs are silent on what happens if ac_cv_path_SYMPOW is pre-set, so it appears to be more robust.

I can't find any explicit docs, but as a cache variable, it does what you'd expect: the configure script first tests the value of ac_cv_path_SYMPOW, and if it's set, its value gets used in lieu of the check:

if ${ac_cv_path_SYMPOW+:} false; then :
  $as_echo_n "(cached) " >&6
else
  # the check gets run and ac_cv_path_SYMPOW is populated
fi
SYMPOW=$ac_cv_path_SYMPOW

comment:10 Changed 2 years ago by vbraun

  • Branch changed from u/dimpase/packages/sympowconfig to 99ebe047c653cccc930d667876f8c7782138d6eb
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:11 Changed 23 months ago by mkoeppe

  • Commit 99ebe047c653cccc930d667876f8c7782138d6eb deleted

Follow-up: It looks like this accepts a broken sympow on fedora-32 - #3360

comment:12 Changed 23 months ago by mkoeppe

Also with sympow on ubuntu-bionic-standard (https://github.com/mkoeppe/sage/runs/867730944)

comment:13 Changed 23 months ago by mkoeppe

comment:14 Changed 23 months ago by mkoeppe

I've opened #30147 (Fix spkg-configure.m4 for sympow) for this. Please fix!

Note: See TracTickets for help on using tickets.