[gentoo-dev] Proposal for flag-o-matic.eclass (append-ldflags)

[gentoo-dev] Proposal for flag-o-matic.eclass (append-ldflags)

[Jeremy Olexa]

Hello,
I am seeking a positive code review on the following change to
flag-o-matic.eclass, diff is below (reasons are below that):

%% cvs diff
Index: flag-o-matic.eclass
===================================================================
RCS file: /var/cvsroot/gentoo-x86/eclass/flag-o-matic.eclass,v
retrieving revision 1.126
diff -u -r1.126 flag-o-matic.eclass
--- flag-o-matic.eclass 3 Nov 2008 05:52:39 -0000       1.126
+++ flag-o-matic.eclass 25 Nov 2008 18:36:04 -0000
@@ -417,7 +417,8 @@

       x=""
       for x in "$@" ; do
-               test-flag-${comp} "${x}" && flags="${flags}${flags:+ }${x}"
+               test-flag-${comp} "${x}" && flags="${flags}${flags:+ }${x}" || \
+                       ewarn "removing ${x} because ${comp} rejected it"
       done

       echo "${flags}"
@@ -656,7 +657,7 @@
                       ewarn "Appending a library link instruction
(${flag}); libraries to link to should not be passed through LDFLAGS"
       done

-       export LDFLAGS="${LDFLAGS} $*"
+       export LDFLAGS="${LDFLAGS} $(test-flags "$@")"
       return 0
 }

Reason:
We hit this little gem in Gentoo Prefix when some ebuilds started
using flags that non-GNU linkers didn't accept and actually aborted
on. For example, the darwin ld does not accept --no-as-needed. So, the
initial work-around was to check to see if we had a GNU ld, and only
apply if so. (aside, further investigation revealed that GNU and
darwin ld are actually pretty non-clever in this regard. For example,
the hp-ux linker will just ignore non-valid flags) But this is not
very friendly to the lack of Gentoo Prefix developer availability. ;)

A convienient side effect of the above patch is that it protects
Gentoo users from typos in ebuilds. For example, "append-ldflags
--foo" will cause compilation to abort, but with the above patch, a
ewarn will be issued instead and compilation will continue. Besides
typos, if the GNU ld ever changes in some non-compatible way, there
will be less breakage.

There will be claims that additional checking to the flags should not
be added to Gentoo Linux because it is redundant (Gentoo only uses GNU
ld). However, time test-flags "$@" only takes 0m0.017s on my host. I
guess this number could be larger on less modern hosts..I don't know.

Comments? Good, bad, or otherwise? The Gentoo Prefix team always aims
to have as minimal diffs as possible from the gentoo-x86 tree, hence
the motivation for this request to review and inclusion into the
gentoo-x86 tree.

Thanks,
Jeremy

Re: [gentoo-dev] Proposal for flag-o-matic.eclass (append-ldflags)

[Donnie Berkholz]

[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]

On 10:33 Mon 08 Dec     , Jeremy Olexa wrote:
> Hello,
> I am seeking a positive code review on the following change to
> flag-o-matic.eclass, diff is below (reasons are below that):
> 
> %% cvs diff
> Index: flag-o-matic.eclass
> ===================================================================
> RCS file: /var/cvsroot/gentoo-x86/eclass/flag-o-matic.eclass,v
> retrieving revision 1.126
> diff -u -r1.126 flag-o-matic.eclass
> --- flag-o-matic.eclass 3 Nov 2008 05:52:39 -0000       1.126
> +++ flag-o-matic.eclass 25 Nov 2008 18:36:04 -0000
> @@ -417,7 +417,8 @@
> 
>        x=""
>        for x in "$@" ; do
> -               test-flag-${comp} "${x}" && flags="${flags}${flags:+ }${x}"
> +               test-flag-${comp} "${x}" && flags="${flags}${flags:+ }${x}" || \
> +                       ewarn "removing ${x} because ${comp} rejected it"
>        done
> 
>        echo "${flags}"
> @@ -656,7 +657,7 @@
>                        ewarn "Appending a library link instruction
> (${flag}); libraries to link to should not be passed through LDFLAGS"
>        done
> 
> -       export LDFLAGS="${LDFLAGS} $*"
> +       export LDFLAGS="${LDFLAGS} $(test-flags "$@")"
>        return 0
>  }

I like the consistency with other flags:

comet $ grep FLAGS.*test-fl /usr/portage/eclass/flag-o-matic.eclass 
	export CFLAGS=$(test-flags-CC ${CFLAGS})
	export CXXFLAGS=$(test-flags-CXX ${CXXFLAGS})
	export FFLAGS=$(test-flags-F77 ${FFLAGS})
	export FCFLAGS=$(test-flags-FC ${FCFLAGS})

-- 
Thanks,
Donnie

Donnie Berkholz
Developer, Gentoo Linux
Blog: http://dberkholz.wordpress.com

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[gentoo-dev] Re: Proposal for flag-o-matic.eclass (append-ldflags)

[Jeremy Olexa]

On Mon, Dec 8, 2008 at 10:33 AM, Jeremy Olexa <darkside@gentoo.org> wrote:
> Hello,
> I am seeking a positive code review on the following change to
> flag-o-matic.eclass, diff is below (reasons are below that):

Er, cancel that. The proposed patch isn't robust enough to catch
something like "append-ldflags -Wl,--bad-flag" As such, we will leave
it local to Gentoo Prefix until we can come up with a better idea.

Thanks,
Jeremy

Re: [gentoo-dev] Proposal for flag-o-matic.eclass (append-ldflags)

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1792 bytes --]

On Tuesday 09 December 2008 14:14:05 Donnie Berkholz wrote:
> On 10:33 Mon 08 Dec     , Jeremy Olexa wrote:
> > Hello,
> > I am seeking a positive code review on the following change to
> > flag-o-matic.eclass, diff is below (reasons are below that):
> >
> > %% cvs diff
> > Index: flag-o-matic.eclass
> > RCS file: /var/cvsroot/gentoo-x86/eclass/flag-o-matic.eclass,v
> > retrieving revision 1.126
> > diff -u -r1.126 flag-o-matic.eclass
> > --- flag-o-matic.eclass 3 Nov 2008 05:52:39 -0000       1.126
> > +++ flag-o-matic.eclass 25 Nov 2008 18:36:04 -0000
> > @@ -417,7 +417,8 @@
> >
> >        x=""
> >        for x in "$@" ; do
> > -               test-flag-${comp} "${x}" && flags="${flags}${flags:+
> > }${x}" +               test-flag-${comp} "${x}" &&
> > flags="${flags}${flags:+ }${x}" || \ +                       ewarn
> > "removing ${x} because ${comp} rejected it" done
> >
> >        echo "${flags}"
> > @@ -656,7 +657,7 @@
> >                        ewarn "Appending a library link instruction
> > (${flag}); libraries to link to should not be passed through LDFLAGS"
> >        done
> >
> > -       export LDFLAGS="${LDFLAGS} $*"
> > +       export LDFLAGS="${LDFLAGS} $(test-flags "$@")"
> >        return 0
> >  }
>
> I like the consistency with other flags:
>
> comet $ grep FLAGS.*test-fl /usr/portage/eclass/flag-o-matic.eclass
> 	export CFLAGS=$(test-flags-CC ${CFLAGS})
> 	export CXXFLAGS=$(test-flags-CXX ${CXXFLAGS})
> 	export FFLAGS=$(test-flags-F77 ${FFLAGS})
> 	export FCFLAGS=$(test-flags-FC ${FCFLAGS})

your grep ignores the bigger picture.  this is only in the strip-unsupported-
flag function.  so while we should flesh it out to include CPPFLAGS/LDFLAGS, 
it doesnt really address the original question.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [gentoo-dev] Re: Proposal for flag-o-matic.eclass (append-ldflags)

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 879 bytes --]

On Sunday 14 December 2008 16:39:17 Jeremy Olexa wrote:
> On Mon, Dec 8, 2008 at 10:33 AM, Jeremy Olexa <darkside@gentoo.org> wrote:
> > I am seeking a positive code review on the following change to
> > flag-o-matic.eclass, diff is below (reasons are below that):
>
> Er, cancel that. The proposed patch isn't robust enough to catch
> something like "append-ldflags -Wl,--bad-flag"

sounds like we need a test-flag-LD() func that can go the extra mile

> As such, we will leave
> it local to Gentoo Prefix until we can come up with a better idea.

stick it behind userland_GNU ?  i dont mind extending append-ldflags in such a 
way, but the "pro" you listed originally (protect users from typos in ebuilds) 
isnt a pro in my book -- it should fail.

i.e. something like:
append-ldflags() {
	use userland_GNU || set -- $(test-flag-LD "$@")
	......
}
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [gentoo-dev] Re: Proposal for flag-o-matic.eclass (append-ldflags)

[Fabian Groffen]

On 28-08-2009 16:08:59 -0400, Mike Frysinger wrote:
> > As such, we will leave
> > it local to Gentoo Prefix until we can come up with a better idea.
> 
> stick it behind userland_GNU ?  i dont mind extending append-ldflags
> in such a way, but the "pro" you listed originally (protect users from
> typos in ebuilds) isnt a pro in my book -- it should fail.

basically we would need something like "linker_GNU", since all Prefix
arches have userland_GNU set to true

at some point we did stuff like grepping for GNU in ld -v output, but
that's expensive, and it makes probably more sense to do some CHOST
matches with known configurations then, handling the obvious cases
(--{no-,}as-needed etc.)

> i.e. something like:
> append-ldflags() {
> 	use userland_GNU || set -- $(test-flag-LD "$@")
> 	......
> }

thanks


-- 
Fabian Groffen
Gentoo on a different level