allowing external ECM to be called gmp-ecm or ecm

[dimpase]

ECM executable in Fedora is called gmp-ecm

This PR allows both names, ecm and gmp-ecm, to be used

[dimpase]

This problem was mentioned in #36860

[dimpase]

Do we need sage.features.ecm ?

[mkoeppe]

why basename

[orlitzky]

pretty sure that's a -z test in disguise, too

[dimpase]

Because sage.interfaces.ecm takes ecm or gmp-ecm as the data it needs to call the executable, not the full path. So it assumes it's on PATH, I'm just keeping it like this here.

[mkoeppe]

Maybe the PATH was only set at configure time?

[dimpase]

Same argument applies for the previous design. How is this relevant to this PR?

[mkoeppe]

Suggested change

	SAGE_ECMBIN = var("SAGE_ECMBIN")
	ECM = var("ECM")

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

[tornaria]

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.

[enriqueartal]

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.

[tornaria]

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?

[dimpase]

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.

[tornaria]

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.

[orlitzky]

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).

[dimpase]

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

hmm, where does this happen?

[tornaria]

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.

[dimpase]

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

[mkoeppe]

Suggested change

	SAGE_ECMBIN = "@SAGE_ECMBIN@"
	ECM = "@ECM@"

[orlitzky]

Normally I would take this opportunity to rant about packagers renaming executables and breaking every AC_PATH_PROG out there, but the naming of this package/executable are crazy. The repo is "ecm" but the package is "gmp-ecm" but the executable is "ecm" but the README says "GMP-ECM reads the numbers to be factored from stdin" but the library is -lecm but the program calls itself "GMP-ECM 7.0.5" etc.

[mkoeppe]

Do we need sage.features.ecm ?

Yes please.

[dimpase]

We need to do something with SAGE_ECMBIN in case the package is already installed, it seems to me.

[dimpase]

We need to do something with SAGE_ECMBIN in case the package is already installed, it seems to me.

if we can assume that pkgs/sage-conf/_sage_conf/_conf.py is present in such a case, then all is good as is.

[dimpase]

I've left the internal variable name in .m4 stay, it's more self-documenting.

[enriqueartal]

I have tested it with python 3.12 in Fedora 39, system ecm is taken. The only relevant error is a crash testing src/sage/libs/gap/element.pyx (which happens with or without this changes only with python 3.12).

[dimpase]

@tornaria runs Sage with Python 3.12. I am not sure whether he sees this error too.

[tornaria]

@tornaria runs Sage with Python 3.12. I am not sure whether he sees this error too.

What error? I don't see any error.

sage -t --long --warn-long --random-seed=139019732416246170153767453232131014193 src/sage/libs/gap/element.pyx
    [506 tests, 3.19 s]

[tornaria]

Should you get the filename from the feature instead? This way it seems it will only work if ECM is set, there's no fallback if ECM is None.

[mkoeppe]

Yes, it should be using the Feature.

[dimpase]

I have no idea how to use Feature here. It seems to provide absolute executable names, but no basenames.
Perhaps its a missing feature in Feature.

I am certainly not going to implement basename in Python just here.

[mkoeppe]

Why do you think you need the basename?

[dimpase]

Because this is what sage.interfaces.ecm is using.
Not sure why whoever wrote this got so creative, but that's how it is.

[mkoeppe]

What are you talking about. It's just using Popen, of course you can pass the full executable name to it, not just the basename.

[tornaria]

I have no idea how to use Feature here. It seems to provide absolute executable names, but no basenames. Perhaps its a missing feature in Feature.

Using the full path seems better, to make sure there's a single point where the decision of which executable to use is made. How the feature found the path, whether it's static or dynamic, if we want to support PATH being different at build time than at run time, etc, is all abstracted out in the feature.

I am certainly not going to implement basename in Python just here.

Just for the record, there's os.path.basename() (and os.path.dirname(), and os.path.split())

[enriqueartal]

@tornaria runs Sage with Python 3.12. I am not sure whether he sees this error too.

What error? I don't see any error.

sage -t --long --warn-long --random-seed=139019732416246170153767453232131014193 src/sage/libs/gap/element.pyx
    [506 tests, 3.19 s]

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.

[dimpase]

Please point at the item(s) that need work

[mkoeppe]

Needs work: #37011 (comment)

[dimpase]

e1d3f37 must be reverted, for in some cases (system Python 3.11 and ecm on Gentoo), it creates a clash of names (too many ECM, ecm, Ecm, etc around), and ugly errors in sage.interfaces.ecm

sage -t --warn-long 72.5 --random-seed=153732071471673388791781934815376131504 src/sage/interfaces/ecm.py
**********************************************************************
File "src/sage/interfaces/ecm.py", line 33, in sage.interfaces.ecm
Failed example:
    ecm.factor(n, B1=10)
Exception raised:
    Traceback (most recent call last):
      File "/mnt/opt/Sage/sage-dev/src/sage/doctest/forker.py", line 709, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/mnt/opt/Sage/sage-dev/src/sage/doctest/forker.py", line 1144, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.interfaces.ecm[2]>", line 1, in <module>
        ecm.factor(n, B1=Integer(10))
      File "/mnt/opt/Sage/sage-dev/src/sage/interfaces/ecm.py", line 663, in factor
        n_factorization = self.find_factor(n, B1=B1)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/mnt/opt/Sage/sage-dev/src/sage/interfaces/ecm.py", line 567, in find_factor
        factors = self._find_factor(n, factor_digits, B1, **kwds)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/mnt/opt/Sage/sage-dev/src/sage/interfaces/ecm.py", line 517, in _find_factor
        out = self._run_ecm(cmd, n)
              ^^^^^^^^^^^^^^^^^^^^^
      File "/mnt/opt/Sage/sage-dev/src/sage/interfaces/ecm.py", line 224, in _run_ecm
        p = Popen(cmd, stdout=PIPE, stdin=PIPE, stderr=PIPE, encoding='latin-1')
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.11/subprocess.py", line 1026, in __init__
        self._execute_child(args, executable, preexec_fn, close_fds,
      File "/usr/lib/python3.11/subprocess.py", line 1824, in _execute_child
        and os.path.dirname(executable)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "<frozen posixpath>", line 152, in dirname
    TypeError: expected str, bytes or os.PathLike object, not type
...
7 items had failures:
   2 of   6 in sage.interfaces.ecm
   1 of   2 in sage.interfaces.ecm.ECM.__call__
   1 of   4 in sage.interfaces.ecm.ECM._find_factor
   1 of   3 in sage.interfaces.ecm.ECM.factor
   1 of   6 in sage.interfaces.ecm.ECM.find_factor
   4 of   9 in sage.interfaces.ecm.ECM.one_curve
   2 of   4 in sage.interfaces.ecm.ECM.time
    [47 tests, 12 failures, 0.08 s]

[dimpase]

OK, got rid of baseline call now.

Do we want

--- a/src/sage/env.py
+++ b/src/sage/env.py
@@ -212,7 +212,7 @@ MAXIMA = var("MAXIMA", "maxima")
 MAXIMA_FAS = var("MAXIMA_FAS")
 KENZO_FAS = var("KENZO_FAS")
 SAGE_NAUTY_BINS_PREFIX = var("SAGE_NAUTY_BINS_PREFIX", "")
-SAGE_ECMBIN = var("SAGE_ECMBIN")
+SAGE_ECMBIN = var("SAGE_ECMBIN", "ecm")
 RUBIKS_BINS_PREFIX = var("RUBIKS_BINS_PREFIX", "")
 FOURTITWO_HILBERT = var("FOURTITWO_HILBERT")
 FOURTITWO_MARKOV = var("FOURTITWO_MARKOV")

[orlitzky]

I presume this is in the name of modularising sagelib. Feels wrong to me too, and I think I mentioned that already, it's demonstrating deficiencies in sagelib's deps solving.

Features do allow us to split off a module without rewriting a bunch of tests, but even in that situation it's better to use # needs sage.number_theory or whatever new module provides the functionality rather than depending on an implementation detail like the ecm binary being present. In any case, we obviously haven't needed # optional - ecm yet, so for the moment it's a premature de-optimization.

[enriqueartal]

I checked the branch, system ecm is used and no particular problem arise.

[dimpase]

Feel free to set it to positive review

[tornaria]

OK, got rid of baseline call now.

Do we want

--- a/src/sage/env.py
+++ b/src/sage/env.py
@@ -212,7 +212,7 @@ MAXIMA = var("MAXIMA", "maxima")
 MAXIMA_FAS = var("MAXIMA_FAS")
 KENZO_FAS = var("KENZO_FAS")
 SAGE_NAUTY_BINS_PREFIX = var("SAGE_NAUTY_BINS_PREFIX", "")
-SAGE_ECMBIN = var("SAGE_ECMBIN")
+SAGE_ECMBIN = var("SAGE_ECMBIN", "ecm")
 RUBIKS_BINS_PREFIX = var("RUBIKS_BINS_PREFIX", "")
 FOURTITWO_HILBERT = var("FOURTITWO_HILBERT")
 FOURTITWO_MARKOV = var("FOURTITWO_MARKOV")

The PR was merged without this important fix!

As of now, 10.4.beta0 is broken. Let's try not to add new knobs without a reasonable default.

[tornaria]

Also: it means the ecm feature is just useless bloat, as it didn't prevent my test run to fail.

IMO the only reason the feature bloat would be justified is if it would try ecm and fallback to gmp-ecm at runtime. The feature will traverse all PATH searching for an ecm executable, it might as well search again for gmp-ecm as a fallback.

@orlitzky it seems perfectly fine to start the search with a name (full path?) that is placed there by meson at build time, but there's also some value for binary wheels that the same package works in different distros, or that ecm works if it's installed later (so it's not a build dependency).

[orlitzky]

@orlitzky it seems perfectly fine to start the search with a name (full path?) that is placed there by meson at build time, but there's also some value for binary wheels that the same package works in different distros, or that ecm works if it's installed later (so it's not a build dependency).

Sure, I'm not suggesting we break any of that. If you're building for a distro, your build host will detect the correct path (for the distro) and substitute it into the config file that you ship as part of the resulting binary package. This is how basically every C package on debian/fedora work and I'm sure void is not much different.

For binary wheels, same trick. Where is gmp-ecm supposed to come from? If it's from pypi, then you install the pypi package that provides it on your build host, and you build the binary wheel. It detects the correct path... etc. The end result is always editable and can fall back to a sane default like "ecm," keeping in mind that there are a fixed number of exceptions using "gmp-ecm" while everyone else has plain "ecm" in their PATH.

Overriding the check to avoid the build dependency is also pretty common. We do this a lot with autotools by setting the ac_cv_foo cache variables to trick ./configure into thinking it has already detected something. It's apparently possible with meson using override_find_program() behind some build flag, but I confess, I haven't tried it yet.

[vbraun]

I'm getting

sage -t --long --warn-long 43.5 --random-seed=123 src/sage/rings/integer.pyx
**********************************************************************
File "src/sage/rings/integer.pyx", line 3068, in sage.rings.integer.Integer.divisors
Warning, slow doctest:
    for i in range(20):           # long time                             # needs sage.libs.pari
        try:
            alarm(RDF.random_element(1e-3, 0.5))
            _ = n.divisors()
            cancel_alarm()  # we never get here
        except AlarmInterrupt:
            pass
Test ran for 44.61 s, check ran for 0.00 s
**********************************************************************
File "src/sage/rings/integer.pyx", line 3956, in sage.rings.integer.Integer.?
Failed example:
    n.factor(algorithm='ecm')
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/src/sage/doctest/forker.py", line 715, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/src/sage/doctest/forker.py", line 1150, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.integer.Integer.?[20]>", line 1, in <module>
        n.factor(algorithm='ecm')
      File "sage/rings/integer.pyx", line 4052, in sage.rings.integer.Integer.factor
        res = [(p, 1) for p in ecm.factor(n, proof=proof)]
      File "/home/release/Sage/src/sage/interfaces/ecm.py", line 663, in factor
        n_factorization = self.find_factor(n, B1=B1)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/src/sage/interfaces/ecm.py", line 567, in find_factor
        factors = self._find_factor(n, factor_digits, B1, **kwds)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/src/sage/interfaces/ecm.py", line 517, in _find_factor
        out = self._run_ecm(cmd, n)
              ^^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/src/sage/interfaces/ecm.py", line 224, in _run_ecm
        p = Popen(cmd, stdout=PIPE, stdin=PIPE, stderr=PIPE, encoding='latin-1')
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/subprocess.py", line 1024, in __init__
        self._execute_child(args, executable, preexec_fn, close_fds,
      File "/home/release/Sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/subprocess.py", line 1901, in _execute_child
        raise child_exception_type(errno_num, err_msg, err_filename)
    PermissionError: [Errno 13] Permission denied: ''
**********************************************************************
1 item had failures:
   1 of  24 in sage.rings.integer.Integer.?
    [1174 tests, 1 failure, 53.97 s]
----------------------------------------------------------------------
sage -t --long --warn-long 43.5 --random-seed=123 src/sage/rings/integer.pyx  # 1 doctest failed
----------------------------------------------------------------------

Fedora 39 x86_64

config.log

[enriqueartal]

In Fedora 39 with the sage ecm package I got the same error, no error with system package

[tornaria]

I'm getting

sage -t --long --warn-long 43.5 --random-seed=123 src/sage/rings/integer.pyx
**********************************************************************
File "src/sage/rings/integer.pyx", line 3068, in sage.rings.integer.Integer.divisors
Warning, slow doctest:
    for i in range(20):           # long time                             # needs sage.libs.pari
        try:
            alarm(RDF.random_element(1e-3, 0.5))
            _ = n.divisors()
            cancel_alarm()  # we never get here
        except AlarmInterrupt:
            pass
Test ran for 44.61 s, check ran for 0.00 s
**********************************************************************
File "src/sage/rings/integer.pyx", line 3956, in sage.rings.integer.Integer.?
Failed example:
    n.factor(algorithm='ecm')
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/src/sage/doctest/forker.py", line 715, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/src/sage/doctest/forker.py", line 1150, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.integer.Integer.?[20]>", line 1, in <module>
        n.factor(algorithm='ecm')
      File "sage/rings/integer.pyx", line 4052, in sage.rings.integer.Integer.factor
        res = [(p, 1) for p in ecm.factor(n, proof=proof)]
      File "/home/release/Sage/src/sage/interfaces/ecm.py", line 663, in factor
        n_factorization = self.find_factor(n, B1=B1)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/src/sage/interfaces/ecm.py", line 567, in find_factor
        factors = self._find_factor(n, factor_digits, B1, **kwds)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/src/sage/interfaces/ecm.py", line 517, in _find_factor
        out = self._run_ecm(cmd, n)
              ^^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/src/sage/interfaces/ecm.py", line 224, in _run_ecm
        p = Popen(cmd, stdout=PIPE, stdin=PIPE, stderr=PIPE, encoding='latin-1')
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/subprocess.py", line 1024, in __init__
        self._execute_child(args, executable, preexec_fn, close_fds,
      File "/home/release/Sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/subprocess.py", line 1901, in _execute_child
        raise child_exception_type(errno_num, err_msg, err_filename)
    PermissionError: [Errno 13] Permission denied: ''
**********************************************************************
1 item had failures:
   1 of  24 in sage.rings.integer.Integer.?
    [1174 tests, 1 failure, 53.97 s]
----------------------------------------------------------------------
sage -t --long --warn-long 43.5 --random-seed=123 src/sage/rings/integer.pyx  # 1 doctest failed
----------------------------------------------------------------------

Fedora 39 x86_64

config.log

@vbraun @enriqueartal

Looking at the config.log here, it seems ecm spkg is installed from a previous run. Because of this, the line AC_SUBST(SAGE_ECMBIN, $ECMBIN) in build/pkgs/ecm/spkg-configure.m4 is not being run, so in effect you are running with SAGE_ECMBIN = "@SAGE_ECMBIN@" (from pkgs/sage-conf/_sage_conf/_conf.py.in) or maybe with SAGE_ECMBIN = "".

The default I added in #37701 for the case when SAGE_ECMBIN has not been set at all in sage-conf (which is the case when running sagelib without sage-the-distro).

Can you try to rebuild ecm spkg?

[enriqueartal]

Rebuilt with the same results in the test. By the way, SAGE_ECMBIN = ""

[dimpase]

have you merged #37701 before rebuilding and testing?

[vbraun]

In fact, incremental builds are what is broken

• ecm is not installed in the base os
• Sage make distclean && make succeeeds
• Tests succeed
• re-running ./bootstrap && ./configure && make works
• Tests now fail with PermissionError: [Errno 13] Permission denied: ''

[enriqueartal]

have you merged #37701 before rebuilding and testing?

Yes