Perl5-Alien / Alien-Base

Base classes for Alien:: modules (deprecated, see Alien-Build)
Other
35 stars 19 forks source link

Add a property alien_extra_site_config #166

Closed gugod closed 7 years ago

gugod commented 8 years ago

This property is used to put key/value in "config.site" file when autoconf is used. It is added so that enviroment variables such as CFLAGS/CPPFLAGS can effect the tests ran by "configure"

I was stucked making "Alien::pHash" and realized that something important is missing in the build tool. The problem I ran into is because phash depends on cimg -- which was already released as "Alien::CImg". I'm figuring out how to build phash that links to CImg, particularly in the context of "share" installation type. So IOW: how do I add the correct "-I/path/to/Alien-CImg/include" to make the existince of "CImg.h" correctly detected when running "./configure" script of pHash. (CImg is a source-only release -- it is only a .h file)

I tried to use "alien_env" but it does not effect the outcome, although when I manually run "./configure" with the env CFLAGS/CPPFLAGS/CXXFLAGS it worked.

Eventually I figured out that Alien::Base is generating a special "config.site" and overriding certain environment variable within it.

See: https://github.com/gugod/Alien-pHash/blob/wip-link-to-alien-cimg/Build.PL#L38

Maybe I could've just use "alien_env" property for this purpose, since I'm not sure if there is a need to distinguish these variables. If that's preferred I can rework this PR.

mohawk2 commented 8 years ago

The adding of things like -I flags is something that ExtUtils::Depends is all about. Are we leveraging that fully in A::B yet?

plicease commented 8 years ago

for reference the generated site.config was introduced with #113 . The intent is to support 64 bit Perls on hybrid 32/64bit systesm where 32bit is the default output from compilers.

If this PR is accepted we need to document well when this feature should be used. (I am happy to write the documentation, but we should discuss here). We should also consider if there is a better way. @gugod can you elaborate on how you might use alien_env?

plicease commented 8 years ago

I will also review this more closely next week.

gugod commented 8 years ago

Sure @plicease, let me explain more.

I was mainly trying out to install libphash without system packages at all. The current WIP of Alien::pHash disable a bunch of feature so it depends only on a library called "CImg". So my goal is to install these 2 packages under CPAN site_lib directory, and use them via XS/FFI.

After some reading about Alien::Base I believe that's what install_type "share" is all about. In other word, I'm trying to do a pure "share"-type installation. I want no dependencies of system packages.

In other word, I'm trying to make this work for me:

export ALIEN_INSTALL_TYPE=share
cpanm --install Alien::pHash

So I tried making Alien::CImg -- which is a dependency of libphash. That' was pretty trvial. Alien::Base installs CImg.h to lib/perl5/auto/share/dist/Alien-CImg/include/CImg.h so far so good.

But I got stucked at the "./configure" phrase of phash. Basically "./configure" failed because CImg.h is missing from its point of view. I keep trying different parameters of Alien::Base::ModuleBuild, none of them can passthru CFLAGS/CXXFLAGS to the "./configure" script. It appeared to me that alien_env could be the parametere I want, but that did not do the job. However, if I manually set CFLAGS and CXXFLAGS to something like -I/path/to/lib/perl5/auto/share/dist/Alien-CImg/include/CImg.h then manually run "./configure", it worked out perfectly. After some digging I concluded that Alien::Base::ModuleBuild needs some modification.

I also looked into how Alien::OTR dealt with this, since libotr depends on libgcrypt. It turns out it use this wrapper as its "./configure" command:

https://metacpan.org/source/AJGB/Alien-OTR-4.1.1.0/inc/configure.pl

... which correctly works with "share" install_type. The "./configure" script of libotr has a dedicate argument to specify the prefix of libgcrypt. The "./configure" script from libphash does not have such argument so I can only relying on env vars, or the content from "config.site". I also tried to put a "config.site" under the $prefix of Alien-pHash at installation time but that also failed to work -- this is when I figured out that Alien::Base::ModuleBuild is overriding CONFIG_SITE for its own purpose.

That should be the whole story.

My PR introduce a new parameter to Alien::Base::ModuleBuild, but maybe alien_env is better. We just need to correctly deal with special values (CFLAGS/CPPFLAGS/CXXFLAGS/SITE_CONFIG... etc)

plicease commented 7 years ago

Sorry I meant to come back to this a while ago! I was thinking it might make sense for alien_env to override the site.config, but in reviewing this change again in a fresh light I think this is the better, more extensible solution, and I'd like to vote for its inclusion. I'll wait for others on the team to weigh in, or seven days before merging. I will write the documentation for the feature after merging, unless somebody else provides a PR for that.

Merge: aye

plicease commented 7 years ago

rebased and merged. I hope to do a dev release this weekend, and if everything looks good a prod release next week.

cpantesters has been a bit iffy lately though, so who knows.