[PATCH xserver] meson: Fix module_dir configuration

[Aaron Plattner]

On 04/06/2018 09:59 PM, Aaron Plattner wrote:
> 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
>>> option.
>>>
>>> Signed-off-by: Aaron Plattner <aplattner at 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}',
>>> get_option('libdir')))
>>>       sdkconfig.set('includedir', join_paths('${prefix}',
>>> get_option('includedir')))
>>>       sdkconfig.set('datarootdir', join_paths('${prefix}',
>>> get_option('datadir')))
>>> -    sdkconfig.set('moduledir', join_paths('${exec_prefix}',
>>> get_option('module_dir')))
>>> +    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'), 'xorg'))
>>>       sdkconfig.set('sysconfigdir', join_paths('${datarootdir}',
>>> 'X11/xorg.conf.d'))
>>>   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 string')
>>>     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
>>        ${libdir}
>>
>>     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.

Sorry I forgot about this until now. I confirmed that I still get
/usr/lib/xorg/modules with this patch. I'll send out a v2.

I still get the server looking for /etc/X11/etc/xorg.conf. I'm not sure
what that's about yet.

-- Aaron

>> Thierry
>>
>> --- >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'), 'log')
>>   endif
>>   -module_dir = get_option('module_dir')
>> -if module_dir == ''
>> -    module_dir = join_paths(get_option('libdir'), 'xorg/modules')
>> -endif
>> +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',
>> get_option('xkb_default_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'), libexecdir))
>>   manpage_config.set('suid_wrapper_dir',
>> join_paths(get_option('prefix'), 'libexec'))
>> 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 string')
>>     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 at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel