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

prefer libs build from source over the system #30

Merged
merged 1 commit into from Aug 30, 2014
Merged

prefer libs build from source over the system #30

merged 1 commit into from Aug 30, 2014

Conversation

plicease
Copy link
Contributor

I'm working on an Alien::Base dist, and it works with the system package and it works building from source, but if I install from source and then install the system packages, it stops working with this error:

t/list_contents_of_archive.t .. Can't load '/home/ollisg/dev/Archive-Libarchive-XS/.build/2FkGCFgZ1T/blib/arch/auto/Archive/Libarchive/XS/XS.so' for module Archive::Libarchive::XS: libarchive.so.13: cannot open shared object file: No such file or directory at /home/ollisg/perl5/perlbrew/perls/perl-5.18.1/lib/5.18.1/x86_64-linux/DynaLoader.pm line 190.
 at t/list_contents_of_archive.t line 3.

This patch prefers the Alien::Base lib dir to the system one, and makes it work for me.

@jberger
Copy link
Member

jberger commented Nov 13, 2013

This seems worthy of consideration. I think most of the time one would prefer to use a system installed version of a library if available. That is why the AB has the current behavior. But certainly there are times when you would want to prefer a manually installed one, as you have demonstrated.

I wonder if this could be configurable somehow. It would be easy to do with an env variable, but I wonder if something more cute could be better.

I am far from home currently attending YAPC::Brasil, so I can't look at this right now, but if I don't get back to it in 2 weeks, ping me again.

@plicease
Copy link
Contributor Author

Thanks, I appreciate your consideration.

I think preferring the system lib is okay, if it works, but currently it is looking for libarchive.so.13 (the version installed from source) in the system location which is libarchive.so.12. It should at least look for the right library in the right location. I can help with a simplified test case if you need help reproducing this.

I think the install time and run time loading will have to be consistent too, for example in this sequence:

% cpanm Alien::XYZ
% cpanm XYZ::XS
% apt-get install libxyz-dev
% perl -MXYZ::XS

in the last step, the using XYZ::XS should use the source built version since its XS code was linked against that version. In this sequence it could use the system lib:

% cpanm Alien::XYZ
% apt-get install libxyz-dev
% cpanm XYZ::XS
% perl -MXYZ::XS

though you may have problems if you are using multiple perl modules installed before and after the system lib (and thus linked against different versions).

@devel-chm
Copy link

On Wed, Nov 13, 2013 at 10:18 AM, Graham Ollis notifications@github.comwrote:

Thanks, I appreciate your consideration.

I think preferring the system lib is okay, if it works, but currently it
is looking for libarchive.so.13 (the version installed from source) in the
system location which is libarchive.so.12. It should at least look for the
right library in the right location. I can with a simplified test case if
you need help reproducing this.

I think the install time and run time loading will have to be consistent
too, for example in this sequence:

% cpanm Alien::XYZ
% cpanm XYZ::XS
% apt-get install libxyz-dev
% perl -MXYZ::XS

in the last step, the using XYZ::XS should use the source built version
since its XS code was linked against that version.

No, I think Alien::XYZ will install and configure for a local install. The
fact
that you have installed a system-wide version would not be detected
unless you reinstall Alien:XYZ.

In this sequence it could use the system lib:

% cpanm Alien::XYZ
% apt-get install libxyz-dev
% cpanm XYZ::XS
% perl -MXYZ::XS

This would/should be the case if you installed the system libxyz stuff
before the Alien::XYZ step. Otherwise, as before, you would need to
reinstall Alien::XYZ to detect....

though you may have problems if you are using multiple perl modules

installed before and after the system lib (and thus linked against
different versions).


Reply to this email directly or view it on GitHubhttps://github.com//pull/30#issuecomment-28402688
.

@plicease
Copy link
Contributor Author

I think there may be some confusion since my comment was "prefer libs build from source over the system", but what I should have said was "prefer libs build from source over the system when that is what you used when you installed your Alien::XYZ distribution".

I agree with @devel-chm's comment, in that is how it SHOULD work, I think the least complicated approach is to, once Alien::XYZ is installed it should use the same lib, unless Alien::XYZ is reinstalled. Since my original issue was this one:

% cpanm Alien::XYZ
% apt-get install libxyz-dev
% cpanm XYZ::XS                 # fails

I thought @jberger was saying that the cpanm XYZ::XS should prefer the system lib, so my second comment was simply raising concerns with that approach (and if that IS what he was saying, then I still think it is tricky, but doable).

With my patch it still prefers the system lib, if that is what you used at install of Alien::XYZ even in this scenario:

% cpanm Alien::XYZ
% apt-get install libxyz-dev
% cpanm --reinstall Alien::XYZ
% cpanm XYZ::XS                       # works with or without my patch, and uses the system lib

(I just tested this)

@plicease
Copy link
Contributor Author

plicease commented Dec 2, 2013

As requested, @jberger I am pinging you on this again.

My belief was that this was a fix to the intended behavior, that is that the lib used when you install Alien::XYZ should be the lib that you use until re-install of Alien::XYZ. This patch will still prefer the system lib if that is what is found when Alien::XYZ is installed.

I commented on some of the complexity if that wasn't the intended behavior, in the very least if you calculate at runtime which lib to use it should be consistent, as it is, it is looking for the custom built lib in the system lib location and this causes a failure if you install the system lib after Alien::XYZ, but before XYZ::XS.

@jberger
Copy link
Member

jberger commented Dec 15, 2013

I'm starting to lean towards accepting this patch. It's being discussed a little on #pdl and it is sinking into my head. I think that most times this patch makes more sense than the original, though edge cases exist for both.

@plicease
Copy link
Contributor Author

I think that a strong argument in favor of sticking with the lib used at install time is that is the lib that you have tested against. Presumably the system developers have packaged things up appropriately, but they may have made decisions that would prevent carefully crafted tests in the Alien::XYZ from passing, or the lib may just be very very old and doesn't support features that you are using when you compiled, tested and installed XYZ::XS.

I also think this is the least surprising approach, I wouldn't expect if I was working on something totally unrelated to Perl and had to install some development libraries that my Perl scripts would stop working. Ideally it wouldn't break if you reinstalled Alien::XYZ without reinstalling XYZ::XS either, but at least in that case you are actively working with Perl.

I believe if we do accept that the system library should be used regardless of what was used during install that the pkg-config information from the share directory should not be used at all, as it very easily could conflict with what the system lib is expecting. It should probably also report install_type = system. The fact that it was reporting install_type = share and was in a code path that couldn't be reached in a system install lead me to believe that this was a bug rather than a deliberate design decision.

@devel-chm
Copy link

On Mon, Dec 16, 2013 at 6:06 AM, Graham Ollis notifications@github.comwrote:

I think that a strong argument in favor of sticking with the lib used at
install time is that is the lib that you have tested against. Presumably
the system developers have packaged things up appropriately, but they may
have made decisions that would prevent carefully crafted tests in the
Alien::XYZ from passing, or the lib may just be very very old and doesn't
support features that you are using when you compiled and installed
XYZ::XS.

I also think this is the least surprising approach, I wouldn't expect if I
was working on something totally unrelated to Perl and had to install some
development libraries that my Perl scripts would stop working. Ideally it
wouldn't break if you reinstalled Alien::XYZ without reinstalling XYZ::XSeither, but at least in that case you are actively working with Perl.

Interesting. Maybe Alien::XYZ could try 'use Alien::XYZ' itself
just in case it has already been installed. At least a warning
could be printed if things are going to be changed.

I believe if we do accept that the system library should be used
regardless of what was used during install that the pkg-config information
from the share directory should not be used at all. It should probably also
report install_type = system. The fact that it was reporting install_type =
share and was in a code path that couldn't be reached in a system install
lead me to believe that this was a bug rather than a deliberate design
decision.

I can't think of a good case where silently substituting a system
library would be a viable approach. It would be nice if a module
using Alien::XYZ to configure and build could be notified if the
Alien::XYZ were re-installed and give a warning or something...

@zmughal
Copy link
Member

zmughal commented Aug 16, 2014

I just came across this issue while working on Image::Leptonica. I have a system installed liblept that is there because Tesseract OCR needs it, but the one I provide with Alien::Leptonica is newer and has more functions.

This really will require further thought as it is really is a toolchain issue. There are several questions like: Do we assume that the user always wants the newest library? Should the Alien package force a preference or should that be left to the downstream of the Alien package? I can envision there being flags that can be set with either environment variables or package import tags which can indicate the preference.

@plicease
Copy link
Contributor Author

@zmughal I agree that these are all issues, and would love to see them resolved in a thoughtful way, but I do not agree that this is what this defect is about.

The problem is that Alien::Base can and will use the compiler and linker flags from a built libfoo but the libfoo.so from the system if you install the operating system package after installing Alien::Libfoo. I am a bit dismayed as to why this is not considered a bug.

@mohawk2
Copy link
Contributor

mohawk2 commented Aug 18, 2014

Speaking with an outsider's perspective, that clearly is a bug. Raise issue?

@zmughal
Copy link
Member

zmughal commented Aug 20, 2014

I see no reason to not go ahead with this PR. It is the only solution that will load the library from the shared directory whether or not a system library available.

I think loading from the shared directory is a sensible default since, like @plicease said, that is what the install was tested against and is guaranteed to work. The other issues that I talked about can be addressed later as they are special cases that most people won't want[*]. Merging this PR will not preclude us from allowing users to choose between system & shared libraries if Alien::Base adds that at a later point.

And to address warning users, we could try to load from each of the paths in turn, a la which -a, but that is a question for another PR.

Is there something I'm missing?

[*] I can only imagine people wanting to load the system version of a library if that version was patched to work better than the one directly from upstream. In that case, another thing to think about is library versions the same way we think about module versions, e.g., I want libz >= v1.2.

@mohawk2
Copy link
Contributor

mohawk2 commented Aug 30, 2014

@zmughal, if you believe this code addresses the rationale behind the PR (keep reporting right info even if system library becomes installed), you ought to merge it.

If anyone feels there are further issues, they can raise them in a new issue and/or PR.

@zmughal
Copy link
Member

zmughal commented Aug 30, 2014

I'm interpreting the org procedure for this requiring a majority vote

The addition and modification of features is decided by majority vote or the pumpkin-holder.

My vote on merging this PR: aye.

@plicease
Copy link
Contributor Author

I agree, of course.

@plicease
Copy link
Contributor Author

How many is a majority?

@zmughal
Copy link
Member

zmughal commented Aug 30, 2014

Currently 4, since there are 6 in the Owners team.

@jberger
Copy link
Member

jberger commented Aug 30, 2014

This statement I believe I agree with:

"prefer libs build from source over the system when that is what you used when you installed your Alien::XYZ distribution"

but beyond that, I have never been able to convince myself on either side of this argument. Therefore I abstain. 3/5 means pass right?

mohawk2 added a commit that referenced this pull request Aug 30, 2014
prefer libs build from source over the system
@mohawk2 mohawk2 merged commit b1e64d2 into Perl5-Alien:master Aug 30, 2014
@devel-chm
Copy link

One of the problems with Alien modules is the fixed assumptions
that are implicitly forced by a one-size-fits-all approach. My take
is that if you install Module::Using::XXX which uses Alien::XXX to
hook up with the non-perl dependency, then that should not break
due to the Alien::XXX implementation and it should be possible to
figure out what is wrong if something happens.

This ties in with my Alien2 proposal that the job of an Alien2
module is not just to make a dependency available and drop it
after that. It should support making a dependency available as
well. E.g., runtime verification of the build/use environment
should be possible. It should be possible to prefer a system
installed library over a self-installed one. One should not be
forced to use a static library (this is a hack-around for win32
which really doesn't work with the way windows sets things
up as an OS).

I think the above means I think it is a bug. But since I don't
actually see the statement being voted on, can't help there...

On Sat, Aug 30, 2014 at 5:23 PM, Joel Berger notifications@github.com
wrote:

This statement I believe I agree with:

"prefer libs build from source over the system when that is what you used
when you installed your Alien::XYZ distribution"

but beyond that, I have never been able to convince myself on either side
of this argument. Therefore I abstain. 3/5 means pass right?


Reply to this email directly or view it on GitHub
#30 (comment).

@jberger
Copy link
Member

jberger commented Aug 30, 2014

One of the problems with Alien modules is the fixed assumptions
that are implicitly forced by a one-size-fits-all approach.

This is the reason it took so long for someone to try to abstract it. There will likely be eventual limitations, but let's keep abstracting where possible.

My original idea included version pinning as an option(ie, install version X only, vestiges of a full version selector plugin system appear in the code, though never fully realized). The primary mechanism allows for the upstream library to release a new version without the Alien:: wrapper needing a new release just for a config change. That said, it seems fair that once installed, the version be pinned to the version used when building (and testing).

@devel-chm
Copy link

On systems without a package system, it works better
to check for a pre-existing install first before trying to
install/build/get the dependency. For one, it allows the
exit with instructions as a quick start implementation.

--Chris

@mohawk2
Copy link
Contributor

mohawk2 commented Aug 30, 2014

I've opened a new issue (#45) for discussion of packaging-system matters.

@plicease plicease deleted the prefer_build_over_system branch September 1, 2014 03:18
plicease pushed a commit that referenced this pull request Jul 17, 2017
prefer libs build from source over the system
plicease pushed a commit that referenced this pull request Jul 17, 2017
prefer libs build from source over the system
plicease pushed a commit that referenced this pull request Jul 17, 2017
prefer libs build from source over the system
plicease pushed a commit that referenced this pull request Jul 17, 2017
prefer libs build from source over the system
plicease pushed a commit that referenced this pull request Jul 17, 2017
prefer libs build from source over the system
plicease pushed a commit that referenced this pull request Jul 17, 2017
…stem

Alien-Base: 
Alien-Base: prefer libs build from source over the system
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.

None yet

5 participants