On Sun, 24 Mar 2013 16:41:15 +0100 Michał Górny <mgo...@gentoo.org> wrote:
> On Sun, 24 Mar 2013 16:14:52 +0100 > Alexis Ballier <aball...@gentoo.org> wrote: > > > On Sat, 23 Mar 2013 17:26:38 +0100 > > Michał Górny <mgo...@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.