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

allowing external ECM to be called gmp-ecm or ecm #37011

Merged
merged 5 commits into from
Mar 25, 2024
Merged

allowing external ECM to be called gmp-ecm or ecm #37011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7a9ce37
allowing external ECM to be called gmp-ecm, not ecm
dimpase Jan 5, 2024
2c53f08
add sage.features.ecm
dimpase Jan 5, 2024
afeffaa
replace SAGE_ECMBIN with ECM
dimpase Jan 5, 2024
c84929a
Revert "replace SAGE_ECMBIN with ECM"
dimpase Jan 7, 2024
2ea4345
remove call to baseline
dimpase Jan 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions build/pkgs/ecm/spkg-configure.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
SAGE_SPKG_CONFIGURE([ecm], [
m4_pushdef([SAGE_ECM_MINVER],[7.0.4])
ECMBIN=ecm
SAGE_SPKG_DEPCHECK([gmp], [
AC_CHECK_HEADER(ecm.h, [
AX_ABSOLUTE_HEADER([ecm.h])
Expand All @@ -18,20 +19,21 @@ SAGE_SPKG_CONFIGURE([ecm], [
AC_SEARCH_LIBS([ecm_factor], [ecm], [], [sage_spkg_install_ecm=yes])
])
])
AC_PATH_PROG([ECMBIN], [ecm])
if test x$ECMBIN != x; then
AC_PATH_PROGS([ECMBIN], [ecm gmp-ecm])
AS_IF([test x$ECMBIN != x], [
ecmbin_version=`echo 121 | $ECMBIN 4 | grep ^GMP |
$SED -n -e 's/GMP\-ECM \([[0-9]]*\.[[0-9]]*\.[[0-9]]*\).*/\1/p'`
fi
AS_IF([test -n "$ecmbin_version"], [
AX_COMPARE_VERSION([$ecmbin_version], [ge], [$SAGE_ECM_MINVER], [
ac_cv_ECMBIN="$ecmbin_version"
AS_IF([test -n "$ecmbin_version"], [
AX_COMPARE_VERSION([$ecmbin_version], [ge], [$SAGE_ECM_MINVER], [
ac_cv_ECMBIN="$ecmbin_version"
])
])
])
], [ECMBIN=ecm])
fi
AS_IF([test -z "$ac_cv_ECM"], [sage_spkg_install_ecm=yes])
AS_IF([test -z "$ac_cv_ECMBIN"], [sage_spkg_install_ecm=yes])
], [sage_spkg_install_ecm=yes])
])
m4_popdef([SAGE_ECM_MINVER])
AC_SUBST(SAGE_ECMBIN, $ECMBIN)
])
2 changes: 2 additions & 0 deletions pkgs/sage-conf/_sage_conf/_conf.py.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ ECL_CONFIG = "@SAGE_ECL_CONFIG@".replace('${prefix}', SAGE_LOCAL)

SAGE_NAUTY_BINS_PREFIX = "@SAGE_NAUTY_BINS_PREFIX@"

SAGE_ECMBIN = "@SAGE_ECMBIN@"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SAGE_ECMBIN = "@SAGE_ECMBIN@"
ECM = "@ECM@"


# Names or paths of the 4ti2 executables
FOURTITWO_HILBERT = "@FOURTITWO_HILBERT@"
FOURTITWO_MARKOV = "@FOURTITWO_MARKOV@"
Expand Down
1 change: 1 addition & 0 deletions src/sage/env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ def var(key: str, *fallbacks: Optional[str], force: bool = False) -> Optional[st
MAXIMA_FAS = var("MAXIMA_FAS")
KENZO_FAS = var("KENZO_FAS")
SAGE_NAUTY_BINS_PREFIX = var("SAGE_NAUTY_BINS_PREFIX", "")
SAGE_ECMBIN = var("SAGE_ECMBIN")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SAGE_ECMBIN = var("SAGE_ECMBIN")
ECM = var("ECM")

... just like MAXIMA. Only nauty has this weird naming

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a variable for everything under the sun? There's a well established way to configure binary locations, it's called PATH. If you must: wouldn't it be better to use SAGE_* to keep it in sage "namespace"?

As for the two standard names for the binary, that's unfortunate, is there a valid reason why Fedora does that? Is it possible/desirable to have the executable feature try both?

For non-standard setups one can always place a symlink in a custom directory and add it to PATH.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A symlink worked and one could asked fedora packagers to place this symlink, but apparently it is not the only distribution. Concerning the error in src/sage/libs/gap/element.pyx, it happens in line 2489:

for i in range(100):
        rnd = [ randint(-10,10) for i in range(randint(0,7)) ]
        # compute the sum in GAP
        _ = libgap.Sum(rnd)
        try:
            libgap.Sum(*rnd)
            print('This should have triggered a ValueError')
            print('because Sum needs a list as argument')
        except ValueError:
            pass

getting Killed due to segmentation fault. Running this code in the terminal works, the error appears only in tests and with python 3.12.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A symlink worked and one could asked fedora packagers to place this symlink, but apparently it is not the only distribution.

You mean: ask the fedora packager not to rename the executable (https://src.fedoraproject.org/rpms/gmp-ecm/blob/rawhide/f/gmp-ecm.spec#_99). We don't need to do anything special in void linux for ecm to be named ecm. Anyway, that line there is 15 years old, and possibly other distros also use the same name for the binary, so I think it's ok to try both names (I'm only objecting to do it at configure time: just do it at runtime).

Concerning the error in src/sage/libs/gap/element.pyx, it happens in line 2489:

for i in range(100):
        rnd = [ randint(-10,10) for i in range(randint(0,7)) ]
        # compute the sum in GAP
        _ = libgap.Sum(rnd)
        try:
            libgap.Sum(*rnd)
            print('This should have triggered a ValueError')
            print('because Sum needs a list as argument')
        except ValueError:
            pass

getting Killed due to segmentation fault. Running this code in the terminal works, the error appears only in tests and with python 3.12.

I never saw that, I also can't see what this has to do with python 3.12. Unless something there was compiled for python 3.11 and now run on python 3.12 (since layout of integers changed). But something like that I would expect to give many more issues...

Is it something that happens every time, or at random? Did you manage to get a clean backtrace of the segfault? Are you sure the sage+gap used for tests is exactly the same you are using for trying this in the terminal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is how the package got into Fedora in the first place.

https://bugzilla.redhat.com/show_bug.cgi?id=473330#c2

there is a comment from the reviewer
requesting the binary to be called gmp-ecm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, the meson PR is #36524. Works completely without sage-conf and sage-setup, and determines the necessary config at build time.

I would say, as a rough guide, binaries should be found at build time and databases/static files should be replaced by python modules. With that sage's features could be simplified (or even partly removed).

With #37024 I can build sagemath-standard from pypi with no configure step involved and no sage_conf hack.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I unsubscribed to keep my sanity. Moreover, I'm not interested in building the kitchen sink. For sagelib, setuptools builds it just fine, but depending on sage_conf, sage.env, configure-time detection that only happens when you build the kitchen sink, is just taxing me to keep me in your walled-garden. Thanks but, no thanks.

I'm pretty sure we want exactly the same thing. If you ever think I'm concerned about sage-conf, sage-env, or anything under build/, then please slow down and read again. The meson build system builds only sagelib. That's the point.

Setuptools cannot build sagelib, unless you happen to run your own distro. Setuptools can't detect C libraries or programs in general; see e.g. cvxopt for what happens when you try. Meson lets us fix that: it gives us a way to build only sagelib, but reliably, and without all of the sage-the-distro scaffolding.

I don't think it's an improvement, and this PR is already calling shutil.which() once, what I say is, if shutil.which('ecm') fails, try shutil.which('gmp-ecm') before complaining. I'm sure the energy wasted doing that in a year of world-wide use of sagemath will be way less than the energy it would take to have fedora fix their disrespect of upstream.

Again, we don't disagree. I feel the same way about distros renaming executables and breaking consumers. It was my first comment on this ticket.

With #37024 I can build sagemath-standard from pypi with no configure step involved and no sage_conf hack.

One more case where we want exactly the same thing. I just remade combinatorial_designs as a python package (https://github.com/orlitzky/mols-handbook-data) and am waiting for a repo to make a release (#37025).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's an improvement, and this PR is already calling shutil.which() once

hmm, where does this happen?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's an improvement, and this PR is already calling shutil.which() once

hmm, where does this happen?

That's how the Executable feature knows it's available. My proposal is to modify this class so one can pass a list of strings instead of just a string. In that case, it would try each option until the first one that works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never understood the need for sage.features. It seems to papering over a deficiency in Sage package management...

RUBIKS_BINS_PREFIX = var("RUBIKS_BINS_PREFIX", "")
FOURTITWO_HILBERT = var("FOURTITWO_HILBERT")
FOURTITWO_MARKOV = var("FOURTITWO_MARKOV")
Expand Down
42 changes: 42 additions & 0 deletions src/sage/features/ecm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# -*- coding: utf-8 -*-
r"""
Feature for testing the presence of ``ecm`` or ``gmp-ecm``
"""
# ****************************************************************************
# Copyright (C) 2032 Dima Pasechnik <dima@pasechnik.info>
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 2 of the License, or
# (at your option) any later version.
# https://www.gnu.org/licenses/
# ****************************************************************************

from . import Executable
from sage.env import SAGE_ECMBIN


class Ecm(Executable):
r"""
A :class:`~sage.features.Feature` describing the presence of :ref:`GMP-ECM <spkg_ecm>`.

EXAMPLES::

sage: from sage.features.ecm import Ecm
sage: Ecm().is_present()
FeatureTestResult('ecm', True)
"""
def __init__(self):
r"""
TESTS::

sage: from sage.features.ecm import Ecm
sage: isinstance(Ecm(), Ecm)
True
"""
Executable.__init__(self, name="ecm", executable=SAGE_ECMBIN,
spkg="ecm", type="standard")


def all_features():
return [Ecm()]
3 changes: 2 additions & 1 deletion src/sage/interfaces/ecm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
from sage.structure.sage_object import SageObject
from sage.rings.integer_ring import ZZ

from sage.env import SAGE_ECMBIN

class ECM(SageObject):

Expand Down Expand Up @@ -182,7 +183,7 @@ def __init__(self, B1=10, B2=None, **kwds):
self._cmd = self._make_cmd(B1, B2, kwds)

def _make_cmd(self, B1, B2, kwds):
ecm = ['ecm']
ecm = [SAGE_ECMBIN]
options = []
for x, v in kwds.items():
if v is False:
Expand Down
Loading