Skip to content
This repository has been archived by the owner on Mar 7, 2019. It is now read-only.

add property alien_isolate_dynamic #51

Merged
merged 1 commit into from
Sep 4, 2014
Merged

add property alien_isolate_dynamic #51

merged 1 commit into from
Sep 4, 2014

Conversation

plicease
Copy link
Contributor

@plicease plicease commented Sep 2, 2014

This is my proposed solution for #11. It isolates the dynamic libraries into a separate directory so they will not be considered when creating XS modules. This is only done when alien_isolate_dynamic is set to true. For this to be useful, the author of a Alien::Foo module will need to build position independent code on platforms that require it, in which case also accepting #47 or something similar will be highly desirable.

There are a number of advantages to forcing static libraries when building XS modules:

  1. upgrades to Alien::Foo will no longer break Foo::XS. The downside is that Foo::XS will still be using the old version of libfoo, but I believe this is better than breaking the module.
  2. Alien::Foo is no longer a run time dep for Foo::XS (just a build dep) so there is less to load
  3. The method that currently AB uses for including dynamic libraries in platform specific search paths is broken on some platforms, such as Solaris (Loading .so file from share directory does not work on Solaris #50) and Windows (add bin (if it exists) directory to PATH on win32 #32). Though this is probably solvable.

There are also risks, but given that this alien_isolate_dynamic is optional they seem to be minimal.

That being said I think there is an argument for making alien_isolate_dynamic true by default, if not immediately, then some time down the road. I think the number of Alien::Foo type dists that would benefit from this feature is probably larger than the number with risks.

@mohawk2
Copy link
Contributor

mohawk2 commented Sep 3, 2014

Has this been tried on a couple of examples, and on Windows and Unix-like?

@plicease
Copy link
Contributor Author

plicease commented Sep 4, 2014

I tested it with Alien::FFI and Alien::Editline ... they are not on CPAN yet, I was developing them mostly as examples. They do the right thing on Linux and Windows.

@mohawk2
Copy link
Contributor

mohawk2 commented Sep 4, 2014

Good enough for me: aye.

@zmughal
Copy link
Member

zmughal commented Sep 4, 2014

Nice!

Looking at line 32 of lib/Alien/Editline.pm, I see that it suggests calling the ->dlls() method.

my($dll) = Alien::Editline->new->dlls;

This function isn't in Alien::Base yet, is it? If an easy interface like that is available, I would say merge this.

Merge: Aye.

@plicease
Copy link
Contributor Author

plicease commented Sep 4, 2014

@zmughal good catch. dlls is what I was using in Alien::Libarchive::Installer and friends and the doco for Alien::Editline and Alien::FFI are based on the Alien::Libarchive doco. But when I was working on Alien::Base I called it dynamic_lib for two reasons

  1. I was thinking it matched the libs accessor better.
  2. dlls sounds a little windowsish to me; not a terrible thing, but was going for more of a cross platform terminology.

Now that I write this though I think it should be dynamic_libs to match the libs accessor, and also because technically there might be multiple dynamic libraries from a project.

Preferences anyone?

@plicease
Copy link
Contributor Author

plicease commented Sep 4, 2014

Also I should write some documentation for the dynamic_libs sub

@zmughal
Copy link
Member

zmughal commented Sep 4, 2014

I like dynamic_libs.

Aside: when I was making Alien::LibMagic, I actually had multiple .dlls on Windows: libgnurx was needed by libmagic to deal with missing POSIX functions. But I may spin out that to a separate Alien package.

@mohawk2
Copy link
Contributor

mohawk2 commented Sep 4, 2014

I like dynamic_libs too.

plicease added a commit that referenced this pull request Sep 4, 2014
add property alien_isolate_dynamic
@plicease plicease merged commit 041df2a into Perl5-Alien:master Sep 4, 2014
@plicease
Copy link
Contributor Author

plicease commented Sep 4, 2014

Oops forgot to change name, but did so with this commit:

df7d78c

@plicease plicease deleted the isolate branch September 4, 2014 14:44
plicease added a commit that referenced this pull request Jul 17, 2017
add property alien_isolate_dynamic
plicease added a commit that referenced this pull request Jul 17, 2017
add property alien_isolate_dynamic
plicease added a commit that referenced this pull request Jul 17, 2017
add property alien_isolate_dynamic
plicease added a commit that referenced this pull request Jul 17, 2017
add property alien_isolate_dynamic
plicease added a commit that referenced this pull request Jul 17, 2017
add property alien_isolate_dynamic
plicease added a commit that referenced this pull request Jul 17, 2017
Alien-Base: 
Alien-Base: add property alien_isolate_dynamic
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants