SINGULAR_SO default is incorrect of Cygwin

[embray]

It should be under $SAGE_LOCAL/bin/cygSingular-<version>.dll. Rather than guess at the version the attached patch just takes the latest one (which will normally be the only one). It's pretty hacky, but then so is this code on other platforms too.

For the purposes of the standard Sage distribution this should generally work though.

Component: porting: Cygwin

Author: Erik Bray, Jeroen Demeyer

Branch/Commit: cb3dadf

Reviewer: Jeroen Demeyer, Volker Braun

Issue created by migration from https://trac.sagemath.org/ticket/22628

[jdemeyer]

comment:2

So on Cygwin there is no "symlink" cygSingular.dll -> cygSingular-VERSION.dll? It would be simpler if there would be. It would also be simpler if cygSingular.dll would just be called libSingular.dll. Is there any chance of getting done in upstream Singular? Then the same code could be used on all platforms.

[embray]

comment:3

Generally DLLs do not go in /lib on Cygwin. The reason for this is that Windows searches $PATH for DLLs. Further, the default way Cygwin implements symlinks is as normal files. So Windows would see a symlink named cygSingular.dll and try to load the actual file cygSingular.dll as a DLL and fail.

In any case, I think it's better to explicitly point to the platform-specific location of a shared library.

(In fact, rather than hard-coding this it would be better if there were a routine to get shared library paths in a platform-specific way. ctypes.util.find_library comes close, but unfortunately it does not return absolute paths (there are a couple issues in the Python bug tracker about fixing this, but I don't know when it will be).)

[embray]

comment:4

Replying to @embray:

Generally DLLs do not go in /lib on Cygwin. The reason for this is that Windows searches $PATH for DLLs. Further, the default way Cygwin implements symlinks is as normal files. So Windows would see a symlink named cygSingular.dll and try to load the actual file cygSingular.dll as a DLL and fail.

That said, for the purposes of dlopen it might be fine. I think Cygwin will resolve the symlink before passing to the underlying Windows APIs. Regardless, I think this explicit approach is better. I don't see the point of making a symlink on the filesystem that is otherwise pointless for this one case.

[jdemeyer]

comment:5

There is still a potential problem when changing Singular versions. Then multiple DLLs might be installed. How do you know which is the correct one?

[embray]

comment:6

Would there be though? I worried about this too, but I thought any previous version would be uninstalled before installing a new version.

[jdemeyer]

comment:7

Replying to @embray:

Would there be though? I worried about this too, but I thought any previous version would be uninstalled before installing a new version.

build/pkgs/singular/spkg-install has this wonderful part:

remove_old_version()
{   
    # the following is a little verbose but it ensures we leave no trace of 3.x
    # _or_ 4.x
    rm -f "$SAGE_LOCAL"/bin/*Singular*
    rm -f "$SAGE_LOCAL"/bin/*singular*
    rm -rf "$SAGE_LOCAL/include/singular" # 3.x and 4.x
    rm -rf "$SAGE_LOCAL/include/factory" # 3.x and 4.x
    rm -f "$SAGE_LOCAL/include/factor.h" # 3.x only
    rm -f "$SAGE_LOCAL/include/factoryconf.h" # 3.x only
    rm -rf "$SAGE_LOCAL/include/omalloc" #4.x only
    rm -f "$SAGE_LOCAL/include/omalloc.h" # 3.x only
    rm -f "$SAGE_LOCAL/include/omlimits.h" # 3.x only
    rm -rf "$SAGE_LOCAL/include/resources" #4.x only
    rm -rf "$SAGE_LOCAL/include/gfanlib" #4.x only
    rm -f "$SAGE_LOCAL"/lib/libsingular* # 3.x with lower case
    rm -f "$SAGE_LOCAL"/lib/libsingcf*.a # 3.x only additional archives
    rm -f "$SAGE_LOCAL"/lib/libsingfac*.a # 3.x only additional archives
    rm -f "$SAGE_LOCAL"/lib/libSingular* # 4.x with upper case
    rm -f "$SAGE_LOCAL"/lib/p_Procs_Field* # 3.x only
    # a bunch of additional libraries for 4.x
    rm -f "$SAGE_LOCAL"/lib/libpolys*
    rm -f "$SAGE_LOCAL"/lib/libfactory*
    rm -f "$SAGE_LOCAL"/lib/libomalloc*
    rm -f "$SAGE_LOCAL"/lib/libresources*
    rm -r "$SAGE_LOCAL"/lib/libgfan*
    rm -rf "$SAGE_LOCAL/share/singular"
    rm -f "$SAGE_LOCAL"/share/info/singular*
}

which doesn't deal with any of the Cygwin names for the library (that would be another good reason why Cygwin should just the name libSingular instead of cygSingular).

[embray]

comment:8

See #22652

[embray]

comment:9

Yeah, that's pretty hideous, which is why I wrote #22510. In the meantime I'll update the existing "uninstall" to handle Cygwin better. Unfortunately the DLL naming can't be changed.

[embray]

comment:10

At the very least the Singular spkg-install needs to be updated to handle uninstallation better on Cygwin (this is likely the case for other packages as well, but the broader issue can wait for another time).

[sagetrac-git]

Changed commit from c069fc2 to 33233d1

[sagetrac-git]

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

33233d1

Better library cleanup for Singular on Cygwin

[embray]

comment:12

This ensures Singular will be "uninstalled" better when upgrading on Cygwin. This way there really should only be one SINGULAR_SO (if any) found in the method used by this patch.

Much as it's not nice to have platform-specific checks in sage.env I think for something like this (that is, locating a shared library for dlopen()) it's inevitable. So I'd rather leave it explicit for now, and deal with it better later (e.g. in #22652).

[sagetrac-git]

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

15e37fe

'libresources' was renamed 'libsingular_resources' in Singular 4.0

[sagetrac-git]

Changed commit from 33233d1 to 15e37fe

[jdemeyer]

comment:14

Don't hardcode the extension. OS X uses .dylib

[embray]

comment:15

Ah, right. I'll leave it for Cygwin but remove it for the 'else' case.

[sagetrac-git]

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

4f33a48

Don't hard-code the extension on other platforms (e.g. to support OSX)

[sagetrac-git]

Changed commit from 15e37fe to 4f33a48

[jdemeyer]

Reviewer: Jeroen Demeyer

[embray]

comment:20

I think I had rm -rf because one of the lines I replaced was rm -r "$SAGE_LOCAL"/lib/libgfan*, though I don't know why that would need -r either.

I agree SINGULAR_SO = None is fairly useless, but there should still be a better fallback in case the library is not found. Currently this just results in an IndexError if so.

[jdemeyer]

comment:21

Replying to @embray:

I think I had rm -rf because one of the lines I replaced was rm -r "$SAGE_LOCAL"/lib/libgfan*, though I don't know why that would need -r either.

The -r would only be needed if there would be a directory called libgfan...

That seems like something which shouldn't happen and I'd rather see an error in that case.

Currently this just results in an IndexError if so.

At least, it's very clear where the error happens. Setting SINGULAR_SO = None might just lead to more obscure errors later.

[embray]

comment:22

The -r would only be needed if there would be a directory called libgfan...

Yeah. I didn't put it there.

[embray]

comment:23

At least, it's very clear where the error happens. Setting SINGULAR_SO = None might just lead to more obscure errors later.

Better a more explicit error message in that "later" case then. Or earlier, if you prefer. My thinking was just that it might be nice to be able to import sage.env even if the Singular install is broken, for debugging purposes if nothing else.

[jdemeyer]

comment:24

Replying to @embray:

Yeah. I didn't put it there.

I'm not entirely sure that I follow you. You did change -f to -rf. I changed back -rf to -f in my commit.

[embray]

comment:25

You did change -f to -rf

No, I added an entirely new line that used -rf--I didn't change some existing line from -f to -rf. As I explained in [comment:21], one of the lines I replaced contained rm -r (probably unnecessarily). I didn't really think about it though and just took the union of all the flags on the rm calls I was replacing (because honestly, knowing the history of this project and projects like Singular maybe there was a directory matching 'libgfan*' on somebody's install once... =_=)

[embray]

comment:26

Replying to @embray:

At least, it's very clear where the error happens. Setting SINGULAR_SO = None might just lead to more obscure errors later.

Better a more explicit error message in that "later" case then. Or earlier, if you prefer. My thinking was just that it might be nice to be able to import sage.env even if the Singular install is broken, for debugging purposes if nothing else.

Any comment on this? This is the only thing keeping this from having positive_review. There should be an explicit error message if a valid path for SINGULAR_SO is not found (as opposed to an obscure-looking IndexError).

[tscrim]

comment:28

Ping? FYI - I needed this to build on Cygwin.

[embray]

comment:29

If Jeroen doesn't mind I could switch this back over to my branch and just add the minor fix I'm proposing. It's mostly just a nitpick.

[embray]

Changed branch from u/jdemeyer/cygwin/singular_so to u/embray/cygwin/singular_so

[embray]

Changed commit from a5ddc04 to 2e45504

[embray]

Changed author from Erik Bray to Erik Bray, Jeroen Demeyer

[embray]

comment:30

Went ahead and did what I had in mind. This will error out very early on in sage.env on all platforms if Singular is not built/installed properly.

---

New commits:

2e45504

raise an error very early in sage.env if SINGULAR_SO cannot be located

[sagetrac-git]

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

cb3dadf

Still allow SINGULAR_SO to be read from the environment in case it's specified there

[sagetrac-git]

Changed commit from 2e45504 to cb3dadf

[vbraun]

Changed reviewer from Jeroen Demeyer to Jeroen Demeyer, Volker Braun

[vbraun]

Changed dependencies from #21957 to none

[vbraun]

Changed branch from u/embray/cygwin/singular_so to cb3dadf