Opened 22 months ago

Last modified 3 days ago

#29024 needs_work enhancement

spkg-configure.m4 for singular

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.5
Component: build: configure Keywords: sd111
Cc: dimpase, fbissey, slelievre, isuruf, mjo, gh-tobiasdiez, arojas, gh-dkwo Merged in:
Authors: Michael Orlitzky, Samuel Lelièvre Reviewers: Dima Pasechnik, Matthias Koeppe
Report Upstream: Reported upstream. No feedback yet. Work issues:
Branch: u/mjo/ticket/29024 (Commits, GitHub, GitLab) Commit: a3c53051a5961226700bdb8a40f831092f8dffcd
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

  1. For singular, the location of the dynamic library needs to be communicated to the sage runtime.

We add a configuration variable SINGULAR_SO to sage_conf.py.in.

The filename can be found from src/Singular/libSingular.la (currently not installed):

# The name that we can dlopen(3).
dlname='libSingular-4.1.1.dylib'
...
# Directory that this library needs to be installed in:
libdir='/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib'

This will:

  • enable venv builds of sagelib (#30578, #29013) -- for which _get_shared_lib_filename is wrong
  • provide preparation for adding an spkg-configure.m4 for singular.
  1. src/bin/sage-env unconditionally sets environment variable SINGULARPATH="$SAGE_LOCAL/share/singular"

If SINGULARPATH is already set by the user, perhaps the directory in $SAGE_LOCAL should be prepended instead of overwriting it.

(After #32254, SINGULARPATH is only used by qepcad -- this last use should be removed in #31275.)

  1. src/bin/sage-env unconditionally sets environment variable SINGULAR_EXECUTABLE="$SAGE_LOCAL/bin/Singular".

It should be investigated whether this is actually needed in any supported configuration. If not, remove.

(Removed in #32302.)

  1. src/sage/interfaces/singular.py tries to use SINGULARPATH as if it is a directory name (it is a colon-separated list of directories):
    singular_docdir = SINGULARPATH + "/../info/"
    

This should be fixed so that it works when SINGULARPATH is set by a user to a colon-separated list.

(Solved in #32254 - Obtain singular.hlp location via libsingular_resources)

Attachments (2)

spkg-configure.m4 (219 bytes) - added by gh-thierry-FreeBSD 18 months ago.
spkg-configure.m4 to be put under /build/pkgs/singular/
homebrew-singular-experiment.patch (3.0 KB) - added by dimpase 14 months ago.

Download all attachments as: .zip

Change History (142)

comment:1 follow-up: Changed 22 months ago by mkoeppe

Which versions of singular are good?

comment:2 in reply to: ↑ 1 Changed 22 months ago by gh-mwageringel

Replying to mkoeppe:

Which versions of singular are good?

Sage does not currently work correctly with any version of Singular, see #25993.

comment:3 Changed 21 months ago by dimpase

well, Debian does have a Singular version which is compatible with their Sage package (v8.6 or so, I guess).

comment:4 Changed 21 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

Thanks for the info. I'll check back when #25993 is done.

comment:5 follow-up: Changed 18 months ago by gh-thierry-FreeBSD

This ticket should be listed in #27330 (ATM singular is listed in "No Tickets yet").

comment:6 in reply to: ↑ 5 Changed 18 months ago by mkoeppe

Replying to gh-thierry-FreeBSD:

This ticket should be listed in #27330 (ATM singular is listed in "No Tickets yet").

I've added it

comment:7 Changed 18 months ago by dimpase

  • Component changed from packages: standard to build: configure

Changed 18 months ago by gh-thierry-FreeBSD

spkg-configure.m4 to be put under /build/pkgs/singular/

comment:8 follow-up: Changed 18 months ago by gh-thierry-FreeBSD

[Copying elements from #29024]

  • my 1st problem was a build failure with Singular taken from an external package: to solve it, Singular must be built --with-ntl: it would be fine to add this check to spkg-configure.m4 (but I do not know how!)
  • my 2nd error was when building dochtml: NameError: Singular library 'freegb.lib' not found. To solve it, src/sage/env.py and src/bin/sage-env must be modified so that SINGULARPATH is set to singlar's share dir (and not SAGE_SHARE / DESTDIR), and SINGULAR_EXECUTABLE is set to the real Singular and not under the DESTDIR directory.

Now it is OK for me (on FreeBSD).

Last edited 17 months ago by slelievre (previous) (diff)

comment:9 Changed 17 months ago by mkoeppe

  • Cc mjo added

comment:10 Changed 16 months ago by mkoeppe

  • Description modified (diff)

comment:11 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:12 Changed 14 months ago by dimpase

the error message

        raise RuntimeError(
            "libSingular not found--a working Singular install in $SAGE_LOCAL "
            "is required for Sage to work")

should be modified, too. Perhaps just remove in $SAGE_LOCAL

comment:13 Changed 14 months ago by mkoeppe

Moving this ticket forward is great, but note that it is unrelated to the build problem on homebrew reported on sage-devel... and it is also unlikely that the homebrew version of singular actually works for us (homebrew does not like to patch).

As I explained in https://groups.google.com/d/msg/sage-devel/KXK_zxzfhIQ/RO3FryYWAwAJ, on the reporter's system the computed order of -I options is wrong -- something that I was not able reproduce on my system.

comment:14 in reply to: ↑ 8 Changed 14 months ago by dimpase

Replying to gh-thierry-FreeBSD:

[Copying elements from #29024]

  • my 1st problem was a build failure with Singular taken from an external package: to solve it, Singular must be built --with-ntl: it would be fine to add this check to spkg-configure.m4 (but I do not know how!)
  • my 2nd error was when building dochtml: NameError: Singular library 'freegb.lib' not found. To solve it, src/sage/env.py and src/bin/sage-env must be modified so that SINGULARPATH is set to singlar's share dir (and not SAGE_SHARE / DESTDIR), and SINGULAR_EXECUTABLE is set to the real Singular and not under the DESTDIR directory.

Now it is OK for me (on FreeBSD).

The fix in SageMath needs some programming - specifically, the code in singular.pyx calls dlopen on SINGULAR_SO, which is computed by _get_shared_lib_filename dynamically in env.py, and assuming that it is in SAGE_LOCAL. So this probably can instead by handled using cython_aliases from the same module (which does compute all the needed info for Singular, it seems). Or by fixing _get_shared_lib_filename so that it also searches elsewhere.

comment:15 Changed 14 months ago by mkoeppe

Is it known whether this comment added in 2009 to singular.pyx is still valid:

    This initializes the SINGULAR library. This is a hack to some
    extent.

    SINGULAR has a concept of compiled extension modules similar to
    Sage. For this, the compiled modules need to see the symbols from
    the main program. However, SINGULAR is a shared library in this
    context these symbols are not known globally. The work around so
    far is to load the library again and to specify ``RTLD_GLOBAL``.

comment:16 follow-up: Changed 14 months ago by dimpase

Someone working on innards of Singular might know. (I'd be surprised if it was changed).

Singular is not alone - this hack is also applied to libgap. I'm inclined to fix _get_shared_lib_filename so that it can accept other libdirs, not only SAGE_LOCAL/lib

Changed 14 months ago by dimpase

comment:17 in reply to: ↑ 16 Changed 14 months ago by mkoeppe

Replying to dimpase:

I'm inclined to fix _get_shared_lib_filename so that it can accept other libdirs, not only SAGE_LOCAL/lib

Yes, this function certainly would need changing if we continue to use it. For example, sysconfig.get_config_var('LIBDIR') gives a directory of the system python, not of the current venv.

But I think better solution would be to wrap loading of modules that are linked to singular by sys.setdlopenflags.

This is what pytorch does when it has to use RTLD_GLOBAL (see https://github.com/pytorch/pytorch/blob/master/torch/__init__.py#L132)

comment:19 Changed 14 months ago by mkoeppe

I have opened #30561 for this

comment:20 Changed 13 months ago by mkoeppe

  • Description modified (diff)
  • Milestone changed from sage-9.3 to sage-9.2
  • Summary changed from spkg-configure.m4 for singular to Make location of the Singular shared library configurable through sage-config

comment:21 Changed 13 months ago by mkoeppe

  • Description modified (diff)

comment:22 Changed 13 months ago by mkoeppe

  • Description modified (diff)

comment:23 Changed 12 months ago by mkoeppe

  • Cc gh-tobiasdiez added
  • Description modified (diff)

comment:24 Changed 12 months ago by mkoeppe

  • Description modified (diff)

comment:25 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:26 Changed 11 months ago by mkoeppe

  • Keywords sd111 added

Hoping we can make progress on this ticket this week - https://wiki.sagemath.org/days111

comment:27 Changed 10 months ago by mkoeppe

  • Cc arojas added

comment:28 Changed 7 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

comment:29 Changed 5 months ago by mkoeppe

  • Dependencies set to #31642

With #31642 taking us to a released version of Singular, it is time to work on an spkg-configure.m4 for using system singular. Arch already has the latest version (https://repology.org/project/singular/versions)

comment:30 Changed 4 months ago by mkoeppe

  • Summary changed from Make location of the Singular shared library configurable through sage-config to spkg-configure.m4 for singular

comment:31 Changed 3 months ago by mjo

The SINGULARPATH is being used to find the singular's docdir so that sage can manually parse its GNU info file and retrieve the docstring for Singular functions.

Currently the help file is mis-named (https://github.com/Singular/Singular/issues/1107), but even so, there's a much more reliable way to get this information without parsing the file ourselves (and therefore, without SINGULARPATH):

$ info "(singular.hlp)align" | tail -n+2

5.1.1 align
-----------

`*Syntax:*'
     `align (' vector_expression, int_expression `)'
     `align (' module_expression, int_expression `)'

`*Type:*'
     type of the first argument

`*Purpose:*'
     maps module generators `gen(i)' to `gen(i+s)' for all i.

`*Example:*'
            ring r=0,(x,y,z),(c,dp);
            align([1,2,3],3);
          ==> [0,0,0,1,2,3]
            align([0,0,1,2,3],-1);
          ==> [0,1,2,3]
            align(freemodule(2),1);
          ==> _[1]=[0,1]
          ==> _[2]=[0,0,1]

comment:32 follow-up: Changed 3 months ago by mjo

  • Branch set to u/mjo/ticket/29024
  • Commit set to a8610506141953cd344b699ca01e329c4a67fe2b

This branch is only half-baked, but it already eliminates problems 2, 3, and 4 from the description at the cost of requiring GNU Info to be installed.

Can the first issue be avoided as well? Why are we using dlopen to load SINGULAR_SO instead of just linking to it?

comment:33 in reply to: ↑ 32 Changed 3 months ago by mkoeppe

Replying to mjo:

Why are we using dlopen to load SINGULAR_SO instead of just linking to it?

See #30561

comment:34 Changed 3 months ago by mjo

  • Branch u/mjo/ticket/29024 deleted
  • Commit a8610506141953cd344b699ca01e329c4a67fe2b deleted

comment:35 Changed 3 months ago by mkoeppe

  • Dependencies changed from #31642 to #32302
  • Description modified (diff)

comment:36 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:37 Changed 2 months ago by mjo

  • Dependencies changed from #32302 to #32302, #31275

comment:38 Changed 2 months ago by mjo

  • Dependencies changed from #32302, #31275 to #32302, #31275, #32243

comment:39 Changed 2 months ago by mjo

  • Authors set to Michael Orlitzky
  • Branch set to u/mjo/ticket/29024
  • Commit set to a3b5983adf9e89e75ce3295f0ace0307b5cdb2d2
  • Status changed from new to needs_review

I've left the dlopen() alone because I don't feel like trying to eliminate it. This branch uses autoconf to find out the extension of shared libraries on the system (SHLIBEXT), and then tries to dlopen("libSingular.$SHLIBEXT"). If it works, we use the system copy, and use the same dlopen() strategy in singular.pyx. Otherwise we fall back to the SPKG and pass the full SAGE_LOCAL path to dlopen(). Works well enough on Gentoo.


New commits:

5fa1125Trac #32302: drop SINGULAR_EXECUTABLE from src/bin/sage-env.
5395116Trac #32243: look for cddlib-0.94m headers in the right place.
ea42a9bTrac #32243: patch gfan to look for cddlib headers in new location.
c0340b3Trac #29024: put the shared library extension in sage_conf.
3919061Trac #29024: new spkg-configure.m4 for singular.
fc4dc8cTrac #29024: don't set SINGULARPATH in sage-env.
ae26f76Trac #29024: use SINGULAR_LIB_BASE and SHLIBEXT to replace SINGULAR_SO.
a3b5983Trac #29024: add Gentoo package information.

comment:40 follow-up: Changed 2 months ago by dimpase

by the way, why there is no hint for pkgconfig in build/pkgs/pkgconfig/distros/gentoo.txt (and for many other obvious distros)?

I only notices as Gentoo's emerge was telling be about the switch to dev-util/pkgconf.

comment:41 follow-up: Changed 2 months ago by dimpase

OK, what's this:

[dochtml] make doc-inventory--Bug
[dochtml] cd /mnt/opt/Sage/sage-dev && ./sage --docbuild --no-pdf-links Bug inventory 
[dochtml] Bug >>Could not get expanded executable from "libSingular.so"<< at feResource.cc:434
[dochtml] 'Bug' is not a recognized document. Type 'sage --docbuild -D' for a list
[dochtml] of documents, or 'sage --docbuild --help' for more help.
[dochtml] make[5]: *** [Makefile:20: doc-inventory--Bug] Error 1

also,

$ ./sage
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 9.4.rc2, Release Date: 2021-08-12                 │
│ Using Python 3.8.9. Type "help()" for help.                        │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
Bug >>Could not get expanded executable from "libSingular.so"<< at feResource.cc:434
sage: 2+2                                                                                                                            
4
sage:           

comment:42 follow-up: Changed 2 months ago by dimpase

Can this be coming from Gentoo's Singular install? Weird.

comment:43 in reply to: ↑ 42 Changed 2 months ago by mjo

Replying to dimpase:

Can this be coming from Gentoo's Singular install? Weird.

Oh, right, I saw that too. Singular is trying to guess the path of its own executable and is failing. It doesn't hurt anything (that I noticed...), and is an upstream bug like the message says. I'll dig deeper as this gets closer to being merged and figure out what to report upstream.

comment:44 in reply to: ↑ 40 ; follow-up: Changed 2 months ago by mjo

Replying to dimpase:

by the way, why there is no hint for pkgconfig in build/pkgs/pkgconfig/distros/gentoo.txt (and for many other obvious distros)?

I only notices as Gentoo's emerge was telling be about the switch to dev-util/pkgconf.

The Sage "pkgconfig" package is a python module and not the tool itself. There's nothing we can do with it yet, but eventually it's dev-python/pkgconfig.

comment:45 Changed 2 months ago by mjo

You can make the BUG warning go away with,

$ SINGULAR_BIN_DIR=$(dirname $(type -P Singular)) sage

but hopefully that is not needed long-term.

comment:47 in reply to: ↑ 44 ; follow-up: Changed 2 months ago by mkoeppe

Replying to mjo:

The Sage "pkgconfig" package is a python module and not the tool itself. There's nothing we can do with it yet, but eventually it's dev-python/pkgconfig.

build/pkgs/pkgconf/distros also does not have an entry for gentoo (and neither for arch)

comment:48 Changed 2 months ago by mkoeppe

build/pkgs/singular/distros looks like it needs more entries for some distributions

comment:49 in reply to: ↑ 47 Changed 2 months ago by mjo

Replying to mkoeppe:

Replying to mjo:

The Sage "pkgconfig" package is a python module and not the tool itself. There's nothing we can do with it yet, but eventually it's dev-python/pkgconfig.

build/pkgs/pkgconf/distros also does not have an entry for gentoo (and neither for arch)

You can't bootstrap without pkgconf and you would be hard-pressed to find a Gentoo machine without it (being source-based), but it certainly can't hurt. The message that Dima saw says that we are getting rid of the old implementation, so now there's only dev-util/pkgconf to put in gentoo.txt.

comment:50 Changed 2 months ago by slelievre

  • Branch changed from u/mjo/ticket/29024 to u/slelievre/29024
  • Commit changed from a3b5983adf9e89e75ce3295f0ace0307b5cdb2d2 to a5cfcec18a1eb65068151189b22dd0ba461ca2cf

Here are Singular distro files for Arch Linux, Cygwin and Fedora.

Revert to your branch if those are not helping.


New commits:

a5cfcec29024: Add Singular distro info

comment:51 Changed 2 months ago by mkoeppe

-# As of 2021-03-20, build of homebrew's singular from source fails
+# [2021-03-20] building from source with Homebrew's singular fails

this rewording changes the meaning. What failed at this time was really the from-source build of the homebrew package (via brew install -s, or implied when using brew with a non-standard prefix)

comment:52 Changed 2 months ago by git

  • Commit changed from a5cfcec18a1eb65068151189b22dd0ba461ca2cf to fe67a2174fb5076228c13a8b6b5b94a697002348

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

fe67a2129024: Add Singular distro info

comment:53 in reply to: ↑ 41 Changed 2 months ago by mkoeppe

Replying to dimpase:

OK, what's this:

[dochtml] make doc-inventory--Bug

This is likely caused by unexpected output (on stdout!) from the command ./sage --docbuild --all-documents reference

comment:54 Changed 8 weeks ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:55 follow-up: Changed 6 weeks ago by dimpase

We probably also need to do something with SINGULARPATH, a variable hold, if needed, paths to Singular's .lib files, something that may be installed by Sage's optional packages - it is done e.g. by p_group_cohomology. So to make the latter work with system-wide Singular one needs to export SINGULARPATH=$SAGE_LOCAL/share/singular/LIB.

comment:56 in reply to: ↑ 55 Changed 6 weeks ago by mjo

Replying to dimpase:

We probably also need to do something with SINGULARPATH, a variable hold, if needed, paths to Singular's .lib files, something that may be installed by Sage's optional packages - it is done e.g. by p_group_cohomology. So to make the latter work with system-wide Singular one needs to export SINGULARPATH=$SAGE_LOCAL/share/singular/LIB.

What we really need is a system for optional packages to set their own environment variables, so we don't have to set every possible environment variable (for an unknowable number of optional packages) in sage-env. Of course Gentoo and everyone else already figured this out 20 years ago. We can have sage-env source any files in local/etc/env.d, and then tell optional packages to install what they need (e.g. a script that exports SINGULARPATH) there.

I guess I will waste my time reimplementing that so that some day I can stop wasting my time rebuilding the same copy of singular that I already have installed.

comment:57 Changed 6 weeks ago by mkoeppe

Best to plan to move away from any such dependence on setting environment variables.

Instead, it should be set at runtime when initializing the singular interface / libsingular.

Last edited 6 weeks ago by mkoeppe (previous) (diff)

comment:58 Changed 6 weeks ago by mkoeppe

(see #30818, #21707)

comment:59 follow-up: Changed 6 weeks ago by mkoeppe

In fact, do we actually install any custom Singular packages in SAGE_LOCAL/share/singular/LIB? I don't think we do.

src/sage/rings/function_field/function_field.py uses the correct approach -- it loads its custom Singular library using a fully qualified path to the library, which is just in the installed package data of the Sage library.

If the p_group_cohomology Sage code relies on SINGULARPATH, it should be changed so it uses the same approach.

comment:60 in reply to: ↑ 59 ; follow-ups: Changed 6 weeks ago by dimpase

Replying to mkoeppe:

In fact, do we actually install any custom Singular packages in SAGE_LOCAL/share/singular/LIB? I don't think we do.

Not sure what "we" is here, but p_group_cohomology does install a custom Singular lib, called dickson.lib, and it ends up in $SAGE_LOCAL/share/singular/LIB, as spkg-install has

sdh_install singular_helper/dickson.lib "$SAGE_SHARE/singular/LIB/"
  • which is fine if Singular is built by Sage, but not with the external Singular.

src/sage/rings/function_field/function_field.py uses the correct approach -- it loads its custom Singular library using a fully qualified path to the library, which is just in the installed package data of the Sage library.

If the p_group_cohomology Sage code relies on SINGULARPATH, it should be changed so it uses the same approach.

It doesn't rely on SINGULARPATH is seems to me. SINGULARPATH can be used by Singular, and system-wide Singular doesn't know about $SAGE_LOCAL/share/singular/LIB, obviously, unless something is done.

Should p_group_cohomology be using $SAGE_SHARE/singular/LIB/ to install dickson.lib, or some other location?

Regarding src/sage/ext_data/singular/function_field/core.lib, one sees

            from sage.env import SAGE_EXTCODE
            lib(SAGE_EXTCODE + '/singular/function_field/core.lib')

but

$ find . -name core.lib
./pkgs/sagemath-standard/build/lib.linux-x86_64-3.8/sage/ext_data/singular/function_field/core.lib
./src/sage/ext_data/singular/function_field/core.lib
./local/lib/python3.8/site-packages/sage/ext_data/singular/function_field/core.lib

so it also gets installed into site-packages. Is it a bug?

comment:61 in reply to: ↑ 60 Changed 6 weeks ago by mkoeppe

Replying to dimpase:

Regarding src/sage/ext_data/singular/function_field/core.lib, one sees

            from sage.env import SAGE_EXTCODE
            lib(SAGE_EXTCODE + '/singular/function_field/core.lib')

but

$ find . -name core.lib
./pkgs/sagemath-standard/build/lib.linux-x86_64-3.8/sage/ext_data/singular/function_field/core.lib
./src/sage/ext_data/singular/function_field/core.lib
./local/lib/python3.8/site-packages/sage/ext_data/singular/function_field/core.lib

so it also gets installed into site-packages.

core.lib is package_data of the distribution package sagemath-standard. As such it gets installed alongside the Python files, in site-packages.

This works as designed.

comment:62 in reply to: ↑ 60 ; follow-up: Changed 6 weeks ago by mkoeppe

Replying to dimpase:

Replying to mkoeppe:

In fact, do we actually install any custom Singular packages in SAGE_LOCAL/share/singular/LIB? I don't think we do.

Not sure what "we" is here

... any package in the Sage distribution ...

Should p_group_cohomology be using $SAGE_SHARE/singular/LIB/ to install dickson.lib, or some other location?

As I said, this should be changed to follow the same model as src/sage/ext_data/singular/function_field/core.lib

comment:63 follow-up: Changed 6 weeks ago by mkoeppe

Is there any code that actually loads dickson.lib?

comment:64 in reply to: ↑ 63 Changed 6 weeks ago by dimpase

Replying to mkoeppe:

Is there any code that actually loads dickson.lib?

sage -t src/sage/tests/modular_group_cohomology.py will trigger loading it, if p_group_cohomology is installed.

comment:65 in reply to: ↑ 62 Changed 6 weeks ago by dimpase

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

Replying to mkoeppe:

Replying to dimpase:

Replying to mkoeppe:

In fact, do we actually install any custom Singular packages in SAGE_LOCAL/share/singular/LIB? I don't think we do.

Not sure what "we" is here

... any package in the Sage distribution ...

Should p_group_cohomology be using $SAGE_SHARE/singular/LIB/ to install dickson.lib, or some other location?

As I said, this should be changed to follow the same model as src/sage/ext_data/singular/function_field/core.lib

I've opened https://github.com/sagemath/p_group_cohomology/issues/5 for this

comment:66 Changed 6 weeks ago by mkoeppe

As p_group_cohomology is broken since Sage 9.4 anyway (https://github.com/sagemath/p_group_cohomology/issues/4), I think we can proceed here on the ticket without worrying about SINGULARPATH.

comment:67 follow-up: Changed 5 weeks ago by dimpase

So are we waiting for the upstream fix here, or proceed with making sure that

export SINGULAR_BIN_DIR=$(dirname $(type -P Singular))

is added at the correct place?

comment:68 Changed 5 weeks ago by mjo

I think we'll have to add it. Upstream started to work on the issue but we're nowhere near having a complete fix in a released version of Singular. The BUG warning is harmless I think; but if we don't hide it, we're going to have to deal with a million bug reports about it.

I'll add the workaround and create a ticket to remove it whenever the fix is in an upstream release.

comment:69 Changed 5 weeks ago by mjo

Actually, I just looked at the upstream issue and see that I missed a notification about a new commit. Let me go test it. We may still want to add the workaround for a few months until a new release is made and distros catch up.

comment:70 in reply to: ↑ 67 ; follow-up: Changed 5 weeks ago by mkoeppe

Replying to dimpase:

So are we waiting for the upstream fix here, or proceed with making sure that

export SINGULAR_BIN_DIR=$(dirname $(type -P Singular))

is added at the correct place?

Can we put this environment setting in src/sage/libs/singular/singular.pyx (init_libsingular)?

comment:71 in reply to: ↑ 70 ; follow-ups: Changed 5 weeks ago by mjo

Replying to mkoeppe:

Replying to dimpase:

So are we waiting for the upstream fix here, or proceed with making sure that

export SINGULAR_BIN_DIR=$(dirname $(type -P Singular))

is added at the correct place?

Can we put this environment setting in src/sage/libs/singular/singular.pyx (init_libsingular)?

Both type -P and command -v are shell built-ins on my system, making them a bit hard to use from python code. Does putting it in sage-env cause a problem I've overlooked, or are you just thinking it would be more appropriate in init_singular()? I don't disagree -- but considering that this is a temporary workaround, it might just be easier to add the one line in sage-env instead of the python code to launch a full-blown shell and parse its output.

comment:72 in reply to: ↑ 71 Changed 5 weeks ago by mkoeppe

Replying to mjo:

Does putting it in sage-env cause a problem I've overlooked

Yes, we are working on making sage pip-installable and importable into normal Python environments (import sage.all), and for this depending on sage-env needs to be removed

comment:73 in reply to: ↑ 71 Changed 5 weeks ago by mkoeppe

Replying to mjo:

temporary workaround

Well, versions of singular that need this workaround may be in relevant distributions for years...

comment:75 in reply to: ↑ 74 Changed 5 weeks ago by mjo

  • Branch changed from u/slelievre/29024 to u/mjo/ticket/29024
  • Commit changed from fe67a2174fb5076228c13a8b6b5b94a697002348 to 3cebd4f26aba00ed35aa6956e2a69e323353a4a3

Replying to mkoeppe:

And I think this just https://docs.python.org/3/library/shutil.html#shutil.which

Yep, here's an untested branch. It's going to take me all night to rebuild sage after the rebase but I'll make sure it works in the morning. And report back to upstream... that last commit looks strange to me.


New commits:

28837b9Trac #29024: put the shared library extension in sage_conf.
c383777Trac #29024: new spkg-configure.m4 for singular.
8e80875Trac #29024: don't set SINGULARPATH in sage-env.
9b8244fTrac #29024: use SINGULAR_LIB_BASE and SHLIBEXT to replace SINGULAR_SO.
f6e4f89Trac #29024: add Gentoo package information.
105524729024: Add Singular distro info
3cebd4fTrac #29024: work around Singular issue 1113 in init_libsingular().

comment:76 Changed 5 weeks ago by mjo

It works.

comment:77 Changed 5 weeks ago by mkoeppe

Ready for review?

comment:78 Changed 5 weeks ago by mjo

  • Authors changed from Michael Orlitzky to Michael Orlitzky, Samuel Lelièvre
  • Status changed from needs_work to needs_review

comment:79 Changed 5 weeks ago by mkoeppe

This will need a GH Actions run

comment:81 Changed 5 weeks ago by dimpase

  • Reviewers set to Dima Pasechnik, https://github.com/sagemath/sagetrac-mirror/actions/runs/1240853423

comment:82 follow-up: Changed 5 weeks ago by mkoeppe

Doesn't accept singular on cygwin-standard (https://github.com/mkoeppe/sage/runs/3625055086)

-----------------------------------------------------------------------------
Checking whether SageMath should install SPKG singular...
checking whether any of gmp mpir ntl flint readline mpfr cddlib is installed as or will be installed as SPKG... no
checking for Singular... /usr/bin/Singular
checking for SINGULAR... no
checking if we can dlopen(libSingular.dll)... configure: no suitable system package found for SPKG singular

comment:83 Changed 5 weeks ago by mkoeppe

Works OK on archlinux-latest-standard (https://github.com/mkoeppe/sage/runs/3625059112)

----------------------------------------------------------------------------
Checking whether SageMath should install SPKG singular...
checking whether any of gmp mpir ntl flint readline mpfr cddlib is installed as or will be installed as SPKG... no
checking for Singular... /usr/sbin/Singular
checking for SINGULAR... yes
checking if we can dlopen(libSingular.so)... configure: will use system package and not install SPKG singular

(but looks like an AC_MSG_RESULT is missing)

comment:84 Changed 5 weeks ago by mkoeppe

Also OK (as expected) on gentoo-standard

No luck on debian, fedora, and homebrew because of the cddlib dependency. It works correctly but does not accept system singular.

Last edited 5 weeks ago by mkoeppe (previous) (diff)

comment:85 in reply to: ↑ 82 Changed 5 weeks ago by mjo

Replying to mkoeppe:

Doesn't accept singular on cygwin-standard (https://github.com/mkoeppe/sage/runs/3625055086)

I think that's expected, since it has an older version of Singular than the pkg-config test looks for.

comment:86 Changed 5 weeks ago by git

  • Commit changed from 3cebd4f26aba00ed35aa6956e2a69e323353a4a3 to d8118acd08ade1751ad1ecd8394415d32a643c2d

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

d8118acTrac #29024: add missing AC_MSG_RESULTs in singular/spkg-configure.m4.

comment:87 Changed 5 weeks ago by mkoeppe

  • Reviewers changed from Dima Pasechnik, https://github.com/sagemath/sagetrac-mirror/actions/runs/1240853423 to Dima Pasechnik, Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:88 Changed 5 weeks ago by mkoeppe

  • Status changed from positive_review to needs_work

Actually this needs more work on Cygwin: https://github.com/mkoeppe/sage/runs/3638292833?check_suite_focus=true

./sage -t -p --all --logfile=logs/ptest.log
no stored timings available
Running doctests with ID 2021-09-18-08-09-52-cb78f982.
Git branch: HEAD
Using --optional=build,cygwin,dochtml,pip,sage,sage_spkg
Doctesting entire Sage library.
Sorting sources by runtime so that slower doctests are run first....
Doctesting 4363 files using 3 threads.
Traceback (most recent call last):
  File "/cygdrive/d/a/sage/sage/src/bin/sage-runtests", line 144, in <module>
    err = DC.run()
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/doctest/control.py", line 1207, in run
    self.run_doctests()
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/doctest/control.py", line 904, in run_doctests
    self.dispatcher = DocTestDispatcher(self)
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/doctest/forker.py", line 1624, in __init__
    init_sage(controller)
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/doctest/forker.py", line 192, in init_sage
    controller.load_environment()
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/doctest/control.py", line 525, in load_environment
    return import_module(self.options.environment)
  File "/usr/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 848, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/repl/ipython_kernel/all_jupyter.py", line 5, in <module>
    from sage.all_cmdline import *
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/all_cmdline.py", line 19, in <module>
    from sage.all import *
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/all.py", line 141, in <module>
    from sage.rings.all      import *
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/rings/all.py", line 83, in <module>
    from .polynomial.all import *
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/rings/polynomial/all.py", line 42, in <module>
    from sage.rings.polynomial.laurent_polynomial_ring import LaurentPolynomialRing
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/rings/polynomial/laurent_polynomial_ring.py", line 50, in <module>
    from sage.rings.polynomial.laurent_polynomial import LaurentPolynomial_mpair, LaurentPolynomial_univariate
  File "sage/rings/polynomial/laurent_polynomial.pyx", line 1, in init sage.rings.polynomial.laurent_polynomial (build/cythonized/sage/rings/polynomial/laurent_polynomial.c:40817)
  File "sage/matrix/matrix0.pyx", line 28, in init sage.matrix.matrix0 (build/cythonized/sage/matrix/matrix0.c:44570)
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/modules/free_module.py", line 167, in <module>
    import sage.matrix.matrix_space
  File "/opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/python3.8/site-packages/sage/matrix/matrix_space.py", line 55, in <module>
    from . import matrix_mpolynomial_dense
  File "sage/matrix/matrix_mpolynomial_dense.pyx", line 1, in init sage.matrix.matrix_mpolynomial_dense (build/cythonized/sage/matrix/matrix_mpolynomial_dense.cpp:8666)
  File "sage/rings/polynomial/multi_polynomial_libsingular.pyx", line 1, in init sage.rings.polynomial.multi_polynomial_libsingular (build/cythonized/sage/rings/polynomial/multi_polynomial_libsingular.cpp:49840)
  File "sage/libs/singular/singular.pyx", line 808, in init sage.libs.singular.singular (build/cythonized/sage/libs/singular/singular.cpp:11287)
  File "sage/libs/singular/singular.pyx", line 785, in sage.libs.singular.singular.init_libsingular (build/cythonized/sage/libs/singular/singular.cpp:8697)
ImportError: cannot load Singular library from /opt/sage-032178416da49f83f86c3b8ce2797134fbb97bd8/lib/libSingular.dll (b'No such file or directory')
make: *** [Makefile:233: ptest-nodoc] Error 1

Note that on cygwin, dlls are installed in bin/, not in lib/

comment:89 Changed 5 weeks ago by git

  • Commit changed from d8118acd08ade1751ad1ecd8394415d32a643c2d to da802ef6c996c37611ef0be910b714e41acc68b7

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

fbf01c4Trac #29024: support libdir substitutions in sage_conf.
fc2adfbTrac #29024: use new SAGE_LIBDIR substitution for libSingular.so.
da802efTrac #29024: clarify a comment in singular's spkg-configure.

comment:90 Changed 5 weeks ago by mjo

  • Status changed from needs_work to needs_review

I think that will fix it. I thought about eliminating SINGULAR_LIB_BASE entirely, but I can't say for sure that we won't encounter a platform where dlopen() does not search the library path, so I've left it in and invented a way to reference the libdir.

The hacks in sage_conf are starting to pile up, so I've opened #32540 with a proposal to refactor things.

comment:91 Changed 5 weeks ago by mkoeppe

Have you tested this on Cygwin?

comment:92 follow-up: Changed 5 weeks ago by mkoeppe

Asking because the situation with DLLs on Cygwin is not really related to anything "LIBDIR". Only the naked DLLs end up in bin/ on the platform, but lib/ is still used for .a and .dll.a files

comment:93 follow-up: Changed 5 weeks ago by dimpase

Homebrew's Singular needs some fixes; 1st of all,

  • .homebrew-build-env

    a b export PKG_CONFIG_PATH 
    2323LIBRARY_PATH="$HOMEBREW/lib$LIBRARY_PATH"
    2424[ -z "$CPATH" ] || CPATH=":${CPATH}"
    2525CPATH="$HOMEBREW/include$CPATH"
    26 for l in readline bzip2 ntl; do
     26for l in readline bzip2 ntl singular; do
    2727    if [ -d "$HOMEBREW/opt/$l/lib" ]; then
    2828        LIBRARY_PATH="$HOMEBREW/opt/$l/lib:$LIBRARY_PATH"
    2929    fi

Then, it appears that dlopen() call in singular.pyx needs a full path to .dylib on macOS. (it might be a Cython bug, or the plarform limitation, I don't know). That is, if I make sure that

SINGULAR_LIB_BASE = "/usr/local/opt/singular/lib/libSingular"

in sage_conf.py then everything works. I am not sure how to achieve the latter automatically. One way might be to export a special env. var. in .homebrew-build-env and use it to set up the proper SINGULAR_LIB_BASE in spkg-configure.m4 - but there could be a better way.

comment:94 Changed 5 weeks ago by dimpase

Once the latter is fixed, we should apply

  • build/pkgs/singular/distros/homebrew.txt

    a b  
    1 # [2021-03-20] Homebrew's Singular can be built from source
    2 # (via `brew install -s`, or implied when using `brew` with
    3 # a non-standard prefix), but that currently fails.
    4 
    5 #singular
     1singular

comment:95 follow-up: Changed 5 weeks ago by arojas

  • Status changed from needs_review to needs_work

This breaks distro installs without sage_conf. Please keep the old _get_shared_lib_path code path as a fallback in case sage_conf gives an ImportError.

comment:96 in reply to: ↑ 95 ; follow-up: Changed 5 weeks ago by dimpase

Replying to arojas:

This breaks distro installs without sage_conf. Please keep the old _get_shared_lib_path code path as a fallback in case sage_conf gives an ImportError.

Do distros use env. vars from env.py? (That's where the change was, the env. var. there is not computed any more.

comment:97 in reply to: ↑ 96 Changed 5 weeks ago by arojas

Replying to dimpase:

Replying to arojas:

This breaks distro installs without sage_conf. Please keep the old _get_shared_lib_path code path as a fallback in case sage_conf gives an ImportError.

Do distros use env. vars from env.py? (That's where the change was, the env. var. there is not computed any more.

Yes, env.py is part of sagelib. Actually, a simple fallback to SINGULAR_LIB_PATH="libSingular.so" would be fine for me (if any distro modifies the default library name, I guess it's their job to patch sage accordingly or provide a custom sage_conf)

comment:98 in reply to: ↑ 92 ; follow-up: Changed 5 weeks ago by mjo

Replying to mkoeppe:

Asking because the situation with DLLs on Cygwin is not really related to anything "LIBDIR". Only the naked DLLs end up in bin/ on the platform, but lib/ is still used for .a and .dll.a files

No, I haven't tested yet. I'll have to run it on GH.

Even if $libdir isn't being used... at some point, the build system is deciding to put those dlls into a $directory, and $directory=bin on cygwin while $directory=lib everywhere else, no? I don't suppose you know where that decision is made?

comment:99 in reply to: ↑ 93 ; follow-up: Changed 5 weeks ago by mjo

Replying to dimpase:

Then, it appears that dlopen() call in singular.pyx needs a full path to .dylib on macOS. (it might be a Cython bug, or the plarform limitation, I don't know).

This was a conscious choice, as da802ef now makes explicit. POSIX doesn't say what will happen if you use a filename without a path in dlopen(), so the SPKG is still used as a fallback on systems where it doesn't work.

There are a couple options to improve this, of varying difficulty:

  • We could try to get the libdir from Singular itself. The feResource.cc file provides a bunch of other configuration information, but not the bare libdir at the moment. We could provide a patch upstream that lets us obtain its libdir. (However, the cygwin discussion is suggesting that libdir may not always be the right place?)
  • We can eliminate the dlopen(), if possible. We already link directly to libSingular and that's easy to do portably with -lSingular. I'm not entirely sure why dlopen() is needed, but the init_libsingular() documentation says
    """                                                                                                                                                                      
    This initializes the SINGULAR library. This is a hack to some                                                                                                            
    extent.                                                                                                                                                                  
                                                                                                                                                                                 
    SINGULAR has a concept of compiled extension modules similar to                                                                                                          
    Sage. For this, the compiled modules need to see the symbols from                                                                                                        
    the main program. However, SINGULAR is a shared library in this                                                                                                          
    context these symbols are not known globally. The work around so                                                                                                         
    far is to load the library again and to specify ``RTLD_GLOBAL``.                                                                                                         
    """
    
  • We can invent some hacks that guess the right location on most of the platforms we support.

comment:100 in reply to: ↑ 98 ; follow-up: Changed 5 weeks ago by dimpase

Replying to mjo:

Even if $libdir isn't being used... at some point, the build system is deciding to put those dlls into a $directory, and $directory=bin on cygwin while $directory=lib everywhere else, no? I don't suppose you know where that decision is made?

I think it has to do with cygwin fork() design, Windows address space, rebasing... DLL needs to get a fixed memory address allocation, for fork() to work), and so bin/ is what's beging scanned by the corresponding Cygwin utility rebaseall.

Not running away screaming yet?

comment:101 in reply to: ↑ 99 ; follow-up: Changed 5 weeks ago by dimpase

Replying to mjo:

Replying to dimpase:

Then, it appears that dlopen() call in singular.pyx needs a full path to .dylib on macOS. (it might be a Cython bug, or the plarform limitation, I don't know).

This was a conscious choice, as da802ef now makes explicit. POSIX doesn't say what will happen if you use a filename without a path in dlopen(), so the SPKG is still used as a fallback on systems where it doesn't work.

This does not explain why no-full-path dlopen() succeeds in the test

configure:40299: checking if we can dlopen(libSingular.dylib)
configure:40330: gcc -o conftest -g -O2   -Ddarwin -I/usr/local/opt/gmp/include  -I/usr/local/Cellar/ecl/21.2.1/include  conftest.c -lmpfi -lmpc -lnauty -lglpk -lpari -lncurses -lcurl -lcddgmp -lbz2 -larb -lflint -lmpfr -lgmp -lm  -lntl  -lppl -lgmpxx -lgmp -ldl >&5
configure:40330: $? = 0
configure:40330: ./conftest
configure:40330: $? = 0
configure:40333: result: yes

but fails in singular.pyx.

comment:102 in reply to: ↑ 100 Changed 5 weeks ago by mjo

Replying to dimpase:

Replying to mjo:

Even if $libdir isn't being used... at some point, the build system is deciding to put those dlls into a $directory, and $directory=bin on cygwin while $directory=lib everywhere else, no? I don't suppose you know where that decision is made?

I think it has to do with cygwin fork() design, Windows address space, rebasing... DLL needs to get a fixed memory address allocation, for fork() to work), and so bin/ is what's beging scanned by the corresponding Cygwin utility rebaseall.

Not running away screaming yet?

Is that an option? Can I wrap the whole thing in if [ $(whoami) = "mjo" ]?

The rebaseall thing makes some amount of sense to me (I used cygwin back when Windows XP was around), but I'm still not sure exactly when the dlls either get installed directly to bin, or moved from lib to bin.

comment:103 in reply to: ↑ 101 ; follow-up: Changed 5 weeks ago by mjo

Replying to dimpase:

This does not explain why no-full-path dlopen() succeeds in the test... but fails in singular.pyx.

Oh, no. That's interesting. What error message does it give you?

comment:104 in reply to: ↑ 103 Changed 5 weeks ago by dimpase

Replying to mjo:

Replying to dimpase:

This does not explain why no-full-path dlopen() succeeds in the test... but fails in singular.pyx.

Oh, no. That's interesting. What error message does it give you?

...
--> 808 init_libsingular()
...
    782     handle = dlopen(lib, RTLD_GLOBAL|RTLD_LAZY)
    783     if not handle:
    784         err = dlerror()
--> 785         raise ImportError(f"cannot load Singular library from {SINGULAR_LIB_PATH} ({err})")
        global ImportError = undefined
    786 
    787     # load SINGULAR
    788     siInit(lib)
    789 
...
ImportError: cannot load Singular library from libSingular.dylib (b'dlopen(libSingular.dylib, 9): image not found')

I tried setting LD_LIBRARY_PATH or DYLD_FALLBACK_LIBRARY_PATH (cf. Apple's manpage on dlopen()), to no effect.

PS. related reading https://developer.apple.com/forums/thread/13161 sounds as if Apple broke the latter stuff in the name of "security"...

Last edited 5 weeks ago by dimpase (previous) (diff)

comment:105 Changed 5 weeks ago by mjo

The cygwin magic happens in libtool.m4:

cygwin* | mingw* | pw32* | cegcc*)
...
  # DLL is installed to $(libdir)/../bin by postinstall_cmds                                                                                                               
  postinstall_cmds='base_file=`basename \$file`~
    dlpath=`$SHELL 2>&1 -c '\''. $dir/'\''\$base_file'\''i; echo \$dlname'\''`~
    dldir=$destdir/`dirname \$dlpath`~
    test -d \$dldir || mkdir -p \$dldir~
    $install_prog $dir/$dlname \$dldir/$dlname~
    chmod a+x \$dldir/$dlname~
    if test -n '\''$stripme'\'' && test -n '\''$striplib'\''; then
      eval '\''$striplib \$dldir/$dlname'\'' || exit \$?;
    fi'

I guess I'll just undo the last few commits and special-case cygwin*.

comment:106 Changed 5 weeks ago by dimpase

As far as I know, on Cygwin dlopen() looks up things in $PATH rather than $LD_..., so this appears to be good as it is, no?

Although Conda and macOS still need an action.

comment:107 Changed 5 weeks ago by git

  • Commit changed from da802ef6c996c37611ef0be910b714e41acc68b7 to 8cd60dde05a5421500ddcfdce3f48d1a3efe8a14

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

c3e635aTrac #29024: clarify a comment in singular's spkg-configure.
8cd60ddTrac #29024: fix libSingular path on Cygwin.

comment:108 follow-up: Changed 5 weeks ago by mjo

On cygwin the SPKG will always be used because its Singular is too old. But even that was failing, because the absolute path involving "lib" was wrong. It should be fixed now.

Remaining questions:

  1. Do I need to use "cygSingular" as the library name on Cygwin?
  2. WTF is happening on macOS?

comment:109 in reply to: ↑ 108 Changed 5 weeks ago by dimpase

Replying to mjo:

On cygwin the SPKG will always be used because its Singular is too old.

I've asked for an update: https://cygwin.com/pipermail/cygwin/2021-September/249449.html

But even that was failing, because the absolute path involving "lib" was wrong. It should be fixed now.

Remaining questions:

  1. Do I need to use "cygSingular" as the library name on Cygwin?

Yes, I think so.

  1. WTF is happening on macOS?

I suppose we need to supply the full path to DLL (.e.g. .dylib), via an env. var in .homebrew-build-env and forget about it. (Not sure about Conda).

One discrepancy I noticed is that that singular.pyx is compiled as C++, while the test in spkg-configure is done with plain C; I tried with C++, saw no difference.

Last edited 5 weeks ago by dimpase (previous) (diff)

comment:110 Changed 5 weeks ago by dimpase

  • Branch changed from u/mjo/ticket/29024 to u/dimpase/ticket/29024
  • Commit changed from 8cd60dde05a5421500ddcfdce3f48d1a3efe8a14 to f72df456ba4c29b5b5353948af02d3ed9a8a66d3

Homebrew fix


New commits:

f72df45macOS dlopen() prefers full path sometimes

comment:111 Changed 5 weeks ago by mkoeppe

This is not a good change.

--- a/.homebrew-build-env
+++ b/.homebrew-build-env
@@ -23,7 +23,7 @@ export PKG_CONFIG_PATH
 LIBRARY_PATH="$HOMEBREW/lib$LIBRARY_PATH"
 [ -z "$CPATH" ] || CPATH=":${CPATH}"
 CPATH="$HOMEBREW/include$CPATH"
-for l in readline bzip2 ntl; do
+for l in readline bzip2 ntl singular; do
     if [ -d "$HOMEBREW/opt/$l/lib" ]; then
         LIBRARY_PATH="$HOMEBREW/opt/$l/lib:$LIBRARY_PATH"
     fi

Adding to LIBRARY_PATH is only needed for "keg-only" packages. singular is a regular package, linked into /usr/local.

@@ -45,3 +45,5 @@ for l in gettext; do
     fi
 done
 export ACLOCAL_PATH
+SINGULAR_HOMEBREW_LIBDIR="$HOMEBREW/opt/singular/lib/"
+export SINGULAR_HOMEBREW_LIBDIR

Pretty sure you can find the information you need during configure using pkg-config.

comment:112 Changed 5 weeks ago by dimpase

Indeed, we can instead call PKG_CHECK_VAR([SINGULAR_LIBDIR], [Singular], [libdir]) to get SINGULAR_LIBDIR. I suppose, to be on the safe side we should prepend it to SINGULAR_LIB_BASE unconditionally, not only on macOS.

comment:113 Changed 5 weeks ago by git

  • Commit changed from f72df456ba4c29b5b5353948af02d3ed9a8a66d3 to f11791b35962276ae007802d5397572a58a0e3bd

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

f11791bpkg-config to get the libdir, properly nest tests

comment:114 follow-up: Changed 5 weeks ago by mjo

I was working on this at the same time =(

https://git.sagemath.org/sage.git/commit/?h=u/mjo/ticket/29024&id=5a494d5a6b96949c5744239a0c8f866f3c2d9ac2

I've simplified the variables a bit, so the "base" and extension are no longer separate. I also added the special-case for cygwin into the system-singular code branch.

comment:115 Changed 4 weeks ago by git

  • Commit changed from f11791b35962276ae007802d5397572a58a0e3bd to e6c41f7d7d13fca5dd6f2802c75e8c829b040e92

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

e6c41f7Fedora needs devel pkg

comment:116 in reply to: ↑ 114 Changed 4 weeks ago by dimpase

Replying to mjo:

I was working on this at the same time =(

https://git.sagemath.org/sage.git/commit/?h=u/mjo/ticket/29024&id=5a494d5a6b96949c5744239a0c8f866f3c2d9ac2

I've simplified the variables a bit, so the "base" and extension are no longer separate. I also added the special-case for cygwin into the system-singular code branch.

I'll try to merge your improvements

comment:117 Changed 4 weeks ago by gh-dkwo

  • Cc gh-dkwo added

comment:118 Changed 4 weeks ago by dimpase

  • Dependencies changed from #32302, #31275, #32243 to #32302, #31275, #32243, #32549

there is also #32549 removing mpir spkg, so mpir should disappear in spkg-config.m4

comment:119 Changed 3 weeks ago by dimpase

  • Dependencies changed from #32302, #31275, #32243, #32549 to #32549

comment:120 Changed 3 weeks ago by git

  • Commit changed from e6c41f7d7d13fca5dd6f2802c75e8c829b040e92 to 3404652e8897d662a83953c32d9b1ef8266ef443

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

d5b8ac8Trac #29024: add Gentoo package information.
9589a1d29024: Add Singular distro info
c76a975Trac #29024: work around Singular issue 1113 in init_libsingular().
3730d72Trac #29024: add missing AC_MSG_RESULTs in singular/spkg-configure.m4.
30e7fd6Trac #29024: clarify a comment in singular's spkg-configure.
f64594cTrac #29024: fix libSingular path on Cygwin.
73d301dmacOS dlopen() prefers full path sometimes
604b0c4pkg-config to get the libdir, properly nest tests
ca4aa03Fedora needs devel pkg
3404652dependance on gmp/mpir already covered by mpfr

comment:121 Changed 3 weeks ago by dimpase

  • Dependencies #32549 deleted

comment:122 Changed 8 days ago by git

  • Commit changed from 3404652e8897d662a83953c32d9b1ef8266ef443 to d8db64bda6e1939e1af277b2925dddd0f8167d30

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

a61b3a5Trac #29024: add Gentoo package information.
fa65fa529024: Add Singular distro info
0846173Trac #29024: work around Singular issue 1113 in init_libsingular().
dd69ac1Trac #29024: add missing AC_MSG_RESULTs in singular/spkg-configure.m4.
9d6080fTrac #29024: clarify a comment in singular's spkg-configure.
e0c0e05Trac #29024: fix libSingular path on Cygwin.
93b2f47macOS dlopen() prefers full path sometimes
dca5125pkg-config to get the libdir, properly nest tests
f85d09cFedora needs devel pkg
d8db64bdependance on gmp/mpir already covered by mpfr

comment:123 Changed 8 days ago by dimpase

so what's exactly is different in functionality in the current ticket branch, u/dimpase/ticket/29024 vs u/mjo/ticket/29024 ?

comment:124 Changed 8 days ago by mjo

Now that we're just asking pkg-config for the correct location, the two variables SINGULAR_LIB_BASE and SINGULAR_LIB_FILE are redundant. (The "base" variable would be left empty in the past to let dlopen() search for the library.) I think the main difference in my branch is that I've refactored things to use only a single variable, LIBSINGULAR_PATH. The rest should be cosmetic.

comment:125 follow-up: Changed 7 days ago by dimpase

you do non-nested calls where they can and should be nested. slows down ./configure for no reason.

comment:126 in reply to: ↑ 125 ; follow-up: Changed 7 days ago by mjo

Replying to dimpase:

you do non-nested calls where they can and should be nested. slows down ./configure for no reason.

I find the logic much easier to understand when the "else" branch of a test is not separated from the test by an entire page. Ideally I would be able to put something like return; after each sage_spkg_install_singular=yes, but I don't know of a reliable way to accomplish that.

Running "make" takes many hours, so an extra second or two on systems without Singular didn't seem like a huge problem. And those of us who work on the build system and run ./configure repeatedly should be clever enough to install it. But I could still nest them if that's a deal-breaker for you.

comment:127 in reply to: ↑ 126 Changed 4 days ago by dimpase

Replying to mjo:

Replying to dimpase:

you do non-nested calls where they can and should be nested. slows down ./configure for no reason.

I find the logic much easier to understand when the "else" branch of a test is not separated from the test by an entire page.

Do some coding in Lisp, then, for the training purposes ;-)

Ideally I would be able to put something like return; after each sage_spkg_install_singular=yes, but I don't know of a reliable way to accomplish that.

Running "make" takes many hours, so an extra second or two on systems without Singular didn't seem like a huge problem. And those of us who work on the build system and run ./configure repeatedly should be clever enough to install it. But I could still nest them if that's a deal-breaker for you.

Also, nesting makes sure that the output in case of errors is meaningful - it just errors on the 1st failing place, and that's it.

comment:128 Changed 4 days ago by dimpase

I'm willing to review a branch with nesting - or, you can review my branch...

comment:129 Changed 3 days ago by mjo

  • Branch changed from u/dimpase/ticket/29024 to u/mjo/ticket/29024
  • Commit changed from d8db64bda6e1939e1af277b2925dddd0f8167d30 to eb3b4e559096605857584054ecdfe3556e6444ed
  • Status changed from needs_work to needs_review

Here it is fully nested. I cherry-picked your fix for fedora.txt, but I left "gmp" in the DEPCHECK because it's easier to just list them all than it is to try to keep track of which DEPCHECKs need to be modified when e.g. mpfr loses a dependency.


Last 10 new commits:

dfb5860Trac #29024: add Gentoo package information.
175e6f629024: Add Singular distro info
dc1dcc3Trac #29024: work around Singular issue 1113 in init_libsingular().
ef79af8Trac #29024: add missing AC_MSG_RESULTs in singular/spkg-configure.m4.
2f32b69Trac #29024: clarify a comment in singular's spkg-configure.
8ec888aTrac #29024: fix libSingular path on Cygwin.
62065deTrac #29024: always obtain the absolute path to libSingular.
2aa55e7Trac #29024: fully nest tests in Singular's spkg-configure.m4.
eecd43cFedora needs devel pkg
eb3b4e5Trac #29024: drop "mpir" from singular's DEPCHECK.

comment:130 Changed 3 days ago by dimpase

build/pkgs/singular/distros/homebrew.txt should be like in my branch:

  • build/pkgs/singular/distros/homebrew.txt

    a b  
    1 # [2021-03-20] Homebrew's Singular can be built from source
    2 # (via `brew install -s`, or implied when using `brew` with
    3 # a non-standard prefix), but that currently fails.
    4 
    5 #singular
     1singular

comment:131 follow-up: Changed 3 days ago by git

  • Commit changed from eb3b4e559096605857584054ecdfe3556e6444ed to a3c53051a5961226700bdb8a40f831092f8dffcd

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

a3c5305Trac #29024: re-enable the system singular hint on homebrew.

comment:132 Changed 3 days ago by dimpase

  • Status changed from needs_review to positive_review

lgtm

comment:133 follow-up: Changed 3 days ago by arojas

  • Status changed from positive_review to needs_work

comment:95 hasn't been addressed. This breaks sage_conf-less installs.

comment:134 in reply to: ↑ 133 ; follow-up: Changed 3 days ago by dimpase

  • Status changed from needs_work to positive_review

Replying to arojas:

comment:95 hasn't been addressed. This breaks sage_conf-less installs.

Please open a followup ticket. It's not clear to me how or what exactly needs to be be fixed.

comment:135 in reply to: ↑ 131 Changed 3 days ago by mkoeppe

  • Status changed from positive_review to needs_work

Replying to git:

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

a3c5305Trac #29024: re-enable the system singular hint on homebrew.

This needs testing on GH Actions.

comment:136 Changed 3 days ago by dimpase

needs work ? On what exactly?

comment:137 Changed 3 days ago by dimpase

I suggest we run tests on the followup ticket for sage_conf-less installs.

The most one might need to adjust here is the list of distros on which this works.

comment:138 in reply to: ↑ 134 ; follow-up: Changed 3 days ago by arojas

Replying to dimpase:

It's not clear to me how or what exactly needs to be be fixed.

This unconditionally imports sage_conf so it breaks on systems without it. As I said in comment:97 adding a simple fallback should fix it, here is a patch that works for me

  • src/sage/libs/singular/singular.pyx

    diff --git a/src/sage/libs/singular/singular.pyx b/src/sage/libs/singular/singular.pyx
    index ea6d0ab01d..8dfa490de5 100644
    a b cdef init_libsingular(): 
    768768
    769769    cdef void *handle = NULL
    770770
    771     from sage_conf import LIBSINGULAR_PATH
     771    try:
     772        from sage_conf import LIBSINGULAR_PATH
     773    except ImportError:
     774    # No sage_conf, assume everything is properly set up by the distro
     775        LIBSINGULAR_PATH="libSingular.so"
    772776    lib = str_to_bytes(LIBSINGULAR_PATH, FS_ENCODING, "surrogateescape")
    773777
    774778    # This is a workaround for https://github.com/Singular/Singular/issues/1113

comment:139 Changed 3 days ago by dimpase

  • Status changed from needs_work to needs_review

comment:140 in reply to: ↑ 138 Changed 3 days ago by mkoeppe

  • Status changed from needs_review to needs_work

Replying to arojas:

Replying to dimpase:

It's not clear to me how or what exactly needs to be be fixed.

This unconditionally imports sage_conf so it breaks on systems without it. As I said in comment:97 adding a simple fallback should fix it, here is a patch that works for me

  • src/sage/libs/singular/singular.pyx

    diff --git a/src/sage/libs/singular/singular.pyx b/src/sage/libs/singular/singular.pyx
    index ea6d0ab01d..8dfa490de5 100644
    a b cdef init_libsingular(): 
    768768
    769769    cdef void *handle = NULL
    770770
    771     from sage_conf import LIBSINGULAR_PATH
     771    try:
     772        from sage_conf import LIBSINGULAR_PATH
     773    except ImportError:
     774    # No sage_conf, assume everything is properly set up by the distro
     775        LIBSINGULAR_PATH="libSingular.so"
    772776    lib = str_to_bytes(LIBSINGULAR_PATH, FS_ENCODING, "surrogateescape")
    773777
    774778    # This is a workaround for https://github.com/Singular/Singular/issues/1113

Actually this should be rewritten so that it goes through a variable declared in sage.env instead of using sage_conf directly.

sage.env already takes care of conditional import from sage_conf.

Note: See TracTickets for help on using tickets.