| Message ID | 280363cd569a8c6e870107eb219597b42911fed2.1743859985.git.ramsay@ramsayjones.plus.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | miscellaneous build mods (part 1) | expand |
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] If you try an example that is given in the Makefile:312, then the make build: $ make V=1 DEFAULT_EDITOR='"C:\Program Files\Vim\gvim.exe" --nofork' all doc >m-out 2>&1 passes the folowing arguments to (respectively) gcc and asciidoc: -DDEFAULT_EDITOR='"\"C:\\Program Files\\Vim\\gvim.exe\" --nofork"' -a 'git-default-editor="C:\Program Files\Vim\gvim.exe" --nofork' whereas, the meson build: $ meson setup --optimization=2 -Ddocs=man,html -Ddefault_editor='"C:\Program Files\Vim\gvim.exe" --nofork' -Ddefault_pager=more -Dprefix=$HOME -Dpcre2=disabled build/ The Meson build system ... User defined options optimization : 2 prefix : /home/ramsay default_editor: "C:\Program Files\Vim\gvim.exe" --nofork default_pager : more docs : man,html pcre2 : disabled Found ninja-1.11.1 at /usr/bin/ninja $ similarly, passes the folowing arguments to (respectively) gcc and asciidoc: '-DDEFAULT_EDITOR=""C:\\Program Files\\Vim\\gvim.exe" --nofork"' '-agit-default-editor="C:/Program Files/Vim/gvim.exe" --nofork' If you now attempt a 'meson compile' it will, of course, fail to compile editor.c because the DEFAULT_EDITOR is the empty string ("") followed by (C:\\Program Files\\Vim\\gvim.exe" --nofork"). Also, note that the directory seperators have changed from \\ to / in the argument to asciidoc. [Again, spelunking the docs for meson, it said that "if you want quotes, you will have to do it yourself"! ;) ] OK, so I couldn't come up with any incantation which would fix this issue. I will have to admit defeat and ask someone who actually knows meson to fix it. :( Thanks. ATB, Ramsay Jones
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 :) Patrick [1]: https://mesonbuild.com/Reference-manual_elementary_str.html#strreplace
On 14/04/2025 08:54, Patrick Steinhardt wrote: > 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
On Mon, Apr 14, 2025 at 08:19:15PM +0100, Ramsay Jones wrote: > > > On 14/04/2025 08:54, Patrick Steinhardt wrote: > > 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!) Fair enough. Maybe I'll try to upstream a feature like this into Meson. It would be nice to have a `.quoted()` method on `str`, and it shouldn't be hard to do. > 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. Agreed. > 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! Yup. The only other thing was missing spaces around assignment operators, but that alone doesn't feel like it's worth a reroll. Thanks! Patrick
diff --git a/Documentation/meson.build b/Documentation/meson.build index 594546d68b..1642b6e2a3 100644 --- a/Documentation/meson.build +++ b/Documentation/meson.build @@ -242,6 +242,16 @@ if docs_backend == 'asciidoc' '--attribute=build_dir=' + meson.current_build_dir(), ] + pager_opt = get_option('default_pager') + if pager_opt != '' and pager_opt != 'less' + asciidoc_common_options += '-agit-default-pager=' + pager_opt + endif + + editor_opt = get_option('default_editor') + if editor_opt != '' and editor_opt != 'vi' + asciidoc_common_options += '-agit-default-editor=' + editor_opt + endif + documentation_deps = [ asciidoc_conf, ] @@ -279,6 +289,16 @@ elif docs_backend == 'asciidoctor' '--require', 'asciidoctor-extensions', ] + pager_opt = get_option('default_pager') + if pager_opt != '' and pager_opt != 'less' + asciidoc_common_options += '-agit-default-pager=' + pager_opt + endif + + editor_opt = get_option('default_editor') + if editor_opt != '' and editor_opt != 'vi' + asciidoc_common_options += '-agit-default-editor=' + editor_opt + endif + documentation_deps = [ asciidoctor_extensions, ] 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 = [ ]
Some preprocessor -Defines have defaults set in the source code when they have not been provided to the C compiler. In this case, there is no need to pass them on the command-line, unless the build requires a non-standard value. The build variables for DEFAULT_EDITOR and DEFAULT_PAGER have appropriate defaults ('vi' and 'less') set in the code. Add the preprocessor -Defines to the 'libgit_c_args' only if the values set with the corresponding 'options' are different to these standard values. Also, the 'git-var' documentation contains some conditional text which documents the chosen compiled in value, which would not read well for the standard values. Similar to the above, only add the corresponding '-a' attribute arguments to the 'asciidoc_common_options' variable, if the values set in the 'options' are different to these standard values. Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> --- Documentation/meson.build | 20 ++++++++++++++++++++ meson.build | 13 +++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-)