On 04/03/2018 02:27 AM, Thierry Reding wrote:
On Mon, Apr 02, 2018 at 02:31:20PM -0700, Aaron Plattner wrote:
meson.build has code to set the module_dir variable to
${libdir}/xorg/modules if the module_dir option string is empty.
However, this has several problems:

1. The variable is only used for an unused @moduledir@ substitution in
    the man page. The rule for xorg-server.pc uses option('module_dir')
    directly instead.
2. The 'module_dir' option has a default value of 'xorg/modules' so the
    above rule doesn't do anything by default.
3. The xorg-server.pc rule uses ${exec_prefix}/option('module_dir'), so
    the effect of #2 is that the default moduledir is different between
    autoconf and meson. E.g. if ${prefix} is /X, then you get

      autoconf: moduledir=/X/lib/xorg/modules
      meson:    moduledir=/X/xorg/modules

Ugh... you're right. I was setting --libexecdir ${prefix}/lib in my
scripts, which is why I wasn't seeing the above inconsistency.

Fix this by using the module_dir variable when generating
xorg-server.pc, and by removing the default value for the module_dir

Signed-off-by: Aaron Plattner <aplatt...@nvidia.com>
  meson.build       | 2 +-
  meson_options.txt | 3 +--
  2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/meson.build b/meson.build
index 277534093b94..3e3f808b2d7a 100644
--- a/meson.build
+++ b/meson.build
@@ -595,7 +595,7 @@ if build_xorg
      sdkconfig.set('libdir', join_paths('${exec_prefix}', 
      sdkconfig.set('includedir', join_paths('${prefix}', 
      sdkconfig.set('datarootdir', join_paths('${prefix}', 
-    sdkconfig.set('moduledir', join_paths('${exec_prefix}', 
+    sdkconfig.set('moduledir', join_paths('${exec_prefix}', module_dir))

This would still give us an inconsistent path if the user passed in some
other, relative directory for module_dir, right? So if they passed:

        --module-dir foo/xorg/modules

they'd get /usr/foo/xorg/modules in the pkg-config files, but the server
would actually look for it in /usr/lib/foo/xorg/modules.

      sdkconfig.set('sdkdir', join_paths('${prefix}', get_option('includedir'), 
      sdkconfig.set('sysconfigdir', join_paths('${datarootdir}', 
diff --git a/meson_options.txt b/meson_options.txt
index 5c7be0e26ce5..4cf8349ba9e5 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -19,8 +19,7 @@ option('builder_addr', type: 'string', description: 'Builder 
address', value: 'x
  option('builder_string', type: 'string', description: 'Additional builder 
option('log_dir', type: 'string')
-option('module_dir', type: 'string', value: 'xorg/modules',
-       description: 'X.Org modules directory')
+option('module_dir', type: 'string', description: 'X.Org modules directory')

It seems somewhat backwards to me to avoid the feature of assigning a
default value for an option and was totally surprising to me because I
didn't go look for a default assignment in meson.build, so I completely
missed it.

Why don't we do the following:

        1) define that module_dir is either absolute or relative to

        2) keep the default in the option declaration

        3) change the module_dir variable to always be made up of the
           libdir and module_dir options joined

For 3), the Meson documentation specifies that if any of the arguments
to join_paths() is an absolute path, all arguments before it are
dropped, so it automatically deals with the case where users specify an
absolute path.

Something like the below squashed into your patch.

Thanks Thierry. The patch below seems reasonable to me, but I'm out of the office until Wednesday so I won't be able to test it until then.

-- Aaron


--- >8 ---
diff --git a/meson.build b/meson.build
index 3e3f808b2d7a..33e2f6d88b1a 100644
--- a/meson.build
+++ b/meson.build
@@ -267,10 +267,7 @@ if log_dir == ''
      log_dir = join_paths(get_option('prefix'), get_option('localstatedir'), 
-module_dir = get_option('module_dir')
-if module_dir == ''
-    module_dir = join_paths(get_option('libdir'), 'xorg/modules')
+module_dir = join_paths(get_option('libdir'), get_option('module_dir'))
if glamor_option == 'auto'
      build_glamor = build_xorg or build_xwayland
@@ -510,7 +507,7 @@ manpage_config.set('XKB_DFLT_LAYOUT', 
  manpage_config.set('XKB_DFLT_VARIANT', get_option('xkb_default_variant'))
  manpage_config.set('XKB_DFLT_OPTIONS', get_option('xkb_default_options'))
  manpage_config.set('bundle_id_prefix', '...')
-manpage_config.set('modulepath', join_paths(get_option('prefix'), module_dir))
+manpage_config.set('modulepath', module_dir)
  # wtf doesn't this work
  # manpage_config.set('suid_wrapper_dir', join_paths(get_option('prefix'), 
  manpage_config.set('suid_wrapper_dir', join_paths(get_option('prefix'), 
diff --git a/meson_options.txt b/meson_options.txt
index 4cf8349ba9e5..1c8775c3fdaa 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -19,7 +19,8 @@ option('builder_addr', type: 'string', description: 'Builder 
address', value: 'x
  option('builder_string', type: 'string', description: 'Additional builder 
option('log_dir', type: 'string')
-option('module_dir', type: 'string', description: 'X.Org modules directory')
+option('module_dir', type: 'string', value: 'xorg/modules',
+       description: 'X.Org modules directory (absolute or relative to the 
directory specified by the libdir option)')
  option('default_font_path', type: 'string')
option('glx', type: 'boolean', value: true)

xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Reply via email to