On 24.08.22 17:30, Andres Freund wrote:
>> 258f6dc0a7 Don't hardcode tmp_check/ as test directory for tap tests
>> 8ecc33cf04 Split TESTDIR into TESTLOGDIR and TESTDATADIR
>>
>> I think these patches are split up a bit incorrectly. If you apply
>> the first patch by itself, then the output appears in tab_comp_dir/
>> directly under the source directory. And then the second patch moves
>> it to tmp_check/tap_comp_dir/. If there is an intent to apply these
>> patches separately somehow, this should be cleaned up.
>
> How is that happening with that version of the patch? The test puts
> tap_comp_dir under TESTDIR, and TESTDIR is $(CURDIR)/tmp_check. There was an
> earlier version of the patch that was split one more time that did have that
> problem, but I don't quite see how that version has it?
Ok, I see now how this works. It's a bit weird since the meaning of
TESTDIR is changed. I'm not sure if this could create cross-branch
confusion.
>> 97a0b096e8 meson: prereq: Add src/tools/gen_export.pl
>>
>> This produces leading whitespace in the output files that at least on
>> darwin wasn't there before. See attached patch (0002). This should
>> be checked again on other platforms as well.
>
> Hm, to me the indentation as is makes more sense, but ...
Maybe for the 'gnu' format, but on darwin (and others) it's just a flat
list, so indenting it is pointless.
> I wonder if we should rewrite this in python - I chose perl because I thought
> we could share it, but as you pointed out, that's not possible, because we
> don't want to depend on perl during the autoconf build from a tarball.
Given that the code is already written, I wouldn't do it. Introducing
Python into the toolchain would require considerations of minimum
versions, finding the right binaries, formatting, style checking, etc.,
which would probably be a distraction right now. I also think that this
script, whose purpose is to read an input file line by line and print it
back out slightly differently, is not going to be done better in Python.
