Re: [-SPAM-] Re: [PATCH v2 03/13] meson.build: only set build variables for non-default values

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Apr 14, 2025, 19:19 UTC
Message-ID: <7c5a2998-fe71-495f-8841-64e5b2ad03f2@ramsayjones.plus.com>
In-Reply-To: <Z_y_Tp5pfJ-gZLEF@pks.im>

On 14/04/2025 08:54, Patrick Steinhardt wrote:

Show 52 quoted lines

> On Sun, Apr 06, 2025 at 08:49:54PM +0100, Ramsay Jones wrote:
>>
>>
>> On 06/04/2025 20:38, Ramsay Jones wrote:
>> [snip]
>>> diff --git a/meson.build b/meson.build
>>> index 88a29fd043..efd0bd3319 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -693,10 +693,8 @@ endif
>>>  # These variables are used for building libgit.a.
>>>  libgit_c_args = [
>>>    '-DBINDIR="' + get_option('bindir') + '"',
>>> -  '-DDEFAULT_EDITOR="' + get_option('default_editor') + '"',
>>>    '-DDEFAULT_GIT_TEMPLATE_DIR="' + get_option('datadir') / 'git-core/templates' + '"',
>>>    '-DDEFAULT_HELP_FORMAT="' + get_option('default_help_format') + '"',
>>> -  '-DDEFAULT_PAGER="' + get_option('default_pager') + '"',
>>>    '-DETC_GITATTRIBUTES="' + get_option('gitattributes') + '"',
>>>    '-DETC_GITCONFIG="' + get_option('gitconfig') + '"',
>>>    '-DFALLBACK_RUNTIME_PREFIX="' + get_option('prefix') + '"',
>>> @@ -708,6 +706,17 @@ libgit_c_args = [
>>>    '-DPAGER_ENV="' + get_option('pager_environment') + '"',
>>>    '-DSHELL_PATH="' + fs.as_posix(shell.full_path()) + '"',
>>>  ]
>>> +
>>> +editor_opt = get_option('default_editor')
>>> +if editor_opt != '' and editor_opt != 'vi'
>>> +  libgit_c_args += '-DDEFAULT_EDITOR="' + editor_opt + '"'
>>> +endif
>>> +
>>> +pager_opt = get_option('default_pager')
>>> +if pager_opt != '' and pager_opt != 'less'
>>> +  libgit_c_args += '-DDEFAULT_PAGER="' + pager_opt + '"'
>>> +endif
>>> +
>>>  libgit_include_directories = [ '.' ]
>>>  libgit_dependencies = [ ]
>>>  
>>
>>
>> It would be somewhat remiss of me to not mention here that this does not
>> work for any but the simplest of values! :( If you set a simple single
>> 'bareword' like 'vim' or 'more' (even '~/bin/vi') then every thing works
>> just fine. However, if the value contains any of (at least) the following
>> characters: single quote, double quote or backslash, then things
>> stop working!
>>
>> [I spent one whole evening (and a bit - always something else to 'try')
>> trying to 'fix' this problem, without success]
> 
> Shouldn't it be possible to escape these values via `.replace()` [1]? I
> suspect that you already tried, but wanted to ask anyway :)

Yep. :)

I still haven't studied the meson documentation, but when I searched for variations of 'quotes', the results showed that '... if you want quotes, you will have to do it yourself ...'. So, I eventually found '.replace()' in the 'string operations' section of the docs and tried to reproduce what the Makefile does (see #2382):

  ifdef DEFAULT_EDITOR
  DEFAULT_EDITOR_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_EDITOR)))"
  DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))

  BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
  endif

which I translated into (on top of these patches):

  diff --git a/meson.build b/meson.build
  index 8f8a258064..608d665fd3 100644
  --- a/meson.build
  +++ b/meson.build
  @@ -708,7 +708,11 @@ libgit_c_args = [
 
   editor_opt = get_option('default_editor')
   if editor_opt != '' and editor_opt != 'vi'
  -  libgit_c_args += '-DDEFAULT_EDITOR="' + editor_opt + '"'
  +  editor_opt = editor_opt.replace('\\', '\\\\')
  +  editor_opt = editor_opt.replace('"', '\"')
  +  editor_opt = '"' + editor_opt + '"'
  +  editor_opt = editor_opt.replace('\'', '\\\'')
  +  libgit_c_args += '-DDEFAULT_EDITOR=' + editor_opt
   endif
 
[Actually, I think the very first attempt had:

  libgit_c_args += '-DDEFAULT_EDITOR=\'' + editor_opt + '\''

but meson, for some reason, adds a set of ' around the whole -D argument to gcc, so I got rid of them - but it still didn't work!]

Along with many, many, *many* such permutations! (trying to debug this is hard work, with no help from meson).

So, just a little earlier this evening I read an email from Karthik ([PATCH v2 3/4] meson: add support for 'hdr-check') in which he mentioned a problem with backslashes and referenced a github issue on the mesonbuild repo [0], which is worth a read. ;)

Sorry I couldn't fix this issue, but it seems to be (in part) an issue with meson. (Of course the example I used, which is taken directly from the Makefile, happens to be particularly good at demonstrating the problem!)

In any event, I think the current patch is a strict improvement, even if it may need to be updated at a later date. I hope you agree.

Thank you for taking the time to review this series. I think this patch was the only review comment that required a response - please let me know, if that is not the case!

Thanks!

ATB, Ramsay Jones

[0]: https://github.com/mesonbuild/meson/issues/1564