Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Gentoo>
  3. Dev
Proposal for flag-o-matic.eclass (append-ldflags)
darkside at gentoo
Dec 8, 2008, 8:33 AM
Post #1 of 6 (957 views)
Permalink
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: Proposal for flag-o-matic.eclass (append-ldflags) [ In reply to ]
dberkholz at gentoo
Dec 9, 2008, 11:14 AM
Post #2 of 6 (899 views)
Permalink
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
Re: Proposal for flag-o-matic.eclass (append-ldflags) [ In reply to ]
darkside at gentoo
Dec 14, 2008, 1:39 PM
Post #3 of 6 (918 views)
Permalink
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: Proposal for flag-o-matic.eclass (append-ldflags) [ In reply to ]
vapier at gentoo
Aug 28, 2009, 1:04 PM
Post #4 of 6 (749 views)
Permalink
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

Attached Files:

Re: Re: Proposal for flag-o-matic.eclass (append-ldflags) [ In reply to ]
vapier at gentoo
Aug 28, 2009, 1:08 PM
Post #5 of 6 (742 views)
Permalink
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

Attached Files:

Re: Re: Proposal for flag-o-matic.eclass (append-ldflags) [ In reply to ]
grobian at gentoo
Aug 29, 2009, 12:23 AM
Post #6 of 6 (742 views)
Permalink
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

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal