[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gentoo-dev
Subject:    Re: [gentoo-dev] [PATCH 2/2] Support wrapping headers for multilib ABIs.
From:       Alexis Ballier <aballier () gentoo ! org>
Date:       2013-03-24 20:01:08
Message-ID: 20130324210108.4a408c31 () portable
[Download RAW message or body]

On Sun, 24 Mar 2013 16:41:15 +0100
Michał Górny <mgorny@gentoo.org> wrote:

> On Sun, 24 Mar 2013 16:14:52 +0100
> Alexis Ballier <aballier@gentoo.org> wrote:
> 
> > On Sat, 23 Mar 2013 17:26:38 +0100
> > Michał Górny <mgorny@gentoo.org> wrote:
> > 
> > > ---
> > >  gx86/eclass/autotools-multilib.eclass | 82
> > > +++++++++++++++++++++++++++++++++++ 1 file changed, 82
> > > insertions(+)
> > > 
> > > diff --git a/gx86/eclass/autotools-multilib.eclass
> > > b/gx86/eclass/autotools-multilib.eclass index d7372b0..c65c777
> > > 100644 --- a/gx86/eclass/autotools-multilib.eclass
> > > +++ b/gx86/eclass/autotools-multilib.eclass
> > 
> > 
> > why not multilib-build ?
> 
> Because it has two parts which have to be called at the right time.
> It doesn't have a public API yet, so I'm putting it where I can test
> it sanely.


Well, it has nothing to do with autotools :/

I'm not sure how to do it properly though... the function should be
"shared" but multilib-build doesn't export any phase function by design.
Maybe put the function and the variable documentation in
multilib-build, call it from autotools-multilib and add documentation
to autotools-multilib like: "this eclass supports header wrapping, see
multilib-build documentation about MULTILIB_WRAPPED_HEADERS variable"

[...]
> > I'd just go for paths relative to $EPREFIX/usr/include (or
> > $ED/usr/include) for MULTILIB_WRAPPED_HEADERS. That would simplify
> > the code.
> 
> That's true. But on the other hand, that would introduce yet another
> root for paths which would probably end up being confusing.
> Using /usr/include/... feels more natural IMO.

if you have to strip /usr/include to use the input, then just don't
require /usr/include as input ;)

> And of course, it leaves the possibility of supporting other locations
> in the future.

But you're right: There's no need to unnecessarily ban corner cases.


> > > +		# and then usr/include
> > > +		f=${f#usr/include/}
> > > +
> > > +		local dir=${f%/*}
> > > +
> > > +		# $CHOST shall be set by multilib_toolchain_setup
> > > +		dodir "/tmp/multilib-include/${CHOST}/${dir}"
> > > +		mv "${ED}/usr/include/${f}"
> > > "${ED}/tmp/multilib-include/${CHOST}/${dir}/" || die +
> > 
> > why not use $T rather than $ED/tmp ?
> 
> I prefer using $D rather than $T for files which are intended to be
> installed. Purely theoretically, $T could be on another filesystem
> than $D. Then, moving the file back and forth could cause it to lose
> some of the metadata -- and will be slower, of course.

and if some file is left behind it will install stuff in /tmp :/

I usually tend to consider $D as a write-only file system and consider
it cleaner that way, but that's true, moving files around is likely
cleaner if done within $D

> 
> > > +		if [[ ! -f ${ED}/tmp/multilib-include/${f} ]];
> > > then
> > > +			dodir "/tmp/multilib-include/${dir}"
> > > +			cat > "${ED}/tmp/multilib-include/${f}"
> > > <<_EOF_ || die +/* This file is auto-generated by
> > > autotools-multilib.eclass
> > > + * as a multilib-friendly wrapper to /${f}. For the original
> > > content,
> > > + * please see the files that are #included below.
> > > + */
> > > +_EOF_
> > > +		fi
> > > +
> > > +		local defs
> > > +		case "${ABI}" in
> > 
> > didn't you say $ABI may have name collisions?
> > considering the code below, it seems much safer to match on $CHOST
> 
> Yes, that's why I've put the whole matching inline instead of
> introducing a function to obtain the values. The current design
> of the eclass -- which was quite stupid of me -- doesn't provide
> a way to access that ABI_* thing. I am going to change that
> in the future but don't want to step into two different issues
> at a time.
> 
> At a first glance, $CHOST sounds like a neat idea. But I'm afraid it's
> not a really safe choice. From a quick glance, CHOST values for x86
> were:
> 
> i*86-pc-linux-gnu
> x86_64-pc-linux-gnu
> x86_64-pc-linux-gnux32
> 
> I feel like there's a lot of variables here, ends up in a bit ugly
> pattern matching IMO.

Well, almost every matching I've seen uses CHOST, because it's the
safest choice. For your example: 
i?86*) ;;
x86_64*gnux32) ;;
x86_64*) ;;

I haven't checked the details, but that'd be even better: freebsd
defines a different ABI (which is correct because it's incompatible)
but the #ifery for this wrapper is the same, so with this CHOST,
matching freebsd (or other OSes) comes for free. In the end it'll be
shorter :)


> > [...]
> > 
> > It'd be nice to have an attempt to support all the ABIs gentoo
> > supports in that file: I'd prefer to spot possible problems with
> > this solution vs. sedding a template for example to be seen before
> > having to rewrite it.
> > For example, IIRC, ppc64 defines both __powerpc__ and __powerpc64__,
> > the natural way would be:
> > #if defined(__powerpc64__)
> > ppc64 stuff
> > #elif defined(__powerpc__)
> > ppc stuff
> > #endif
> > 
> > with your approach that'd be:
> > #if defined(__powerpc__) && !defined(__powerpc64__)
> > ppc stuff
> > #elif defined(__powerpc64__)
> > ppc64 stuff
> > #endif
> 
> I had the same problem with x32. I've chosen the solution which worked
> and was easy to implement.

it's easy too :p


> > doing with a template has its disadvantages but allows more
> > flexibility in how the #ifery is written; I don't want to realize
> > your approach cannot deal with some weird arches after it has been
> > deployed.
> 
> To be honest, I can't really imagine how we could work with a template
> here. It would at least require the function to be aware of other ABIs
> which I really wanted to avoid. Of course, then there's the whole code
> to handle it, and honestly I don't have a vision how it would work.

Template (short version):

#if defined(__powerpc64__)
ABI=ppc64
#elif defined(__powerpc__)
ABI=ppc
#else
#error "fail"
#endif

When installing a header for $ABI:
sed -e "s/ABI=${ABI}\$/#include <foo>/"

After installing everything:
sed -e "s/ABI=.*/#error \"sorry, no support for \0\"/"


See, there's no need to be aware of any ABI and you even get more
meaningful #errors since you now know the #if path it used (because of
the \0) :)

The clear disadvantage of this approach is that every wrapped header
will have a long list of #if #elif #endif for every gentoo ABI, most of
the paths through these conditions will be invalid, and it will be
harder to read.
I really prefer generating the wrapper on the fly like you do but I
want to be sure it's good enough for all our use cases.

Alexis.

[prev in list] [next in list] [prev in thread] [next in thread]