Patchwork [2/4] xorg-driver-common: Add configure and install appends from meta-intel

login
register
mail settings
Submitter Darren Hart
Date Sept. 5, 2013, 12:18 a.m.
Message ID <7e9270b1c56aeeb6ed22210cab98d2675b092d5d.1378339881.git.dvhart@linux.intel.com>
Download mbox | patch
Permalink /patch/57373/
State New
Headers show

Comments

Darren Hart - Sept. 5, 2013, 12:18 a.m.
As part of pulling in the more generic components of the meta-intel
xserver infrastructure, include the configure and install appends from
the meta-intel version of xorg-driver-common.

Modify the configure prepend to use ${S} in the paths explicitly as
without this the script appears to be running in a build/ directory
instead of the source directory, and cannot find the configure.ac.
This prepend is required for at least the -intel and -mga drivers.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: Ross Burton <ross.burton@intel.com>
Cc: Nitin A Kamble <nitin.a.kamble@intel.com>
---
 .../xorg-driver/xorg-driver-common.inc             |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)
Otavio Salvador - Sept. 5, 2013, 12:54 a.m.
On Wed, Sep 4, 2013 at 9:18 PM, Darren Hart <dvhart@linux.intel.com> wrote:
> As part of pulling in the more generic components of the meta-intel
> xserver infrastructure, include the configure and install appends from
> the meta-intel version of xorg-driver-common.
>
> Modify the configure prepend to use ${S} in the paths explicitly as
> without this the script appears to be running in a build/ directory
> instead of the source directory, and cannot find the configure.ac.
> This prepend is required for at least the -intel and -mga drivers.
>
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> Cc: Ross Burton <ross.burton@intel.com>
> Cc: Nitin A Kamble <nitin.a.kamble@intel.com>
> ---
>  .../xorg-driver/xorg-driver-common.inc             |   17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/meta/recipes-graphics/xorg-driver/xorg-driver-common.inc b/meta/recipes-graphics/xorg-driver/xorg-driver-common.inc
> index 5f5d282..2d4f2ce 100644
> --- a/meta/recipes-graphics/xorg-driver/xorg-driver-common.inc
> +++ b/meta/recipes-graphics/xorg-driver/xorg-driver-common.inc
> @@ -5,7 +5,7 @@ SECTION = "x11/drivers"
>  LICENSE = "MIT-X"
>
>  PE = "2"
> -INC_PR = "r21"
> +INC_PR = "r22"

This is need because you had a PRINC?

>  DEPENDS = "virtual/xserver xproto randrproto util-macros"
>
> @@ -39,3 +39,18 @@ def add_abi_depends(d, name):
>
>      pn = d.getVar("PN", True)
>      d.appendVar('RDEPENDS_' + pn, ' ' + abi)
> +
> +# AC_CHECK_FILE doesn't work when cross compiling, so we create a replacement
> +# macro that simply assumes the test succeeds. This is required for at least
> +# the -intel and -mga drivers.
> +do_configure_prepend () {
> +       echo 'AC_DEFUN(CC_AC_CHECK_FILE, $2)' > ${S}/configure.ac.new
> +       sed 's/AC_CHECK_FILE/CC_AC_CHECK_FILE/g' ${S}/configure.ac >> ${S}/configure.ac.new
> +       mv ${S}/configure.ac.new ${S}/configure.ac
> +}
> +
> +# FIXME: We don't want to include the libtool archives (*.la) from modules
> +# directory, as they serve no useful purpose. Upstream should fix Makefile.am
> +do_install_append() {
> +       find ${D}${libdir}/xorg/modules -regex ".*\.la$" | xargs rm -f --
> +}

Agreed.
Darren Hart - Sept. 5, 2013, 3:33 a.m.
On Wed, 2013-09-04 at 21:54 -0300, Otavio Salvador wrote:
> On Wed, Sep 4, 2013 at 9:18 PM, Darren Hart <dvhart@linux.intel.com> wrote:
> > As part of pulling in the more generic components of the meta-intel
> > xserver infrastructure, include the configure and install appends from
> > the meta-intel version of xorg-driver-common.
> >
> > Modify the configure prepend to use ${S} in the paths explicitly as
> > without this the script appears to be running in a build/ directory
> > instead of the source directory, and cannot find the configure.ac.
> > This prepend is required for at least the -intel and -mga drivers.
> >
> > Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> > Cc: Ross Burton <ross.burton@intel.com>
> > Cc: Nitin A Kamble <nitin.a.kamble@intel.com>
> > ---
> >  .../xorg-driver/xorg-driver-common.inc             |   17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/meta/recipes-graphics/xorg-driver/xorg-driver-common.inc b/meta/recipes-graphics/xorg-driver/xorg-driver-common.inc
> > index 5f5d282..2d4f2ce 100644
> > --- a/meta/recipes-graphics/xorg-driver/xorg-driver-common.inc
> > +++ b/meta/recipes-graphics/xorg-driver/xorg-driver-common.inc
> > @@ -5,7 +5,7 @@ SECTION = "x11/drivers"
> >  LICENSE = "MIT-X"
> >
> >  PE = "2"
> > -INC_PR = "r21"
> > +INC_PR = "r22"
> 
> This is need because you had a PRINC?
> 

It is my understanding that this change will ensure recipes including
this file will get rebuilt after this patch is merged and will get the
updated install append, clearing the .la files.

Yes/No?

Thanks for the review,

Darren

> >  DEPENDS = "virtual/xserver xproto randrproto util-macros"
> >
> > @@ -39,3 +39,18 @@ def add_abi_depends(d, name):
> >
> >      pn = d.getVar("PN", True)
> >      d.appendVar('RDEPENDS_' + pn, ' ' + abi)
> > +
> > +# AC_CHECK_FILE doesn't work when cross compiling, so we create a replacement
> > +# macro that simply assumes the test succeeds. This is required for at least
> > +# the -intel and -mga drivers.
> > +do_configure_prepend () {
> > +       echo 'AC_DEFUN(CC_AC_CHECK_FILE, $2)' > ${S}/configure.ac.new
> > +       sed 's/AC_CHECK_FILE/CC_AC_CHECK_FILE/g' ${S}/configure.ac >> ${S}/configure.ac.new
> > +       mv ${S}/configure.ac.new ${S}/configure.ac
> > +}
> > +
> > +# FIXME: We don't want to include the libtool archives (*.la) from modules
> > +# directory, as they serve no useful purpose. Upstream should fix Makefile.am
> > +do_install_append() {
> > +       find ${D}${libdir}/xorg/modules -regex ".*\.la$" | xargs rm -f --
> > +}
> 
> Agreed.
>
Ross Burton - Sept. 5, 2013, 9:53 a.m.
On 5 September 2013 01:18, Darren Hart <dvhart@linux.intel.com> wrote:
> +# AC_CHECK_FILE doesn't work when cross compiling, so we create a replacement
> +# macro that simply assumes the test succeeds. This is required for at least
> +# the -intel and -mga drivers.
> +do_configure_prepend () {
> +       echo 'AC_DEFUN(CC_AC_CHECK_FILE, $2)' > ${S}/configure.ac.new
> +       sed 's/AC_CHECK_FILE/CC_AC_CHECK_FILE/g' ${S}/configure.ac >> ${S}/configure.ac.new
> +       mv ${S}/configure.ac.new ${S}/configure.ac
> +}

I've been systematically fixing the drivers that use AC_CHECK_FILE so
now as far as I know we're left with just one (-mga), for which my
patch has been submitted upstream.

I'd prefer to see the patch retained and upstream fixed instead of
working around the breakage, because the test that configure.ac is
attempting to do is valid, it's just done wrong.

>+# FIXME: We don't want to include the libtool archives (*.la) from modules
>+# directory, as they serve no useful purpose. Upstream should fix Makefile.am
>+do_install_append() {
>+       find ${D}${libdir}/xorg/modules -regex ".*\.la$" | xargs rm -f --
>+}

Sadly when using libtool you can't "opt-out" of installing .la files
unless you delete the files that just got installed.  In my opinion
this is something that needs to be done globally instead of
per-recipe.

Ross
Otavio Salvador - Sept. 5, 2013, 11:37 a.m.
On Thu, Sep 5, 2013 at 6:53 AM, Burton, Ross <ross.burton@intel.com> wrote:
> On 5 September 2013 01:18, Darren Hart <dvhart@linux.intel.com> wrote:
>> +# AC_CHECK_FILE doesn't work when cross compiling, so we create a replacement
>> +# macro that simply assumes the test succeeds. This is required for at least
>> +# the -intel and -mga drivers.
>> +do_configure_prepend () {
>> +       echo 'AC_DEFUN(CC_AC_CHECK_FILE, $2)' > ${S}/configure.ac.new
>> +       sed 's/AC_CHECK_FILE/CC_AC_CHECK_FILE/g' ${S}/configure.ac >> ${S}/configure.ac.new
>> +       mv ${S}/configure.ac.new ${S}/configure.ac
>> +}
>
> I've been systematically fixing the drivers that use AC_CHECK_FILE so
> now as far as I know we're left with just one (-mga), for which my
> patch has been submitted upstream.
>
> I'd prefer to see the patch retained and upstream fixed instead of
> working around the breakage, because the test that configure.ac is
> attempting to do is valid, it's just done wrong.

So I think this should be dropped from here.

>>+# FIXME: We don't want to include the libtool archives (*.la) from modules
>>+# directory, as they serve no useful purpose. Upstream should fix Makefile.am
>>+do_install_append() {
>>+       find ${D}${libdir}/xorg/modules -regex ".*\.la$" | xargs rm -f --
>>+}
>
> Sadly when using libtool you can't "opt-out" of installing .la files
> unless you delete the files that just got installed.  In my opinion
> this is something that needs to be done globally instead of
> per-recipe.

So autotools.bbclass?
Ross Burton - Sept. 5, 2013, 11:40 a.m.
On 5 September 2013 12:37, Otavio Salvador <otavio@ossystems.com.br> wrote:
>> Sadly when using libtool you can't "opt-out" of installing .la files
>> unless you delete the files that just got installed.  In my opinion
>> this is something that needs to be done globally instead of
>> per-recipe.
>
> So autotools.bbclass?

Exactly.  Phil sent patches to do this previously, and I've a
branch... somewhere that has another unfinished version.

Ross
Richard Purdie - Sept. 5, 2013, 12:14 p.m.
On Thu, 2013-09-05 at 12:40 +0100, Burton, Ross wrote:
> On 5 September 2013 12:37, Otavio Salvador <otavio@ossystems.com.br> wrote:
> >> Sadly when using libtool you can't "opt-out" of installing .la files
> >> unless you delete the files that just got installed.  In my opinion
> >> this is something that needs to be done globally instead of
> >> per-recipe.
> >
> > So autotools.bbclass?
> 
> Exactly.  Phil sent patches to do this previously, and I've a
> branch... somewhere that has another unfinished version.

I continue to have mixed feelings about this: 

* We've not done side by side build comparisons with something like
buildhistory.

* Figuring out any runtime issues with dlopen is the hardest part and we
don't actually have real data on whether there are issues there or not.

* We'd be deviating from the way the libtool authors suggest their tool
should operate. This makes filing bug reports and interacting with
upstream harder. I continue to dream of a libtool with working sysroot
support for example instead of carrying our hacks/workarounds/bugfixes.

* They are used in places, for example the darwin shlibs code currently
uses them. It could be updated to use otool these days mind but I'd
probably make the current code a fallback for unknown arches since it is
guaranteed to work everywhere.

So whilst I know several people love to just delete them, its a slightly
more complex issue than that.

Cheers,

Richard (who isn't keen on libtool and would rather replace that instead)
Phil Blundell - Sept. 5, 2013, 12:23 p.m.
On Thu, 2013-09-05 at 13:14 +0100, Richard Purdie wrote:
> * Figuring out any runtime issues with dlopen is the hardest part and we
> don't actually have real data on whether there are issues there or not.

Do we have any particular reason to believe that there would be issues
(and if so, what they might be)?  I guess it should be easy enough to
gather data if we know what we're looking for.

Right now the .la files go into the -dev packages anyway and hence
aren't usually going to be available at runtime for anything calling
dlopen() on the target.  Is it native packages you're concerned about?

> * We'd be deviating from the way the libtool authors suggest their tool
> should operate. This makes filing bug reports and interacting with
> upstream harder. 

This is true, though interacting with libtool upstream already seems to
be hard enough that I'm not sure this would make a material difference.

> * They are used in places, for example the darwin shlibs code currently
> uses them. It could be updated to use otool these days mind but I'd
> probably make the current code a fallback for unknown arches since it is
> guaranteed to work everywhere.

There's no reason in principle that folks on darwin (and/or
hitherto-undiscovered architectures) couldn't retain the .la files if
they wanted.  The original patch that I sent used a DISTRO_FEATURE to
control this and those people building for darwin could simply refrain
from setting it.  Alternatively we could make it explicitly conditional
on TARGET_OS or some such if there are reasons to believe that some
targets do genuinely require this stuff.

p.
Ross Burton - Sept. 5, 2013, 1:48 p.m.
On 5 September 2013 13:23, Phil Blundell <pb@pbcl.net> wrote:
> On Thu, 2013-09-05 at 13:14 +0100, Richard Purdie wrote:
>> * Figuring out any runtime issues with dlopen is the hardest part and we
>> don't actually have real data on whether there are issues there or not.
>
> Do we have any particular reason to believe that there would be issues
> (and if so, what they might be)?  I guess it should be easy enough to
> gather data if we know what we're looking for.

There won't be any problems with dlopen() because that only looks at
.so files.  Any issues will be at build time, I wouldn't be surprised
if some dependencies changed due to a difference in eg pkgconfig vs
.la linkage.  Then again some other distros blanket-delete .la files
by default so I don't expect any critical problems.

>> * They are used in places, for example the darwin shlibs code currently
>> uses them. It could be updated to use otool these days mind but I'd
>> probably make the current code a fallback for unknown arches since it is
>> guaranteed to work everywhere.
>
> There's no reason in principle that folks on darwin (and/or
> hitherto-undiscovered architectures) couldn't retain the .la files if
> they wanted.  The original patch that I sent used a DISTRO_FEATURE to
> control this and those people building for darwin could simply refrain
> from setting it.  Alternatively we could make it explicitly conditional
> on TARGET_OS or some such if there are reasons to believe that some
> targets do genuinely require this stuff.

I don't think a distro feature is the right approach, but however it's
set can easily be forced using a machine override for Darwin.

Ross
Darren Hart - Sept. 5, 2013, 3 p.m.
On Thu, 2013-09-05 at 10:53 +0100, Burton, Ross wrote:
> On 5 September 2013 01:18, Darren Hart <dvhart@linux.intel.com> wrote:
> > +# AC_CHECK_FILE doesn't work when cross compiling, so we create a replacement
> > +# macro that simply assumes the test succeeds. This is required for at least
> > +# the -intel and -mga drivers.
> > +do_configure_prepend () {
> > +       echo 'AC_DEFUN(CC_AC_CHECK_FILE, $2)' > ${S}/configure.ac.new
> > +       sed 's/AC_CHECK_FILE/CC_AC_CHECK_FILE/g' ${S}/configure.ac >> ${S}/configure.ac.new
> > +       mv ${S}/configure.ac.new ${S}/configure.ac
> > +}
> 
> I've been systematically fixing the drivers that use AC_CHECK_FILE so
> now as far as I know we're left with just one (-mga), for which my
> patch has been submitted upstream.

The xf86-video-intel driver also appears to need it (just FYI re your
comment on mga).
Richard Purdie - Sept. 5, 2013, 3:32 p.m.
On Thu, 2013-09-05 at 13:23 +0100, Phil Blundell wrote:
> On Thu, 2013-09-05 at 13:14 +0100, Richard Purdie wrote:
> > * Figuring out any runtime issues with dlopen is the hardest part and we
> > don't actually have real data on whether there are issues there or not.
> 
> Do we have any particular reason to believe that there would be issues
> (and if so, what they might be)?  I guess it should be easy enough to
> gather data if we know what we're looking for.
> 
> Right now the .la files go into the -dev packages anyway and hence
> aren't usually going to be available at runtime for anything calling
> dlopen() on the target.  Is it native packages you're concerned about?

The -dev package argument is a good one, we seem to be surviving without
those and that probably addresses the concern I was thinking of. I'm
also thinking of ltdl and its dlopen wrapping, not dlopen itself which
is a much smaller subset of recipes.

> > * We'd be deviating from the way the libtool authors suggest their tool
> > should operate. This makes filing bug reports and interacting with
> > upstream harder. 
> 
> This is true, though interacting with libtool upstream already seems to
> be hard enough that I'm not sure this would make a material difference.

It does seem somewhat tricky, agreed.

> > * They are used in places, for example the darwin shlibs code currently
> > uses them. It could be updated to use otool these days mind but I'd
> > probably make the current code a fallback for unknown arches since it is
> > guaranteed to work everywhere.
> 
> There's no reason in principle that folks on darwin (and/or
> hitherto-undiscovered architectures) couldn't retain the .la files if
> they wanted.  The original patch that I sent used a DISTRO_FEATURE to
> control this and those people building for darwin could simply refrain
> from setting it.  Alternatively we could make it explicitly conditional
> on TARGET_OS or some such if there are reasons to believe that some
> targets do genuinely require this stuff.

There are ways to avoid problems for darwin. Using otool instead of
the .la files would be nice, equally mangling DISTRO_FEATURES based on
an SDK override is possible, albeit ugly. The la handling code will
likely bitrot as soon as its disabled by default and metadata starts
getting dropped which is probably a bigger concern. The number of SDK
darwin builds is currently a tiny portion of the usercase and codebase.

I guess the .la files just don't seem to be a big issue that some people
seem to paint them as. Why do we need to remove them and deviate from
the upstream?

Anyhow, if someone can show some world builds with and without the .la
files and see what build history shows we'd be closer to taking a patch
to turning them off. If we do it, we'll have to do it everywhere for
everything since the remaining code will bitrot badly IMO otherwise.

Cheers,

Richard
Ross Burton - Sept. 5, 2013, 4:23 p.m.
On 5 September 2013 16:32, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> I guess the .la files just don't seem to be a big issue that some people
> seem to paint them as. Why do we need to remove them and deviate from
> the upstream?

You appear to be defending choices made by libtool upstream here.  Are
you unwell? :)

> Anyhow, if someone can show some world builds with and without the .la
> files and see what build history shows we'd be closer to taking a patch
> to turning them off. If we do it, we'll have to do it everywhere for
> everything since the remaining code will bitrot badly IMO otherwise.

I'll try and hack something up this week as a proof of concept.

Ross

Patch

diff --git a/meta/recipes-graphics/xorg-driver/xorg-driver-common.inc b/meta/recipes-graphics/xorg-driver/xorg-driver-common.inc
index 5f5d282..2d4f2ce 100644
--- a/meta/recipes-graphics/xorg-driver/xorg-driver-common.inc
+++ b/meta/recipes-graphics/xorg-driver/xorg-driver-common.inc
@@ -5,7 +5,7 @@  SECTION = "x11/drivers"
 LICENSE = "MIT-X"
 
 PE = "2"
-INC_PR = "r21"
+INC_PR = "r22"
 
 DEPENDS = "virtual/xserver xproto randrproto util-macros"
 
@@ -39,3 +39,18 @@  def add_abi_depends(d, name):
 
     pn = d.getVar("PN", True)
     d.appendVar('RDEPENDS_' + pn, ' ' + abi)
+
+# AC_CHECK_FILE doesn't work when cross compiling, so we create a replacement
+# macro that simply assumes the test succeeds. This is required for at least
+# the -intel and -mga drivers.
+do_configure_prepend () {
+	echo 'AC_DEFUN(CC_AC_CHECK_FILE, $2)' > ${S}/configure.ac.new
+	sed 's/AC_CHECK_FILE/CC_AC_CHECK_FILE/g' ${S}/configure.ac >> ${S}/configure.ac.new
+	mv ${S}/configure.ac.new ${S}/configure.ac
+}
+
+# FIXME: We don't want to include the libtool archives (*.la) from modules
+# directory, as they serve no useful purpose. Upstream should fix Makefile.am
+do_install_append() {
+	find ${D}${libdir}/xorg/modules -regex ".*\.la$" | xargs rm -f --
+}