Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic filename extension for shared libraries #17682

Closed
sagetrac-gouezel mannequin opened this issue Jan 27, 2015 · 25 comments
Closed

Generic filename extension for shared libraries #17682

sagetrac-gouezel mannequin opened this issue Jan 27, 2015 · 25 comments

Comments

@sagetrac-gouezel
Copy link
Mannequin

sagetrac-gouezel mannequin commented Jan 27, 2015

Some places in the code and some doctests assume that the filename extension for shared libraries is .so, while it is .dll in cygwin. The attached patch defines once and for all the correct extension depending on the platform, and uses it where necessary.

CC: @jpflori

Component: porting: Cygwin

Author: Sebastien Gouezel

Branch/Commit: ad478d3

Reviewer: Jeroen Demeyer

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

@sagetrac-gouezel sagetrac-gouezel mannequin added this to the sage-6.5 milestone Jan 27, 2015
@sagetrac-gouezel sagetrac-gouezel mannequin added the p: major / 3 label Jan 27, 2015
@sagetrac-gouezel

This comment has been minimized.

@sagetrac-gouezel sagetrac-gouezel mannequin changed the title help Generic filename extension for shared libraries Jan 27, 2015
@sagetrac-gouezel
Copy link
Mannequin Author

sagetrac-gouezel mannequin commented Jan 27, 2015

Branch: u/gouezel/dll_extension

@sagetrac-gouezel
Copy link
Mannequin Author

sagetrac-gouezel mannequin commented Jan 27, 2015

Commit: 0233104

@sagetrac-gouezel
Copy link
Mannequin Author

sagetrac-gouezel mannequin commented Jan 27, 2015

comment:3

I put the definition of the generic filename extension in sage.misc.sageinspect, since its use is relevant for introspection. Tell me if there is a better place!


New commits:

0233104More generic shared library file extension

@sagetrac-gouezel
Copy link
Mannequin Author

sagetrac-gouezel mannequin commented Jan 30, 2015

Author: Sebastien Gouezel

@tscrim
Copy link
Collaborator

tscrim commented Jan 30, 2015

comment:6

(and myself)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 31, 2015

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

db7df5cTicket #17682: add generic extension in cython caching

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 31, 2015

Changed commit from 0233104 to db7df5c

@sagetrac-jakobkroeker
Copy link
Mannequin

sagetrac-jakobkroeker mannequin commented Feb 8, 2015

comment:8

Ahem, I do not completely understand what the patch does (i'm new to sage/python);
Question: what are the consequences, e.g. for init_libsingular()
in src/sage/libs/singular/singular.pyx?

snip:

...
    for extension in ["so", "dylib", "dll"]:
        lib = os.environ['SAGE_LOCAL']+"/lib/libsingular."+extension
        if os.path.exists(lib):
            handle = dlopen(lib, RTLD_GLOBAL|RTLD_LAZY)
            break
...

@sagetrac-gouezel
Copy link
Mannequin Author

sagetrac-gouezel mannequin commented Feb 9, 2015

comment:9

Replying to @sagetrac-jakobkroeker:

Short answer: the code you show is fine, no need to change anything.

Long answer: shared libraries usually have the extension .so on unix-likes. So, some parts of sage's code assume that this is a universal extension, and use it for loading or doctesting purposes. This leads to problems on cygwin, where the extension is .dll. The patch fixes this code, replacing the universal .so with an extension depending on the platform (.dll on windows, .soanywhere else). Code that already considered the possibility of different extensions is fine.

There is yet another difference on MacOS, where the extension for shared libraries can be .so or .dylib -- this means that, in this case, there is no universal possible choice, so I picked .so since it is the most common in sage. In the case of libsingular, I don't know if it is built using .so or .dylib (and I don't have a Mac to test). If it is .so, one might consider using the patch to predict the correct extension, but in any case the current code works fine.

@jdemeyer
Copy link

jdemeyer commented Feb 9, 2015

comment:10

Replying to @sagetrac-gouezel:

There is yet another difference on MacOS, where the extension for shared libraries can be .so or .dylib -- this means that, in this case, there is no universal possible choice

It's more subtle than that. On OS X, both .so and .dylib exist but with a different purpose: .so is meant for "loadable modules" to be opened at runtime by some dlopen() mechanism, which .dylib is for dynamic linking of libraries (linking done when a program a started). For example, Cython extensions would use .so but the libraries in $SAGE_LOCAL/lib would use .dylib.

On Linux, this difference simply doesn't exist.

@sagetrac-gouezel
Copy link
Mannequin Author

sagetrac-gouezel mannequin commented Apr 22, 2015

Changed commit from db7df5c to 4286d97

@sagetrac-gouezel
Copy link
Mannequin Author

sagetrac-gouezel mannequin commented Apr 22, 2015

New commits:

e27535b #17682: More generic shared library file extension
4286d97Ticket #17682: add generic extension in cython caching

@sagetrac-gouezel
Copy link
Mannequin Author

sagetrac-gouezel mannequin commented Apr 22, 2015

Changed branch from u/gouezel/dll_extension to u/gouezel/dll_extension2

@jdemeyer
Copy link

comment:12

I dislike the complete lack of documentation. Given that constants are hard to document, it's probably better to make it a function def ..._extension() (fill in the ... to what this thing represents, generic_so doesn't mean much to me, see also [comment:10]).

@jdemeyer
Copy link

comment:13

Also: can you replace F[-len(generic_so_extension):] == generic_so_extension by F.endswith(extension).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2015

Changed commit from 4286d97 to 8815433

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2015

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

8815433 #17682: use documented function instead of constant

@sagetrac-gouezel
Copy link
Mannequin Author

sagetrac-gouezel mannequin commented Apr 23, 2015

comment:15

Converted generic_so_extension to a documented function shared_lib_extension(). It is a little bit silly to recompute it every time, but given its triviality and the fact that it should never be time-critical, I don't think caching it is useful.

@sagetrac-gouezel sagetrac-gouezel mannequin removed the s: needs work label Apr 23, 2015
@jdemeyer
Copy link

comment:16

I think all this

sage: from site import getsitepackages
sage: site_packages = getsitepackages()[0]
sage: from sage_setup.find import installed_files_by_module
sage: files_by_module = installed_files_by_module(site_packages)
sage: from sage.misc.sageinspect import shared_lib_extension
sage: files_by_module['sage.structure.sage_object'].pop().endswith(shared_lib_extension())

can be replaced by

sage: sage.structure.sage_object.__file__.endswith(shared_lib_extension())

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@jdemeyer
Copy link

comment:17

Can you replace the terminology shared_lib by loadable_module. As I said, these are different on OS X and Cython modules are "loadable modules" and not "shared libraries" (the latter would have a .dylib extension on OS X).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 24, 2015

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

ad478d3 #17682: change to loadable_module_extension

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 24, 2015

Changed commit from 8815433 to ad478d3

@vbraun
Copy link
Member

vbraun commented Apr 28, 2015

Changed branch from u/gouezel/dll_extension2 to ad478d3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants