Opened 5 years ago
Closed 5 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: |
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
- Status changed from new to needs_review
comment:2 Changed 5 years ago by
comment:3 follow-up: ↓ 4 Changed 5 years ago by
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
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 namedcygSingular.dll
and try to load the actual filecygSingular.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
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: ↓ 7 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
See #22652
comment:9 Changed 5 years ago by
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.
comment:10 Changed 5 years ago by
- 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 5 years ago by
- Commit changed from c069fc27fa7933bc2cf353e65729849b63f825d7 to 33233d1788b3d030169c1e65ba83aa3de09cdec3
Branch pushed to git repo; I updated commit sha1. New commits:
33233d1 | Better library cleanup for Singular on Cygwin
|
comment:12 Changed 5 years ago by
- 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 5 years ago by
- 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 5 years ago by
- Status changed from needs_review to needs_work
Don't hardcode the extension. OS X uses .dylib
comment:15 Changed 5 years ago by
Ah, right. I'll leave it for Cygwin but remove it for the 'else' case.
comment:16 Changed 5 years ago by
- Commit changed from 15e37fe1a10e35b6b40d2ba2bcd984e0517a6416 to 4f33a48f6fee31322f89916437cf8009f564df06
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)
|
comment:17 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:18 Changed 5 years ago by
- Branch changed from u/embray/cygwin/singular_so to u/jdemeyer/cygwin/singular_so
comment:19 Changed 5 years ago by
- Commit changed from 4f33a48f6fee31322f89916437cf8009f564df06 to a5ddc0406a1ca806cf3d5159e1b135c95b7fb30d
- Reviewers set to Jeroen Demeyer
comment:20 follow-up: ↓ 21 Changed 5 years ago by
- 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 5 years ago by
Replying to embray:
I think I had
rm -rf
because one of the lines I replaced wasrm -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: ↓ 24 Changed 5 years ago by
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: ↓ 26 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
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 named 'libfan*' on somebody's install once... =_=)
comment:26 in reply to: ↑ 23 Changed 5 years ago by
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 5 years ago by
- Priority changed from major to blocker
comment:28 Changed 5 years ago by
Ping? FYI - I needed this to build on Cygwin.
comment:29 Changed 5 years ago by
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 5 years ago by
- 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:
2e45504 | raise an error very early in sage.env if SINGULAR_SO cannot be located
|
comment:31 Changed 5 years ago by
- Commit changed from 2e45504a577c8d72177cb526617b68358b9038c9 to cb3dadf42350807cc7da3d9e6365ce5cfed78c39
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
|
comment:32 Changed 5 years ago by
- Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Volker Braun
- Status changed from needs_review to positive_review
comment:33 Changed 5 years ago by
- Dependencies #21957 deleted
comment:34 Changed 5 years ago by
- Branch changed from u/embray/cygwin/singular_so to cb3dadf42350807cc7da3d9e6365ce5cfed78c39
- Resolution set to fixed
- Status changed from positive_review to closed
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 ifcygSingular.dll
would just be calledlibSingular.dll
. Is there any chance of getting done in upstream Singular? Then the same code could be used on all platforms.