Re: lz4 --rm on Ubuntu 18.04 (Add LZ4 compression to pg_dump) - Mailing list pgsql-hackers

On Tue, Mar 14, 2023 at 12:16:16AM +0100, Tomas Vondra wrote:
> On 3/9/23 19:00, Tomas Vondra wrote:
> > On 3/9/23 01:30, Michael Paquier wrote:
> >> On Thu, Mar 09, 2023 at 12:39:08AM +0100, Tomas Vondra wrote:
> >>> IMO we should fix that. We have a bunch of buildfarm members running on
> >>> Ubuntu 18.04 (or older) - it's true none of them seems to be running TAP
> >>> tests. But considering how trivial the fix is ...
> >>>
> >>> Barring objections, I'll push a fix early next week.
> >>
> >> +1, better to change that, thanks.  Actually, would --rm be OK even on
> >> Windows?  As far as I can see, the CI detects a LZ4 command for the
> >> VS2019 environment but not the liblz4 libraries that would be needed
> >> to trigger the set of tests.
> > 
> > Thanks for noticing that. I'll investigate next week.
> 
> So, here's a fix that should (I think) replace the 'lz4 --rm' with a
> simple 'rm'. I have two doubts about this, though:
> 
> 1) I haven't found a simple way to inject additional command into the
> test. The pg_dump runs have a predefined list of "steps" to run:
> 
>   -- compress_cmd
>   -- glob_patterns
>   -- command_like
>   -- restore_cmd
> 
> and I don't think there's a good place to inject the 'rm' so I ended up
> adding a 'cleanup_cmd' right after 'compress_cmd'. But it seems a bit
> strange / hacky. Maybe there's a better way?

I don't know if there's a better way, and I don't think it's worth
complicating the tests by more than about 2 lines to handle this.

> 2) I wonder if Windows will know what 'rm' means. I haven't found any
> TAP test doing 'rm' and don't see 'rm' in any $ENV either.

CI probably will, since it's Andres' image built with git-tools and
other helpful stuff installed.  But in general I think it won't; perl is
being used for all the portable stuff.

*If* you wanted to do something to fix this, you could create a key
called files_to_remove_after_loading, and run unlink on those files
rather than running a shell command.  Or maybe just remove the file
unconditionally at the start of the script ?

> That being said, I have no idea how to make this work on our Windows CI.
> As mentioned, the environment is missing the lz4 library - there's a
> 
>   setup_additional_packages_script: |
>     REM choco install -y --no-progress ...
> 
> in the .yml file, but AFAICS the chocolatey does not have lz4 :-/

I updated what I'd done in the zstd patch to also run with LZ4.
This won't apply directly due to other patches, but you get the idea...

Maybe it'd be good to have a commented-out "wraps" hint like there is
for choco.  The downloaded files could be cached, too.

diff --git a/.cirrus.yml b/.cirrus.yml
index a3977a4036e..b4387a739f3 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -644,9 +644,11 @@ task:
     vcvarsall x64
     mkdir subprojects
     meson wrap install zstd
-    meson configure -D zstd:multithread=enabled --force-fallback-for=zstd
+    meson wrap install lz4
+    meson subprojects download
+    meson configure -D zstd:multithread=enabled --force-fallback-for=zstd --force-fallback-for=lz4
     set CC=c:\ProgramData\chocolatey\lib\ccache\tools\ccache-4.8-windows-x86_64\ccache.exe cl.exe
-    meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Db_pch=false
-Dextra_lib_dirs=c:\openssl\1.1\lib-Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR%
-DPG_TEST_EXTRA="%PG_TEST_EXTRA%"-D zstd=enabled -Dc_args="/Z7 /MDd" build
 
+    meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Db_pch=false
-Dextra_lib_dirs=c:\openssl\1.1\lib-Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR%
-DPG_TEST_EXTRA="%PG_TEST_EXTRA%"-D zstd=enabled -D lz4=enabled -Dc_args="/Z7 /MDd" build
 
 
   build_script: |
     vcvarsall x64

-- 
Justin