Opened 6 years ago

Closed 6 years ago

#17682 closed defect (fixed)

Generic filename extension for shared libraries

Reported by: gouezel Owned by:
Priority: major Milestone: sage-6.7
Component: porting: Cygwin Keywords:
Cc: jpflori Merged in:
Authors: Sebastien Gouezel Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: ad478d3 (Commits) Commit: ad478d3d9ca70f98316243c3cb0800c7a9210ca7
Dependencies: Stopgaps:

Description (last modified by gouezel)

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.

Change History (21)

comment:1 Changed 6 years ago by gouezel

  • Component changed from PLEASE CHANGE to porting: Cygwin
  • Description modified (diff)
  • Summary changed from help to Generic filename extension for shared libraries

comment:2 Changed 6 years ago by gouezel

  • Branch set to u/gouezel/dll_extension

comment:3 Changed 6 years ago by gouezel

  • Commit set to 02331047be37ffa47927a0b9db68fed7ef262ae2

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

comment:4 Changed 6 years ago by gouezel

  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to defect

comment:5 Changed 6 years ago by gouezel

  • Authors set to Sebastien Gouezel

comment:6 Changed 6 years ago by tscrim

  • Cc jpflori added

(and myself)

comment:7 Changed 6 years ago by git

  • Commit changed from 02331047be37ffa47927a0b9db68fed7ef262ae2 to db7df5c13b7c57272f0cc262c89d80a632c878ef

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

db7df5cTicket #17682: add generic extension in cython caching

comment:8 follow-up: Changed 6 years ago by jakobkroeker

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

comment:9 in reply to: ↑ 8 ; follow-up: Changed 6 years ago by gouezel

Replying to 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.

comment:10 in reply to: ↑ 9 Changed 6 years ago by jdemeyer

Replying to 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.

comment:11 Changed 6 years ago by gouezel

  • Branch changed from u/gouezel/dll_extension to u/gouezel/dll_extension2
  • Commit changed from db7df5c13b7c57272f0cc262c89d80a632c878ef to 4286d976916a6a6894e51d50e37e70e8b30736e8

New commits:

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

comment:12 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-6.5 to sage-6.7
  • Status changed from needs_review to needs_work

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

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:13 Changed 6 years ago by jdemeyer

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

comment:14 Changed 6 years ago by git

  • Commit changed from 4286d976916a6a6894e51d50e37e70e8b30736e8 to 8815433a50b356d25a0be040d29c7224d6f4b1fb

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

8815433 #17682: use documented function instead of constant

comment:15 Changed 6 years ago by gouezel

  • Status changed from needs_work to needs_review

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.

comment:16 Changed 6 years ago by jdemeyer

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

comment:17 Changed 6 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

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

comment:18 Changed 6 years ago by git

  • Commit changed from 8815433a50b356d25a0be040d29c7224d6f4b1fb to ad478d3d9ca70f98316243c3cb0800c7a9210ca7

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

ad478d3 #17682: change to loadable_module_extension

comment:19 Changed 6 years ago by gouezel

  • Status changed from needs_work to needs_review

comment:20 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:21 Changed 6 years ago by vbraun

  • Branch changed from u/gouezel/dll_extension2 to ad478d3d9ca70f98316243c3cb0800c7a9210ca7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.