[PATCH][1/2] - Xserver Autoconfiguration Merge - Autotoolize Xserver

[GOMBAS Gabor]

Hi,

On Mon, Apr 18, 2005 at 11:19:03PM -0400, Shawn Starr wrote:

> Patches to autotoolize the Xorg Xserver

Some comments/suggestions after a quick reading of the patch:

- Neither AUTOMAKE_OPTIONS nor AM_INIT_AUTOMAKE states what is the
  minimum version of automake you are willing to support. I suggest
  requiring some recent version if you do not want to go through all the
  bugs present in older versions. My suggestion is to require at least
  automake-1.7 (1.6 still had some rather annoying "features").

- Do not use AC_TRY_RUN/AC_TRY_COMPILE/AC_TRY_LINK, but use the
  AC_*_IFELSE macros instead. The AC_TRY_* macros are declared
  obsolote in recent versions of autoconf. The AC_*_IFELSE macros
  already exist in autoconf-2.53 even if they were only announced in
  autoconf-2.55. In fact, autoconf-2.53 already implements
  AC_TRY_COMPILE as

  AC_DEFUN([AC_TRY_COMPILE],
  [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[$1]], [[$2]])], [$3], [$4])])

- Use AC_HELP_STRING for formatting the description of AC_ARG_WITH
  and AC_ARG_ENABLE macros. This will give more uniform "configure
  --help" output. Manally adding spaces to the description is more error
  prone.

- Minor style suggestion: decide between 'test x$var = xval' and
  'test "$var" = val'. Mixing the two styles inside the same
  configure.ac might be confusing.

- SysV shared memory check: I'd suggest only checking for the
  availability of the _interface_ in configure.ac by using
  AC_LINK_IFELSE, and not for the run-time availability using
  AC_TRY_RUN. Reason: you have to do a run-time check anyway because the
  user might boot a different kernel after compiling Xorg but before
  starting the X server. Avoiding AC_TRY_RUN/AC_RUN_IFELSE as much as
  possible makes life a lot easier when cross-compiling.

- Unaligned access checking and AC_RUN_IFELSE in general: If you cannot
  avoid AC_RUN_IFELSE, always put it inside AC_CACHE_CHECK to ensure
  that cross-compilation is still possible by using a pre-seeded
  config.cache.

- The DEFAULT_VENDOR_RELEASE variable seems to be completely redundant.
  The version is already passed to AC_INIT and it can be accessed using
  the AC_PACKAGE_VERSION macro (or the PACKAGE_VERSION shell variable).

- About macro argument quoting: although it is safe not to quote literal
  string arguments I'd suggest still quoting them anyway. The extra
  annoyance is small (just 2 characters), while there are more benefits:

  - configure.ac will look more consistent.
  - If somebody later replaces the string literal with a macro it will
    still "just work" as expected. Without proper quoting "interesting"
    things may happen.
  - "Always quote" is much easier to learn for people not familiar with
    autoconf/m4 quoting rules than "quote most of the time except...".

    Well, there is one exception: AC_HELP_STRING; life is though...
    (and there are other exceptions if you want to use the power of m4
    not just the autoconf macros but then you already have to be
    familiar with the quoting rules).

Gabor

-- 
     ---------------------------------------------------------
     MTA SZTAKI Computer and Automation Research Institute
                Hungarian Academy of Sciences
     ---------------------------------------------------------