[PATCH xserver] config: declare xserver private dependencies in xorg-server.pc

[Dan Nicholson]

On Tue, Jun 15, 2010 at 7:45 AM, Gaetan Nadon <memsize at videotron.ca> wrote:
> On Mon, 2010-06-14 at 21:03 -0700, Dan Nicholson wrote:
>
> On Mon, Jun 14, 2010 at 6:59 PM, Gaetan Nadon <memsize at videotron.ca> wrote:
>> On Mon, 2010-06-14 at 15:07 -0700, Dan Nicholson wrote:
>>
>> On Sun, Jun 13, 2010 at 1:49 PM, Gaetan Nadon <memsize at videotron.ca>
>> wrote:
>>> Any module (drivers) depending on xserver also depends on some of the
>>> server private dependencies. Any driver including xf86.h depends on
>>> xext, kbproto, inputproto and randr.
>>>
>>> These dependencies are in separate packages, so anything can happen,
>>> removal, wrong version, etc... and the driver fails during compilation.
>>> Having the private dependencies declared will ensure all packages the
>>> server depends on are present and at the correct version.
>>>
>>> Currently each module attempts to check for server dependencies with
>>> various degrees of accuracy. With this patch, the driver will only need
>>> to check for its own explicit dependencies.
>>>
>>> Signed-off-by: Gaetan Nadon <memsize at videotron.ca>
>>> ---
>>>  configure.ac      |    9 ++++++++-
>>>  xorg-server.pc.in |    1 +
>>>  2 files changed, 9 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/configure.ac b/configure.ac
>>> index 4ada8f5..cb33637 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -793,9 +793,13 @@ WINDOWSWMPROTO="windowswmproto"
>>>  APPLEWMPROTO="applewmproto >= 1.4"
>>>
>>>  dnl Core modules for most extensions, et al.
>>> -REQUIRED_MODULES="[randrproto >= 1.2.99.3] [renderproto >= 0.11]
>>> [fixesproto >= 4.1] [damageproto >= 1.1] [xcmiscproto >= 1.2.0]
>>> [xextproto
>>> >= 7.0.99.3] [xproto >= 7.0.17] [xtrans >= 1.2.2] [bigreqsproto >= 1.1.0]
>>> fontsproto [inputproto >= 1.9.99.902] [kbproto >= 1.0.3]"
>>> +SDK_REQUIRED_MODULES="[randrproto >= 1.2.99.3] [renderproto >= 0.11]
>>> [xextproto >= 7.0.99.3] [inputproto >= 1.9.99.902] [kbproto >= 1.0.3]"
>>> +REQUIRED_MODULES="[fixesproto >= 4.1] [damageproto >= 1.1] [xcmiscproto
>>> >= 1.2.0] [xproto >= 7.0.17] [xtrans >= 1.2.2] [bigreqsproto >= 1.1.0]
>>> fontsproto $SDK_REQUIRED_MODULES"
>>>  REQUIRED_LIBS="xfont xau"
>>>
>>> +# Make SDK_REQUIRED_MODULES available for inclusion in xorg-server.pc
>>> +AC_SUBST(SDK_REQUIRED_MODULES)
>>> +
>>>  dnl List of libraries that require a specific version
>>>  LIBAPPLEWM="applewm >= 1.4"
>>>  LIBDMX="dmx >= 1.0.99.1"
>>> @@ -947,6 +951,7 @@ if test "x$XV" = xyes; then
>>>        AC_DEFINE(XV, 1, [Support Xv extension])
>>>        AC_DEFINE(XvExtension, 1, [Build Xv extension])
>>>        REQUIRED_MODULES="$REQUIRED_MODULES $VIDEOPROTO"
>>> +       SDK_REQUIRED_MODULES="$SDK_REQUIRED_MODULES $VIDEOPROTO"
>>>  else
>>>        XVMC=no
>>>  fi
>>> @@ -1036,6 +1041,7 @@ case "$DRI2,$HAVE_DRI2PROTO" in
>>>        yes,yes | auto,yes)
>>>                AC_DEFINE(DRI2, 1, [Build DRI2 extension])
>>>                DRI2=yes
>>> +               SDK_REQUIRED_MODULES="$SDK_REQUIRED_MODULES $DRI2PROTO"
>>>                ;;
>>>  esac
>>>  AM_CONDITIONAL(DRI2, test "x$DRI2" = xyes)
>>> @@ -1074,6 +1080,7 @@ if test "x$XINERAMA" = xyes; then
>>>        AC_DEFINE(XINERAMA, 1, [Support Xinerama extension])
>>>        AC_DEFINE(PANORAMIX, 1, [Internal define for Xinerama])
>>>        REQUIRED_MODULES="$REQUIRED_MODULES $XINERAMAPROTO"
>>> +       SDK_REQUIRED_MODULES="$SDK_REQUIRED_MODULES $XINERAMAPROTO"
>>>  fi
>>>
>>>  AM_CONDITIONAL(XACE, [test "x$XACE" = xyes])
>>> diff --git a/xorg-server.pc.in b/xorg-server.pc.in
>>> index 44f886a..1fa4fb5 100644
>>> --- a/xorg-server.pc.in
>>> +++ b/xorg-server.pc.in
>>> @@ -16,5 +16,6 @@ Name: xorg-server
>>>  Description: Modular X.Org X Server
>>>  Version: @PACKAGE_VERSION@
>>>  Requires: pixman-1 pciaccess xproto >= 7.0.17
>>> +Requires.private: @SDK_REQUIRED_MODULES@
>>
>> Could you also move the xproto requirement from REQUIRED_MODULES to
>> SDK_REQUIRED_MODULES so it's always populated from configure.ac? Maybe
>> that can go in another commit. Probably all the modules should be in
>> Requires.private, but that's probably another commit, too.
>>
>> I originally put all modules (save for xproto) in Requires.private. Julien
>> suggested to put only the modules exposed in the sdk.
>
> Hmm, not sure why that would be since it wouldn't make a difference
> for the proto packages. pkg-config --cflags will return the settings
> whether they're in Requires or Requires.private, even on the broken
> pkg-config releases.
>
> I thought private meant the cflags would not be returned.
>
>> As for xproto, all drivers use it explicitly and need the include path. I
>> thought it was safer to leave it there as there might be an older driver
>> that did not list xproto on the PKG_CHECK_MODULES statement and could
>> potentially break. I am not sure I understand why you were suggesting
>> that.
>
> Right now we have xproto >= 7.0.17 explicitly in both configure and
> xorg-server.pc.in. If it's only in configure, then when you bump the
> requirement for some reason, you don't risk the .pc file getting out
> of sync.
>
> That's an xserver issue where xproto >= 7.0.17 is hand-coded in 2 places,
> configure.ac and xorg-server.pc.
> A variable should be used.

Right. It's already there in REQUIRED_MODULES. If you move it to
SDK_REQUIRED_MODULES and remove the current reference in
xorg-server.pc.in, then you've got your variable.

> I don't know how it would break any drivers. The installed .pc file
> will contain xproto >= 7.0.17 whether you substitute it from configure
> or not. Whether the drivers have listed xproto in their
> PKG_CHECK_MODULES shouldn't make a difference on how the xserver
> specifies its requirement.
>
> Correct, with my misunderstanding cleared. That leaves one question: what's
> the difference between Requires and Requires.private?
>
> Requires.private:
>            A list of packages required by this package. The difference from
>            Requires is that the packages listed under Requires.private are
>            not taken into account when a flag list is computed for
>            dynamically linked executable (i.e., when --static was not
>            specified).  In the situation where each .pc file corresponds to
>            a library, Requires.private shall be used exclusively to specify
>            the dependencies between the libraries.
>
> Reading http://people.freedesktop.org/~dbn/pkg-config-guide.html:
>
> Finally, the Cflags contains the compiler flags for using the library.
> Unlike the Libs field, there is not a private variant of Cflags.
> This is because the data types and macro definitions are needed regardless
> of the linking scenario.
>
> My conclusion is that all the proto packages the server depends on should be
> declared in Requires.private, including xproto. Any driver wishing to use a
> specific protocol should check for it in PKG_CHECK_MODULES. That is what
> they do today. In additional of working correctly, this code will also
> reflect and document the system design.

Yes, there's almost no reason to ever use Requires. One exception is
if you want to keep compatibility with pkg-config versions where the
Requires.private handling was broken. This is the reason you see
Requires used a lot in the xorg .pc files. Either way, it will only
affect the libraries, so you can leave pixman and pciaccess under
Requires and nothing will change.

> This only leaves the question of "other modules" upon which the server
> depends but not the drivers. It is tempting to craft a shorter list as this
> patch did, but it is not future proof. One day, a header from, say,
> damageproto can find it's way through xf86.h and no one will think of
> updating SDK_REQUIRED_MODULES. I think it is both correct and less error
> prone for the future to use the current REQUIRED_MODULES list. This is also
> consistent with "drivers are part of the server" architecture statement.

Future additions to {SDK_,}REQUIRED_MODULES would require a review of
whether the module is exposed in the API. Not much you can do here
except be careful when introducing API.

--
Dan