Modernize the build system v2 problem

Modernize the build system v2 problem

[Ramsay Jones]

Hi Patrick,

I took your 'Modernize the build system' v2 series, from 2024-10-09, as patches
from the mailing list and put them on top of master@ef8ce8f3d4 ("Start the 2.48
cycle", 2024-10-10). I had to hand edit the 14th patch to change the version
number from DEF_VER=v2.47.0 to DEF_VER=v2.47.GIT, because of the change of base.
(It would probably have been easier to just base it on v2.47.0, but what would
be the fun in that! :) ).

In order to fix the 'dependency loop' error/warning from make, I applied the
following change:

    diff --git a/Makefile b/Makefile
    index dc60b2581d..c7b28975ac 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -3219,7 +3219,7 @@ test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(
     
     all:: $(TEST_PROGRAMS) $(test_bindir_programs) $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG)
     
    -bin-wrappers/%: bin-wrappers/wrap-for-bin.sh
    +$(test_bindir_programs): bin-wrappers/wrap-for-bin.sh
     	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     	     -e 's|@BUILD_DIR@|$(shell pwd)|' \
     	     -e 's|@GIT_TEXTDOMAINDIR@|$(shell pwd)/po/build/locale|' \

There are several ways to fix it, but this seemed like the easiest. I suspect
that you have already fixed this.

Having determined that the 'make' build procedure seemed to be unaffected,
I now tried the meson build. I had to install meson at this point (ninja
came along for the ride). I have never used meson or ninja before.

At this point I had to fix another fallout from changing the base:

    diff --git a/meson.build b/meson.build
    index 338d472bc6..54557eee03 100644
    --- a/meson.build
    +++ b/meson.build
    @@ -194,7 +194,6 @@ libgit_sources = [
       'reftable/block.c',
       'reftable/blocksource.c',
       'reftable/iter.c',
    -  'reftable/publicbasics.c',
       'reftable/merged.c',
       'reftable/pq.c',
       'reftable/reader.c',

Everything seemed to go without a hitch after that, as far as the build is
concerned, but when I did a 'ninja test' I ended up with three failures:

  Summary of Failures:
  
   979/1028 t9500-gitweb-standalone-no-errors              FAIL           12.36s   exit status 1
   980/1028 t9501-gitweb-standalone-http-status            FAIL            2.19s   exit status 1
   981/1028 t9502-gitweb-standalone-parse-output           FAIL            2.22s   exit status 1
  
  Ok:                 1025
  Expected Fail:      0   
  Fail:               3   
  Unexpected Pass:    0   
  Skipped:            0   
  Timeout:            0   
  
  Full log written to /home/ramsay/git/build/meson-logs/testlog.txt
  FAILED: meson-internal__test 
  /usr/bin/meson test --no-rebuild --print-errorlogs
  ninja: build stopped: subcommand failed.

The failure is caused by an (apparently) mangled 'gitweb.cgi' file. Since I
still had the make build file, I could directly compare the files:

  $ diff ../gitweb/gitweb.cgi gitweb/gitweb.cgi | wc -l
  160
  $ 

I won't bore you with the whole diff, but it begins like so:

  $ diff ../gitweb/gitweb.cgi gitweb/gitweb.cgi
  83c83
  < our $GIT = "/home/ramsay/bin/git";
  ---
  > our $GIT = "/usr/local/bin/git";
  91c91
  < our $project_maxdepth = 2007;
  ---
  > our $project_maxdepth = "2007";
  2497c2497
  < 		{ regexp => qr/^\@\@{$num_sign} /, class => "chunk_header"},
  ---
  > 		{ regexp => qr/^@@{$num_sign} /, class => "chunk_header"},
  2521c2521
  < 		$line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;
  ---
  > 		$line =~ m/^@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) @{2}(.*)$/;

  ...

  $ 

Note that, after the 'template variables' have been substituted, many (all?)
character pairs \@ are replaced with @ (ie the backslashes have gone walkabout).
This results in compilation errors in the 'gitweb.log' file, for example the
log file for the t9500-*.sh test, looks like:

  $ cat gitweb.log
  [Mon Oct 14 15:12:33 2024] gitweb.cgi: Possible unintended interpolation of @2 in string at /home/ramsay/git/build/gitweb/gitweb.cgi line 2521.
  [Mon Oct 14 15:12:33 2024] gitweb.cgi: Possible unintended interpolation of @3 in string at /home/ramsay/git/build/gitweb/gitweb.cgi line 2593.
  [Mon Oct 14 15:12:33 2024] gitweb.cgi: Possible unintended interpolation of @vrfy in string at /home/ramsay/git/build/gitweb/gitweb.cgi line 4212.
  [Mon Oct 14 15:12:33 2024] gitweb.cgi: Global symbol "@vrfy" requires explicit package name (did you forget to declare "my @vrfy"?) at /home/ramsay/git/build/gitweb/gitweb.cgi line 4212.
  [Mon Oct 14 15:12:33 2024] gitweb.cgi: Execution of /home/ramsay/git/build/gitweb/gitweb.cgi aborted due to compilation errors.
  $ 
 
So, keeping in mind that I know absolutely nothing about meson, it seems that
the 'configure_file' function is mangling the 'gitweb.perl' file. I assume
that you are not seeing this, so I suspect that you are using a newer (fixed)
version than me. :(

  $ meson --version
  1.3.2
  $ ninja --version
  1.11.1
  $ 

This is on Linux Mint 22.1, which is based on Ubuntu LTS, so not that old!

I am about to try converting the Makefile 'procedure' into a shell script
to use in both the Makefile and in the meson.build file (I see that the
'configure_file' procedure can take a 'command' to generate the file).

Note that '$project_maxdepth' is a snowflake in the make procedure! :)

Any thoughts?

Thanks.

ATB,
Ramsay Jones

 

Re: Modernize the build system v2 problem

[Patrick Steinhardt]

On Mon, Oct 14, 2024 at 05:59:03PM +0100, Ramsay Jones wrote:
> Hi Patrick,
> 
> I took your 'Modernize the build system' v2 series, from 2024-10-09, as patches
> from the mailing list and put them on top of master@ef8ce8f3d4 ("Start the 2.48
> cycle", 2024-10-10). I had to hand edit the 14th patch to change the version
> number from DEF_VER=v2.47.0 to DEF_VER=v2.47.GIT, because of the change of base.
> (It would probably have been easier to just base it on v2.47.0, but what would
> be the fun in that! :) ).
> 
> In order to fix the 'dependency loop' error/warning from make, I applied the
> following change:
> 
>     diff --git a/Makefile b/Makefile
>     index dc60b2581d..c7b28975ac 100644
>     --- a/Makefile
>     +++ b/Makefile
>     @@ -3219,7 +3219,7 @@ test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(
>      
>      all:: $(TEST_PROGRAMS) $(test_bindir_programs) $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG)
>      
>     -bin-wrappers/%: bin-wrappers/wrap-for-bin.sh
>     +$(test_bindir_programs): bin-wrappers/wrap-for-bin.sh
>      	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>      	     -e 's|@BUILD_DIR@|$(shell pwd)|' \
>      	     -e 's|@GIT_TEXTDOMAINDIR@|$(shell pwd)/po/build/locale|' \

Yup, I've already got this exact change pending in v3.

> There are several ways to fix it, but this seemed like the easiest. I suspect
> that you have already fixed this.
> 
> Having determined that the 'make' build procedure seemed to be unaffected,
> I now tried the meson build. I had to install meson at this point (ninja
> came along for the ride). I have never used meson or ninja before.
> 
> At this point I had to fix another fallout from changing the base:
> 
>     diff --git a/meson.build b/meson.build
>     index 338d472bc6..54557eee03 100644
>     --- a/meson.build
>     +++ b/meson.build
>     @@ -194,7 +194,6 @@ libgit_sources = [
>        'reftable/block.c',
>        'reftable/blocksource.c',
>        'reftable/iter.c',
>     -  'reftable/publicbasics.c',
>        'reftable/merged.c',
>        'reftable/pq.c',
>        'reftable/reader.c',

This one, as well.

[snip]
> So, keeping in mind that I know absolutely nothing about meson, it seems that
> the 'configure_file' function is mangling the 'gitweb.perl' file. I assume
> that you are not seeing this, so I suspect that you are using a newer (fixed)
> version than me. :(

I didn't, no, so this is quite helpful to me.

>   $ meson --version
>   1.3.2
>   $ ninja --version
>   1.11.1
>   $ 
> 
> This is on Linux Mint 22.1, which is based on Ubuntu LTS, so not that old!
> 
> I am about to try converting the Makefile 'procedure' into a shell script
> to use in both the Makefile and in the meson.build file (I see that the
> 'configure_file' procedure can take a 'command' to generate the file).
> 
> Note that '$project_maxdepth' is a snowflake in the make procedure! :)
> 
> Any thoughts?

I'll investigate tomorrow and come up with a fix. I'd prefer to not have
to script our way around this, as eventually I would like to get rid of
shellscripts in the build system altogether. But that is something for
the future anyway, and for now I'll do whatever it takes to fix issues.

In any case, I'll try to reproduce the reported issues tomorrow and will
then come up with a fix. Thanks a lot for testing things!

Patrick

Re: Modernize the build system v2 problem

[Eli Schwartz]

[-- Attachment #1.1: Type: text/plain, Size: 3865 bytes --]

On 10/14/24 12:59 PM, Ramsay Jones wrote:
> Everything seemed to go without a hitch after that, as far as the build is
> concerned, but when I did a 'ninja test' I ended up with three failures:
> 
>   Summary of Failures:
>   
>    979/1028 t9500-gitweb-standalone-no-errors              FAIL           12.36s   exit status 1
>    980/1028 t9501-gitweb-standalone-http-status            FAIL            2.19s   exit status 1
>    981/1028 t9502-gitweb-standalone-parse-output           FAIL            2.22s   exit status 1
>   
>   Ok:                 1025
>   Expected Fail:      0   
>   Fail:               3   
>   Unexpected Pass:    0   
>   Skipped:            0   
>   Timeout:            0   
>   
>   Full log written to /home/ramsay/git/build/meson-logs/testlog.txt
>   FAILED: meson-internal__test 
>   /usr/bin/meson test --no-rebuild --print-errorlogs
>   ninja: build stopped: subcommand failed.
> 
> The failure is caused by an (apparently) mangled 'gitweb.cgi' file. Since I
> still had the make build file, I could directly compare the files:
> 
>   $ diff ../gitweb/gitweb.cgi gitweb/gitweb.cgi | wc -l
>   160
>   $ 
> 
> I won't bore you with the whole diff, but it begins like so:
> 
>   $ diff ../gitweb/gitweb.cgi gitweb/gitweb.cgi
>   83c83
>   < our $GIT = "/home/ramsay/bin/git";
>   ---
>   > our $GIT = "/usr/local/bin/git";
>   91c91
>   < our $project_maxdepth = 2007;
>   ---
>   > our $project_maxdepth = "2007";
>   2497c2497
>   < 		{ regexp => qr/^\@\@{$num_sign} /, class => "chunk_header"},
>   ---
>   > 		{ regexp => qr/^@@{$num_sign} /, class => "chunk_header"},
>   2521c2521
>   < 		$line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;
>   ---
>   > 		$line =~ m/^@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) @{2}(.*)$/;
> 
>   ...
> 
>   $ 
> 
> Note that, after the 'template variables' have been substituted, many (all?)
> character pairs \@ are replaced with @ (ie the backslashes have gone walkabout).
> This results in compilation errors in the 'gitweb.log' file, for example the
> log file for the t9500-*.sh test, looks like:
> 
>   $ cat gitweb.log
>   [Mon Oct 14 15:12:33 2024] gitweb.cgi: Possible unintended interpolation of @2 in string at /home/ramsay/git/build/gitweb/gitweb.cgi line 2521.
>   [Mon Oct 14 15:12:33 2024] gitweb.cgi: Possible unintended interpolation of @3 in string at /home/ramsay/git/build/gitweb/gitweb.cgi line 2593.
>   [Mon Oct 14 15:12:33 2024] gitweb.cgi: Possible unintended interpolation of @vrfy in string at /home/ramsay/git/build/gitweb/gitweb.cgi line 4212.
>   [Mon Oct 14 15:12:33 2024] gitweb.cgi: Global symbol "@vrfy" requires explicit package name (did you forget to declare "my @vrfy"?) at /home/ramsay/git/build/gitweb/gitweb.cgi line 4212.
>   [Mon Oct 14 15:12:33 2024] gitweb.cgi: Execution of /home/ramsay/git/build/gitweb/gitweb.cgi aborted due to compilation errors.
>   $ 
>  
> So, keeping in mind that I know absolutely nothing about meson, it seems that
> the 'configure_file' function is mangling the 'gitweb.perl' file. I assume
> that you are not seeing this, so I suspect that you are using a newer (fixed)
> version than me. :(
> 
>   $ meson --version
>   1.3.2
>   $ ninja --version
>   1.11.1
>   $ 


I recognize this: https://github.com/mesonbuild/meson/pull/13302

Note that for files which can change semi-regularly as part of
development it may be better to avoid configure_file() and create
something like edit-files.sh.in which then produces build rules, not
configure-time changes. This would sidestep the problem entirely as
you'd then process gitweb.cgi via e.g. a sed script.

(The main reason though is because it avoids reconfiguring meson when
the gitweb.cgi script is modified via a patch / git pull.)


-- 
Eli Schwartz

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

Re: Modernize the build system v2 problem

[Ramsay Jones]

On 14/10/2024 20:19, Eli Schwartz wrote:
[snip]

>> So, keeping in mind that I know absolutely nothing about meson, it seems that
>> the 'configure_file' function is mangling the 'gitweb.perl' file. I assume
>> that you are not seeing this, so I suspect that you are using a newer (fixed)
>> version than me. :(
>>
>>   $ meson --version
>>   1.3.2
>>   $ ninja --version
>>   1.11.1
>>   $ 
> 
> 
> I recognize this: https://github.com/mesonbuild/meson/pull/13302

Yep, that looks pretty much on point. :)

Having said that, after squinting at the patch text for about ten
minutes, I'm still not sure it would leave all valid perl syntax
alone. So, ...

> 
> Note that for files which can change semi-regularly as part of
> development it may be better to avoid configure_file() and create
> something like edit-files.sh.in which then produces build rules, not
> configure-time changes. This would sidestep the problem entirely as
> you'd then process gitweb.cgi via e.g. a sed script.

Showing ignorance of meson, I don't quite follow the 'edit-files.sh.in'
suggestion, but I had intended (before Patrick's email) to use a script
to generate 'gitweb.cgi' as the 'command' in the 'configure_file'.
It feels like you are suggesting something else.

> (The main reason though is because it avoids reconfiguring meson when
> the gitweb.cgi script is modified via a patch / git pull.)

Thanks.

ATB,
Ramsay Jones