Обсуждение: pgsql: Check that we have a working tar before trying to use it
Check that we have a working tar before trying to use it Issue exposed by commit edc2332550 and the buildfarm. Backpatch to release 14 where this usage started. Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/f920f7e799c587228227ec94356c760e3f3d5f2b Modified Files -------------- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Andrew Dunstan <andrew@dunslane.net> writes:
> Check that we have a working tar before trying to use it
I don't like the way you did this:
+ || system_log($tar, '--version') != 0);
ISTM that effectively restricts the test to only running
on machines with GNU tar, which basically removes all the
interest of it. We know what GNU tar does ... it's the
weird legacy tar versions that might teach us something.
See 57b5a9646 for a recent example of the sort of bug
this test can no longer find.
regards, tom lane
On 12/8/21 10:39, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> Check that we have a working tar before trying to use it > I don't like the way you did this: > > + || system_log($tar, '--version') != 0); > > ISTM that effectively restricts the test to only running > on machines with GNU tar, which basically removes all the > interest of it. We know what GNU tar does ... it's the > weird legacy tar versions that might teach us something. > See 57b5a9646 for a recent example of the sort of bug > this test can no longer find. > > I tested on a freebsd system before I did this for that reason. It's not using GNU tar: user@freebsd:~ $ tar --version bsdtar 3.5.1 - libarchive 3.5.1 zlib/1.2.11 liblzma/5.2.5 bz2lib/1.0.8 Do you have an alternative test we could use? This is almost exactly what we use to test for lz4. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> On 12/8/21 10:39, Tom Lane wrote:
>> ISTM that effectively restricts the test to only running
>> on machines with GNU tar, which basically removes all the
>> interest of it. We know what GNU tar does ... it's the
>> weird legacy tar versions that might teach us something.
>> See 57b5a9646 for a recent example of the sort of bug
>> this test can no longer find.
> I tested on a freebsd system before I did this for that reason. It's not
> using GNU tar:
> user@freebsd:~ $ tar --version
> bsdtar 3.5.1 - libarchive 3.5.1 zlib/1.2.11 liblzma/5.2.5 bz2lib/1.0.8
Right, but on AIX:
tgl@gcc119:[/home/tgl]tar --version
tar: Not a recognized flag: -
Usage: tar -{c|r|t|u|x} [ -BdDEFhilmopRUsvwZ ] [ -Number ] [ -f TarFil e ]
[ -b Blocks ] [ -S [ Feet ] | [ Feet@Density ] | [ Blocksb ] ]
[ -L InputList ] [-X ExcludeFile] [ -N Blocks ] [ -C Directory ] File ...
Usage: tar {c|r|t|u|x} [ bBdDEfFhilLXmNopRsSUvwZ[0-9] ] ]
[ Blocks ] [ TarFile ] [ InputList ] [ ExcludeFile ]
[ [ Feet ] | [ Feet@Density ] | [ Blocksb ] ] [-C Directory ] File ...
tgl@gcc119:[/home/tgl]echo $?
1
> Do you have an alternative test we could use?
I think you need to be straight up about it, say
touch foo; tar cf foo.tar foo
(At least on the Unix machines I tried, it works to use /dev/null
as the output file, saving one cleanup step. But I don't know
if that'll work on Windows.)
regards, tom lane
On 12/8/21 11:15, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 12/8/21 10:39, Tom Lane wrote:
>>> ISTM that effectively restricts the test to only running
>>> on machines with GNU tar, which basically removes all the
>>> interest of it. We know what GNU tar does ... it's the
>>> weird legacy tar versions that might teach us something.
>>> See 57b5a9646 for a recent example of the sort of bug
>>> this test can no longer find.
>> I tested on a freebsd system before I did this for that reason. It's not
>> using GNU tar:
>> user@freebsd:~ $ tar --version
>> bsdtar 3.5.1 - libarchive 3.5.1 zlib/1.2.11 liblzma/5.2.5 bz2lib/1.0.8
> Right, but on AIX:
>
> tgl@gcc119:[/home/tgl]tar --version
> tar: Not a recognized flag: -
> Usage: tar -{c|r|t|u|x} [ -BdDEFhilmopRUsvwZ ] [ -Number ] [ -f TarFil e ]
> [ -b Blocks ] [ -S [ Feet ] | [ Feet@Density ] | [ Blocksb ] ]
> [ -L InputList ] [-X ExcludeFile] [ -N Blocks ] [ -C Directory ] File ...
> Usage: tar {c|r|t|u|x} [ bBdDEfFhilLXmNopRsSUvwZ[0-9] ] ]
> [ Blocks ] [ TarFile ] [ InputList ] [ ExcludeFile ]
> [ [ Feet ] | [ Feet@Density ] | [ Blocksb ] ] [-C Directory ] File ...
> tgl@gcc119:[/home/tgl]echo $?
> 1
>
>> Do you have an alternative test we could use?
> I think you need to be straight up about it, say
>
> touch foo; tar cf foo.tar foo
>
> (At least on the Unix machines I tried, it works to use /dev/null
> as the output file, saving one cleanup step. But I don't know
> if that'll work on Windows.)
>
>
Fair enough.
Actually, I think it makes more sense to simply revert this. bowerbird,
like all buildfarm animals, has a working tar. It's just that this one
is apparently seriously confused by using paths with a drive spec.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> Actually, I think it makes more sense to simply revert this. bowerbird,
> like all buildfarm animals, has a working tar.
Right, it doesn't seem like tar is the biggest lift when setting up
a buildfarm critter.
> It's just that this one
> is apparently seriously confused by using paths with a drive spec.
Hm, so how would this test have handled that anyway?
regards, tom lane
On 12/8/21 12:05, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> Actually, I think it makes more sense to simply revert this. bowerbird, >> like all buildfarm animals, has a working tar. > Right, it doesn't seem like tar is the biggest lift when setting up > a buildfarm critter. > >> It's just that this one >> is apparently seriously confused by using paths with a drive spec. > Hm, so how would this test have handled that anyway? > > It would have passed the test and failed the invocation as before. It turns out that the tar being used is from the (old) MsysGit installation. So I'm rearranging the path so it finds the system tar instead. That should take care of things. (testing first before I do anything else). On my initial test the system tar handled paths with drive specs just fine. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Hi, On 2021-12-08 12:53:29 -0500, Andrew Dunstan wrote: > It would have passed the test and failed the invocation as before. It > turns out that the tar being used is from the (old) MsysGit > installation. So I'm rearranging the path so it finds the system tar > instead. That should take care of things. (testing first before I do > anything else). On my initial test the system tar handled paths with > drive specs just fine. I just hit this issue. The tar included in a just-updated git-for-windows fails with "tar: Cannot connect to c: resolve failed". Git appears to install itself in front of c:\windows\system32 in PATH (which contains the system tar). Which I think means that after edc2332550 this will have to be manually addressed by most people running tests on windows manually? That doesn't seem great :( Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> Git appears to install itself in front of c:\windows\system32 in PATH (which
> contains the system tar). Which I think means that after edc2332550 this will
> have to be manually addressed by most people running tests on windows
> manually? That doesn't seem great :(
Can we make the default be the system tar (ie a full path, not
just "tar")? I have no idea if that's a stable thing on Windows ...
regards, tom lane
Michael Paquier <michael@paquier.xyz> writes:
> ... I got some copies of tar.exe lying
> around, one from a git installation and a second one from Msys, but
> both don't work even if $ENV{TAR} points at them :/
What goes wrong exactly?
The test I proposed ("tar cf foo.tar foo") doesn't seem like it
will catch the actual problem, which IIUC is that these tars
don't like a drive letter in the file name(s). Can we arrange
to strip that in what's passed to $TAR ? If not, it seems like
we have to test for exactly that point (ie Windows-only test).
regards, tom lane
On 12/8/21 21:16, Andres Freund wrote: > Hi, > > On 2021-12-08 12:53:29 -0500, Andrew Dunstan wrote: >> It would have passed the test and failed the invocation as before. It >> turns out that the tar being used is from the (old) MsysGit >> installation. So I'm rearranging the path so it finds the system tar >> instead. That should take care of things. (testing first before I do >> anything else). On my initial test the system tar handled paths with >> drive specs just fine. > I just hit this issue. The tar included in a just-updated git-for-windows > fails with "tar: Cannot connect to c: resolve failed". > > Git appears to install itself in front of c:\windows\system32 in PATH (which > contains the system tar). Which I think means that after edc2332550 this will > have to be manually addressed by most people running tests on windows > manually? That doesn't seem great :( > I'm not sure how you're installing git, or even if you're installing it at all - maybe it comes with the image you're using. In my standard Windows VM setup I just install git via chocolatey, and it only puts git's cmd directory in the path, not the bin directory. This is a git install option I know I've used in the past when installing manually. (The setup where bowerbird runs is much older). cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 12/9/21 02:10, Michael Paquier wrote: > On Wed, Dec 08, 2021 at 10:49:28PM -0800, Andres Freund wrote: >> Could you expand on why it doesn't work? As far as I can tell tar is shipped >> with windows these days, and %SYSTEMROOT%/system32/tar.exe should point to >> that tar? > FWIW, my Windows 10 VM, that I got from Microsoft a couple of years > ago, does not ship it in this location, simply, even after updates. That's odd. What edition of Windows 10 do you have? If it's not Pro I could understand it. I just checked and every Windows 10 machine I have here has it - and some of them are quite old. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 12/13/21 03:08, Michael Paquier wrote: > Also, > the default values of LZ4, TAR and GZIP_PROGRAM had better be set > *before* loading buildenv.pl, and not after it is loaded, no? On > HEAD, one has no way to unset any of those variables so it is not > possible to skip things if an environment has in its PATH a funky > command that TAP would refuse, like a tar command from MinGW or the > git installation. No. The code only sets them if they have not been previously set by buildenv.pl or the calling environment. That's what "||=" means. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 12/13/21 11:47, Andrew Dunstan wrote: > On 12/13/21 03:08, Michael Paquier wrote: >> Also, >> the default values of LZ4, TAR and GZIP_PROGRAM had better be set >> *before* loading buildenv.pl, and not after it is loaded, no? On >> HEAD, one has no way to unset any of those variables so it is not >> possible to skip things if an environment has in its PATH a funky >> command that TAP would refuse, like a tar command from MinGW or the >> git installation. > > No. The code only sets them if they have not been previously set by > buildenv.pl or the calling environment. That's what "||=" means. > > Sorry, I might have misunderstood what you meant here. Yes, you might have a funky tar in your path. But then so you might with a Makefile build, no? I'm not sure why MSVC builds should be different. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 12/14/21 00:17, Michael Paquier wrote: > On Mon, Dec 13, 2021 at 11:47:36AM -0500, Andrew Dunstan wrote: >> No. The code only sets them if they have not been previously set by >> buildenv.pl or the calling environment. That's what "||=" means. > Well, using ||= after the fact means that it is not possible to delete > any of those environment variables either in buildenv.pl, nor is it > possible to set them to empty string values, as vcregress.pl would > just reset them to the default value. And that's the case I am > arguing for here. We don't have any need to drop those default values > at all. What I am arguing for here is the possibility to have > something more flexible than what HEAD proposes. > > So, if we do something like say the attached, then it is possible to > handle environments like mine, while keeping the flexibility you are > looking for to set those defaults. So that would be the best of both > worlds, no?. I also think that we had better document all that > properly, as we do for any other TAP-related variable that can be used > in vcregress.pl. All that leads me to the attached. Ok, if you think it's important then do it. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com