Opened 5 years ago

Closed 4 years ago

#22628 closed defect (fixed)

SINGULAR_SO default is incorrect of Cygwin

Reported by: embray Owned by:
Priority: blocker Milestone: sage-8.0
Component: porting: Cygwin Keywords:
Cc: Merged in:
Authors: Erik Bray, Jeroen Demeyer Reviewers: Jeroen Demeyer, Volker Braun
Report Upstream: N/A Work issues:
Branch: cb3dadf (Commits, GitHub, GitLab) Commit: cb3dadf42350807cc7da3d9e6365ce5cfed78c39
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (34)

comment:1 Changed 5 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 5 years ago by jdemeyer

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.

comment:3 follow-up: Changed 5 years ago by 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.

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

comment:4 in reply to: ↑ 3 Changed 5 years ago by embray

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.

comment:5 Changed 5 years ago by jdemeyer

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?

comment:6 follow-up: Changed 4 years ago by embray

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

comment:7 in reply to: ↑ 6 Changed 4 years ago by jdemeyer

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

comment:8 Changed 4 years ago by embray

See #22652

comment:9 Changed 4 years ago by embray

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.

Last edited 4 years ago by embray (previous) (diff)

comment:10 Changed 4 years ago by embray

  • Status changed from needs_review to needs_work

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

comment:11 Changed 4 years ago by git

  • Commit changed from c069fc27fa7933bc2cf353e65729849b63f825d7 to 33233d1788b3d030169c1e65ba83aa3de09cdec3

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

33233d1Better library cleanup for Singular on Cygwin

comment:12 Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

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

comment:13 Changed 4 years ago by git

  • Commit changed from 33233d1788b3d030169c1e65ba83aa3de09cdec3 to 15e37fe1a10e35b6b40d2ba2bcd984e0517a6416

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

15e37fe'libresources' was renamed 'libsingular_resources' in Singular 4.0

comment:14 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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

comment:15 Changed 4 years ago by embray

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

comment:16 Changed 4 years ago by git

  • Commit changed from 15e37fe1a10e35b6b40d2ba2bcd984e0517a6416 to 4f33a48f6fee31322f89916437cf8009f564df06

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

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

comment:17 Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

comment:18 Changed 4 years ago by jdemeyer

  • Branch changed from u/embray/cygwin/singular_so to u/jdemeyer/cygwin/singular_so

comment:19 Changed 4 years ago by jdemeyer

  • Commit changed from 4f33a48f6fee31322f89916437cf8009f564df06 to a5ddc0406a1ca806cf3d5159e1b135c95b7fb30d
  • Reviewers set to Jeroen Demeyer

I made two minor corrections. You can set this ticket to positive_review if you agree.


New commits:

48f1827Libraries are not directories, no need for rm -r
a5ddc04SINGULAR_SO should not be None

comment:20 follow-up: Changed 4 years ago by embray

  • Status changed from needs_review to needs_work

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.

comment:21 in reply to: ↑ 20 Changed 4 years ago by jdemeyer

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.

comment:22 follow-up: Changed 4 years ago by embray

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

Yeah. I didn't put it there.

comment:23 follow-up: Changed 4 years ago by 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.

comment:24 in reply to: ↑ 22 Changed 4 years ago by jdemeyer

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.

comment:25 Changed 4 years ago by embray

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

Last edited 4 years ago by embray (previous) (diff)

comment:26 in reply to: ↑ 23 Changed 4 years ago by embray

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

comment:27 Changed 4 years ago by embray

  • Priority changed from major to blocker

comment:28 Changed 4 years ago by tscrim

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

comment:29 Changed 4 years ago by embray

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.

comment:30 Changed 4 years ago by embray

  • Authors changed from Erik Bray to Erik Bray, Jeroen Demeyer
  • Branch changed from u/jdemeyer/cygwin/singular_so to u/embray/cygwin/singular_so
  • Commit changed from a5ddc0406a1ca806cf3d5159e1b135c95b7fb30d to 2e45504a577c8d72177cb526617b68358b9038c9
  • Status changed from needs_work to needs_review

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:

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

comment:31 Changed 4 years ago by git

  • Commit changed from 2e45504a577c8d72177cb526617b68358b9038c9 to cb3dadf42350807cc7da3d9e6365ce5cfed78c39

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

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

comment:32 Changed 4 years ago by vbraun

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Volker Braun
  • Status changed from needs_review to positive_review

comment:33 Changed 4 years ago by vbraun

  • Dependencies #21957 deleted

comment:34 Changed 4 years ago by vbraun

  • Branch changed from u/embray/cygwin/singular_so to cb3dadf42350807cc7da3d9e6365ce5cfed78c39
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.