xorg/driver/xf86-input-evdev: [PATCH] Correct make distchek using the same solution used in other packages.

Paulo César Pereira de Andrade pcpa at mandriva.com.br
Wed Feb 4 14:25:14 PST 2009


Dan Nicholson wrote:

>>  These cases should really be addressed in a different
>> way, as the addition of a option that is only useful to
>> pass distcheck is wrong.
>
> For the patch, please use backticks (``) rather than $() for command
> substitution since it's more portable. Also, please use the variable

  I just reverted to what was already in place, but I agree, and
would prefer to remove gratuitous bash'isms.

> $PKG_CONFIG. PKG_CHECK_MODULES (more specifically,
> PKG_PROG_PKG_CONFIG) has already looked up the tool for us, and using
> pkg-config directly means that we might not be honoring the user's
> wishes. I know that's not how the app-defaults patches were handled,
> but that doesn't make it right.

  You mean, it creates a XORG_sdkdir shell variable?
My reading of m4 cannot find it in pkg.m4 (maybe I did not look
carefully enough ...) That would be very useful indeed to replace
the current calls.

>>  Another "cosmetic" thing that should be addressed is
>> usage of something like:
>> PKG_CHECK_MODULES(XORG, xorg-server xproto $REQUIRED_MODULES)
>> AC_SUBST(XORG_CFLAGS)
>>
>>  First the automake macro says:
>>
>> Checking for XORG... yes
>>
>> while it should say something more like:
>>
>> Checking for xorg-server...
>> Checking for xproto...
>>
>> or maybe in the same line, but not really a xorg issue,
>> but a pkg-config issue?
>
> That's how PKG_CHECK_MODULES works. I think it used to spit out all
> the deps it's looking up, but it gets out of hand when there are a lot
> of them (look at the xserver to see how many modules are used at
> once). You can look in config.log to see the details if you need to.

  It is just illogical what it prints. If the problem is that
there may be too much things to print, it could just put print
something different, like:
Checking for required pkg-config modules...

>> The AC_SUBST issue is because there is a lot of mixed usage
>> in Makefiles of @XORG_CFLAGS@ and $(XORG_CFLAGS)
>
> It doesn't usually matter in practice, but it's nicer to use the
> variable $(var) rather than hardcoding the substitution so that a user
> could override it if necessary:
>
> make XORG_CFLAGS="-I/i/know/what/i_m/doing"

  When possible I have been using $(XORG_CFLAGS), but did not
want to "gratuitously" do a s/@(\w+)@/$(\1)/g in Makefile.am's...
but it could be a good thing to do, mainly for consistency.

> --
> Dan

Paulo




More information about the xorg mailing list