Обсуждение: [HACKERS] PL_stashcache, or, what's our minimum Perl version?
I wanted to do some portability testing on the recently-proposed plperl changes, so I tried to build against a 5.8.0 Perl that I had laying about. It blew up real good: plperl.c: In function `plperl_trusted_init': plperl.c:1017: `PL_stashcache' undeclared (first use in this function) plperl.c:1017: (Each undeclared identifier is reported only once plperl.c:1017: for each function it appears in.) make: *** [plperl.o] Error 1 It's complaining about this: /* invalidate assorted caches */++PL_sub_generation;hv_clear(PL_stashcache); which was introduced by you in commit 1f474d299 (2010-05-13). Some digging suggests that PL_stashcache was added in Perl 5.8.1 circa 2003, although this is impressively underdocumented in any Perl changelog that I could find. So the question is, does anyone care? I wouldn't except that our documentation appears to claim that we work with Perl "5.8 or later". And the lack of field complaints suggests strongly that nobody else cares. So I'm inclined to think we just need to be more specific about the minimum Perl version --- but what exactly? Alternatively, can we make this work with 5.8.0? It looks like PL_stashcache is a macro, so we could make it compile with an "#ifdef PL_stashcache", but I'm pretty nervous about whether that would be breaking needed cleanup behavior. regards, tom lane
I wrote: > So the question is, does anyone care? I wouldn't except that our > documentation appears to claim that we work with Perl "5.8 or later". BTW, what actually says that is installation.sgml: <application>Perl</> 5.8 or later is needed to build from a Git checkout, or if you changed the input files forany of the build steps that use Perl scripts. If building on Windows you will need <application>Perl</> in anycase. <application>Perl</application> is also required to run some test suites. Strictly speaking, there is no statement anywhere (AFAICT) about what Perl versions PL/Perl works with. As an experiment, I built from a "make maintainer-clean" state using that 5.8.0 install, and it worked. So indeed installation.sgml's statement is correct as far as it goes. But I dunno what the situation is for the MSVC build scripts. I did not try the TAP tests either. A look in the buildfarm logs says that the oldest Perl version we're actually testing is 5.8.6 on prairiedog. (castoroides/protosciurus have 5.8.4 but they are not building --with-perl, so that only exercises the build scripts not plperl; they're not doing TAP either.) I only looked into configure.log results though, so I'm not sure about the Windows critters. I kinda suspect we're not actively testing non-MULTIPLICITY builds either. The 5.8.7 test I just ran was with a non-MULTIPLICITY build, so the case doesn't seem actively broken, but I doubt there is any buildfarm coverage. I wonder if it'd be worth getting the buildfarm to log the output of "perl -V" so we could get a clearer picture of what's being tested. regards, tom lane
On 07/27/2017 11:58 PM, Tom Lane wrote: > > I kinda suspect we're not actively testing non-MULTIPLICITY builds > either. The 5.8.7 test I just ran was with a non-MULTIPLICITY build, > so the case doesn't seem actively broken, but I doubt there is any > buildfarm coverage. I wonder if it'd be worth getting the buildfarm > to log the output of "perl -V" so we could get a clearer picture > of what's being tested. > It's quite possible, but in general it will need a new buildfarm client release. If you choose a few possibly critical animalswe can ask the owners to apply a fairly simple patch. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 07/28/2017 08:22 AM, Andrew Dunstan wrote: > > On 07/27/2017 11:58 PM, Tom Lane wrote: >> I kinda suspect we're not actively testing non-MULTIPLICITY builds >> either. The 5.8.7 test I just ran was with a non-MULTIPLICITY build, >> so the case doesn't seem actively broken, but I doubt there is any >> buildfarm coverage. I wonder if it'd be worth getting the buildfarm >> to log the output of "perl -V" so we could get a clearer picture >> of what's being tested. >> > It's quite possible, but in general it will need a new buildfarm client release. If you choose a few possibly criticalanimals we can ask the owners to apply a fairly simple patch. Looks like this, bit it's rather tedious. I think I might back it out. I guess I could also write it to a log file, if we really think we need it. <https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2017-07-28%2018%3A37%3A19> cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> On 07/27/2017 11:58 PM, Tom Lane wrote: >>> I wonder if it'd be worth getting the buildfarm >>> to log the output of "perl -V" so we could get a clearer picture >>> of what's being tested. > Looks like this, bit it's rather tedious. I think I might back it out. I > guess I could also write it to a log file, if we really think we need it. > <https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2017-07-28%2018%3A37%3A19> Yeah, that's awfully verbose :-(. I suspect most vendors wouldn't bother with enumerating quite so many patches. I fixed configure so that it will report what it saw in $Config{ccflags}; that may be sufficient. regards, tom lane
I wrote: > So the question is, does anyone care? I wouldn't except that our > documentation appears to claim that we work with Perl "5.8 or later". > And the lack of field complaints suggests strongly that nobody else > cares. So I'm inclined to think we just need to be more specific > about the minimum Perl version --- but what exactly? I've done some more research on this. It seems to me there are four distinct components to any claim about whether we work with a particular Perl version: 1. Can it run the build-related Perl scripts needed to build PG from a bare git checkout, on non-Windows platforms? 2. Can it run our MSVC build scripts? 3. Does pl/perl compile (and pass its regression tests) against it? 4. Can it run our TAP tests? I have no info to offer about point #2, but I did some tests with ancient 5.8.x Perl versions on my buildfarm hosts prairiedog and gaur. (I would have liked to use something faster, but these Perl versions failed to build at all on more recent platforms, and I thought it rather pointless to try to hack them enough to build.) I find that perl 5.8.0 and later seem to succeed at point #1, but you need at least 5.8.1 to compile plperl. Also, on prairiedog 5.8.1 fails plperl's regression tests because of some seemingly utf8-related bug. That may be an ancient-macOS problem more than anything else, so I didn't poke at it too hard. The really surprising thing I found out is that you can't run the TAP tests on anything older than 5.8.3, because that's when they added "prove". I'm bemused by the idea that such a fundamental facility would get added in a third-digit minor release, but there you have it. (Now, in fairness, this amounted to importing a feature that already existed on CPAN, but still...) 5.8.3 does appear to succeed at points 1,3,4. Now, to get through the TAP tests you need to install IPC::Run, and you also need to update Test::More because the version shipped with 5.8.3 is too old. But at least the failures that you get from lacking these are pretty clear. I am unable to confirm our claim that we work with Test::More 0.82, because CPAN has only versions from a year or three back. (Anyone know of a more, er, comprehensive archive than CPAN?) It's also interesting to speculate about how old a version of IPC::Run is new enough, but I see no easy way to get much data on that either. Anyway, pending some news about compatibility of the MSVC scripts, I think we ought to adjust our docs to state that 5.8.3 is the minimum supported Perl version. Also, I got seriously confused at one point during these tests because, while our configure script carefully sets PERL to an absolute path name, it's content to set PROVE to "prove", paying no attention to whether that version of "prove" matches "perl". Is it really okay to run the TAP tests with a different perl version than we selected for other purposes? I think it'd be a good idea to insist that "prove" be in the same directory we found "perl" in. regards, tom lane
Moin Tom, On Sun, July 30, 2017 1:21 am, Tom Lane wrote: > I wrote: >> So the question is, does anyone care? I wouldn't except that our >> documentation appears to claim that we work with Perl "5.8 or later". >> And the lack of field complaints suggests strongly that nobody else >> cares. So I'm inclined to think we just need to be more specific >> about the minimum Perl version --- but what exactly? My 0.02 cent on Perl versions: Not sure how often People use old Perl versions out in the field. I'd venture this is either happens with "ancient" stuff, e.g. where people just can't or want upgrade. Otherwise, an up-to-date OS is just necessary for security, anyway, and that would contain a Perl from this decade, wouldn't it? If someone is still running Perl 5.8.3, they also got glorius Unicode circa Unicode 4.0 or in this area... > I've done some more research on this. It seems to me there are four > distinct components to any claim about whether we work with a particular > Perl version: > > 1. Can it run the build-related Perl scripts needed to build PG from > a bare git checkout, on non-Windows platforms? > 2. Can it run our MSVC build scripts? > 3. Does pl/perl compile (and pass its regression tests) against it? > 4. Can it run our TAP tests? > > I have no info to offer about point #2, but I did some tests with > ancient 5.8.x Perl versions on my buildfarm hosts prairiedog and > gaur. (I would have liked to use something faster, but these Perl > versions failed to build at all on more recent platforms, and > I thought it rather pointless to try to hack them enough to build.) > > I find that perl 5.8.0 and later seem to succeed at point #1, > but you need at least 5.8.1 to compile plperl. Also, on prairiedog > 5.8.1 fails plperl's regression tests because of some seemingly > utf8-related bug. That may be an ancient-macOS problem more than > anything else, so I didn't poke at it too hard. > > The really surprising thing I found out is that you can't run the > TAP tests on anything older than 5.8.3, because that's when they > added "prove". I'm bemused by the idea that such a fundamental > facility would get added in a third-digit minor release, but there > you have it. (Now, in fairness, this amounted to importing a feature > that already existed on CPAN, but still...) > > 5.8.3 does appear to succeed at points 1,3,4. Now, to get through > the TAP tests you need to install IPC::Run, and you also need to > update Test::More because the version shipped with 5.8.3 is too old. > But at least the failures that you get from lacking these are pretty > clear. > > I am unable to confirm our claim that we work with Test::More 0.82, > because CPAN has only versions from a year or three back. (Anyone > know of a more, er, comprehensive archive than CPAN?) It's also > interesting to speculate about how old a version of IPC::Run is new > enough, but I see no easy way to get much data on that either. > > Anyway, pending some news about compatibility of the MSVC scripts, > I think we ought to adjust our docs to state that 5.8.3 is the > minimum supported Perl version. > > Also, I got seriously confused at one point during these tests because, > while our configure script carefully sets PERL to an absolute path name, > it's content to set PROVE to "prove", paying no attention to whether > that version of "prove" matches "perl". Is it really okay to run the > TAP tests with a different perl version than we selected for other > purposes? I think it'd be a good idea to insist that "prove" be in > the same directory we found "perl" in. Thank you for the analysis. I agree about "prove". As for Test::More: Test::More has been bundled with Perl since 5.6.2 (you can use "corelist" to check for these things), so if all fails, it might be possible to extract a version from a Perl distribution [4]. CPAN authors are encouraged to clean out old versions due to the sheer size of the archive. (Not all got the memo, tho...*cough*) and most mirrors only carry the current files. The original author is Michael G. Schwern [0]. But it seems he cleaned house :) You might have luck with an mirror [1] who didn't clean out, or with archive.org. But with Test::More, it seems a bit confusing, as it is part of Test::Simple [2], which in turn is part of Test2, which is now on github [3]. It's Test-Modules all the way down. I'm not sure you'd find old Test::More versions ready-to-use in this. My apologies if you knew that already. However, I do so happen to have a large archive with Perl releases and CPAN modules. It was first mirrored on mid-2015 - so anything that was deleted before 2015 unfortunately I can't help you with that. But if you need a specific module version, just ping me and I can see if it's in there. Hope this helps, Tels [0]: http://ftp.nluug.nl/languages/perl/CPAN/authors/id/M/MS/MSCHWERN/ [1]: http://mirrors.cpan.org/ [2]: http://search.cpan.org/~exodist/Test-Simple-1.302086/ [3]: https://github.com/Test-More/test-more [4]: http://www.cpan.org/src/5.0/ - tho the timestamps seem to ben have reset on some of the files. 5.8.3 is from 2004 not 2011.
On Sun, Jul 30, 2017 at 01:21:28AM -0400, Tom Lane wrote: > I wrote: > > So the question is, does anyone care? I wouldn't except that our > > documentation appears to claim that we work with Perl "5.8 or later". > > And the lack of field complaints suggests strongly that nobody else > > cares. So I'm inclined to think we just need to be more specific > > about the minimum Perl version --- but what exactly? > > I've done some more research on this. It seems to me there are four > distinct components to any claim about whether we work with a particular > Perl version: > > 1. Can it run the build-related Perl scripts needed to build PG from > a bare git checkout, on non-Windows platforms? > 2. Can it run our MSVC build scripts? > 3. Does pl/perl compile (and pass its regression tests) against it? > 4. Can it run our TAP tests? > 5.8.3 does appear to succeed at points 1,3,4. Now, to get through > the TAP tests you need to install IPC::Run, and you also need to > update Test::More because the version shipped with 5.8.3 is too old. > But at least the failures that you get from lacking these are pretty > clear. > Anyway, pending some news about compatibility of the MSVC scripts, > I think we ought to adjust our docs to state that 5.8.3 is the > minimum supported Perl version. Works for me. I wouldn't wait for testing of the MSVC scripts. Strawberry Perl started with 5.8.8. ActivePerl is far older, but it distributes old versions to subscription holders only. Besides, the main advantage of old-version support is letting folks use a packaged/preinstalled binary, and that doesn't apply on Windows. > Also, I got seriously confused at one point during these tests because, > while our configure script carefully sets PERL to an absolute path name, > it's content to set PROVE to "prove", paying no attention to whether > that version of "prove" matches "perl". Is it really okay to run the > TAP tests with a different perl version than we selected for other > purposes? Typically yes, though one can construct exceptions. > I think it'd be a good idea to insist that "prove" be in > the same directory we found "perl" in. Nah; on my machines, I use /usr/bin/perl and ~/sw/cpan/bin/prove. The latter is built against the former, so there's no particular hazard. nm
Noah Misch <noah@leadboat.com> writes: > On Sun, Jul 30, 2017 at 01:21:28AM -0400, Tom Lane wrote: >> I think it'd be a good idea to insist that "prove" be in >> the same directory we found "perl" in. > Nah; on my machines, I use /usr/bin/perl and ~/sw/cpan/bin/prove. The latter > is built against the former, so there's no particular hazard. Well, OK, but I'd still like to tweak configure so that it records an absolute path for prove rather than just setting PROVE=prove. That way you'd at least be able to tell from the configure log whether you are possibly at risk. regards, tom lane
"Tels" <nospam-pg-abuse@bloodgate.com> writes: > On Sun, July 30, 2017 1:21 am, Tom Lane wrote: >>> So the question is, does anyone care? I wouldn't except that our >>> documentation appears to claim that we work with Perl "5.8 or later". > Not sure how often People use old Perl versions out in the field. I'd > venture this is either happens with "ancient" stuff, e.g. where people > just can't or want upgrade. > Otherwise, an up-to-date OS is just necessary for security, anyway, and > that would contain a Perl from this decade, wouldn't it? Well, that's not really the point, IMO. The reason I'm interested in this is the same reason I run some buildfarm critters on ancient platforms: if we do something that breaks backwards compatibility with old software, we should know it and make a deliberate decision that it's okay. (And update the relevant compatibility claims in our docs.) Moving the compatibility goalposts without knowing it isn't good, especially if it happens in supposedly-stable release branches. >> I am unable to confirm our claim that we work with Test::More 0.82, >> because CPAN has only versions from a year or three back. (Anyone >> know of a more, er, comprehensive archive than CPAN?) It's also >> interesting to speculate about how old a version of IPC::Run is new >> enough, but I see no easy way to get much data on that either. > Test::More has been bundled with Perl since 5.6.2 (you can use "corelist" > to check for these things), so if all fails, it might be possible to > extract a version from a Perl distribution [4]. Yeah, I looked into that. The closest candidate I can find is that perl 5.10.1 contains Test::More 0.92. However, it's not real clear to me exactly which files I'd need to pull out of 5.10.1 and inject into an older tarball --- the layout seems a lot different from a standalone package. regards, tom lane
On Sun, Jul 30, 2017 at 12:05:10PM -0400, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > On Sun, Jul 30, 2017 at 01:21:28AM -0400, Tom Lane wrote: > >> I think it'd be a good idea to insist that "prove" be in > >> the same directory we found "perl" in. > > > Nah; on my machines, I use /usr/bin/perl and ~/sw/cpan/bin/prove. The latter > > is built against the former, so there's no particular hazard. > > Well, OK, but I'd still like to tweak configure so that it records > an absolute path for prove rather than just setting PROVE=prove. > That way you'd at least be able to tell from the configure log > whether you are possibly at risk. That's an improvement.
Noah Misch <noah@leadboat.com> writes: > On Sun, Jul 30, 2017 at 12:05:10PM -0400, Tom Lane wrote: >> Well, OK, but I'd still like to tweak configure so that it records >> an absolute path for prove rather than just setting PROVE=prove. >> That way you'd at least be able to tell from the configure log >> whether you are possibly at risk. > That's an improvement. The reason it does that seems to be that we use AC_CHECK_PROGS rather than AC_PATH_PROGS for locating "prove". I can see no particular consistency to the decisions made in configure.in about which to use: AC_CHECK_PROGS(GCOV, gcov) AC_CHECK_PROGS(LCOV, lcov) AC_CHECK_PROGS(GENHTML, genhtml) AC_CHECK_PROGS(DTRACE, dtrace) AC_CHECK_PROGS(XML2_CONFIG, xml2-config) AC_CHECK_PROGS(DBTOEPUB, dbtoepub) AC_CHECK_PROGS(XMLLINT, xmllint) AC_CHECK_PROGS(XSLTPROC, xsltproc) AC_CHECK_PROGS(OSX, [osx sgml2xml sx]) AC_CHECK_PROGS(FOP, fop) AC_CHECK_PROGS(PROVE, prove) versus AC_PATH_PROG(TAR, tar) PGAC_PATH_BISON PGAC_PATH_FLEX PGAC_PATH_PERL PGAC_PATH_PYTHON AC_PATH_PROG(ZIC, zic) PGAC_PATH_TCLCONFIGSH([$with_tclconfig]) I'm tempted to propose that we switch *all* of these uses of AC_CHECK_PROGS to AC_PATH_PROGS. regards, tom lane
Moin, On Sun, July 30, 2017 12:22 pm, Tom Lane wrote: > "Tels" <nospam-pg-abuse@bloodgate.com> writes: >> On Sun, July 30, 2017 1:21 am, Tom Lane wrote: >>>> So the question is, does anyone care? I wouldn't except that our >>>> documentation appears to claim that we work with Perl "5.8 or later". > >> Not sure how often People use old Perl versions out in the field. I'd >> venture this is either happens with "ancient" stuff, e.g. where people >> just can't or want upgrade. >> Otherwise, an up-to-date OS is just necessary for security, anyway, and >> that would contain a Perl from this decade, wouldn't it? > > Well, that's not really the point, IMO. The reason I'm interested in > this is the same reason I run some buildfarm critters on ancient > platforms: if we do something that breaks backwards compatibility > with old software, we should know it and make a deliberate decision > that it's okay. (And update the relevant compatibility claims in our > docs.) Moving the compatibility goalposts without knowing it isn't > good, especially if it happens in supposedly-stable release branches. Sure, I was just pointing out that moving the goalpost forward knowingly, as you put it, can be ok vs. trying to support ancient software at all costs. Reads to me as we are in agreement here. >>> I am unable to confirm our claim that we work with Test::More 0.82, >>> because CPAN has only versions from a year or three back. (Anyone >>> know of a more, er, comprehensive archive than CPAN?) It's also >>> interesting to speculate about how old a version of IPC::Run is new >>> enough, but I see no easy way to get much data on that either. > >> Test::More has been bundled with Perl since 5.6.2 (you can use >> "corelist" >> to check for these things), so if all fails, it might be possible to >> extract a version from a Perl distribution [4]. > > Yeah, I looked into that. The closest candidate I can find is that > perl 5.10.1 contains Test::More 0.92. However, it's not real clear > to me exactly which files I'd need to pull out of 5.10.1 and inject into > an older tarball --- the layout seems a lot different from a standalone > package. Module distributions contain a MANIFEST like this: http://search.cpan.org/~exodist/Test-Simple/MANIFEST And while reconstructing a module for an old version can be quite a hassle,, it looks like Test::More is just plain Perl and only uses Test::Builder::Module. So basically the two files: http://search.cpan.org/src/EXODIST/Test-Simple-1.302086/lib/Test/More.pm http://search.cpan.org/src/EXODIST/Test-Simple-1.302086/lib/Test/Builder/Module.pm might do the trick. Unless PG uses also Test::Simple or other modules, which I haven't check, then you just need to add these, too. And basically, you put these files into a subdirectory, and use: use lib "mydir"; to tell Perl to load modules from there first. Here is a sample archive with a modern Test::More and an old Test::More from 5.10.1. There are two scripts to see how they are loaded and used. http://bloodgate.com/tmp/test-more-test.tar.gz One thing to watch out is that this would use the old Test::More, but any module it loads (or the script use) comes from the system Perl. So the test might not be 100% complete accurate, f.i. if a newer IO::Scalar is used with the old Test::More. You could also compile and install an old Perl version into a local subdirectory and test with that. That takes a bit more time, though. Hope this helps, Tels
"Tels" <nospam-pg-abuse@bloodgate.com> writes: > On Sun, July 30, 2017 12:22 pm, Tom Lane wrote: >> Yeah, I looked into that. The closest candidate I can find is that >> perl 5.10.1 contains Test::More 0.92. However, it's not real clear >> to me exactly which files I'd need to pull out of 5.10.1 and inject into >> an older tarball --- the layout seems a lot different from a standalone >> package. > So basically the two files: > http://search.cpan.org/src/EXODIST/Test-Simple-1.302086/lib/Test/More.pm > http://search.cpan.org/src/EXODIST/Test-Simple-1.302086/lib/Test/Builder/Module.pm > might do the trick. Thanks for the hint. I transplanted these files out of a 5.10.1 tarball into 5.8.3, then built as usual: lib/Test/Simple.pm lib/Test/More.pm lib/Test/Builder.pm lib/Test/Builder/Module.pm The result seems to work, although it fails a few of 5.8.3's tests, probably because I didn't copy over the relevant test scripts. It's good enough to run PG's tests though. regards, tom lane
Noah Misch <noah@leadboat.com> writes: > On Sun, Jul 30, 2017 at 01:21:28AM -0400, Tom Lane wrote: >> Anyway, pending some news about compatibility of the MSVC scripts, >> I think we ought to adjust our docs to state that 5.8.3 is the >> minimum supported Perl version. > Works for me. Done. I have also reconfigured buildfarm member prairiedog to use a non-MULTIPLICITY build of Perl 5.8.3, with the oldest Test::More and IPC::Run versions I could lay my hands on. Although I'd gotten through a manual "make check-world" with this configuration in HEAD before touching the buildfarm configuration, I see that it just fell over in the back branches. So there's still some more fixing to be done, or else we'll need to change that claim again. Will investigate once the buildfarm run finishes. regards, tom lane
On 7/30/17 12:50, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: >> On Sun, Jul 30, 2017 at 12:05:10PM -0400, Tom Lane wrote: >>> Well, OK, but I'd still like to tweak configure so that it records >>> an absolute path for prove rather than just setting PROVE=prove. >>> That way you'd at least be able to tell from the configure log >>> whether you are possibly at risk. > >> That's an improvement. I disagree with that, unless there is an actual risk. > The reason it does that seems to be that we use AC_CHECK_PROGS > rather than AC_PATH_PROGS for locating "prove". I can see no > particular consistency to the decisions made in configure.in > about which to use: We use the "PATH" variants when we need a fully qualified name. For example, at some point or another, we needed to substitute a fully qualified perl binary name into the headers of scripts. If there is no such requirement, then we should use the non-PATH variants. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > On 7/30/17 12:50, Tom Lane wrote: >> The reason it does that seems to be that we use AC_CHECK_PROGS >> rather than AC_PATH_PROGS for locating "prove". I can see no >> particular consistency to the decisions made in configure.in >> about which to use: > We use the "PATH" variants when we need a fully qualified name. For > example, at some point or another, we needed to substitute a fully > qualified perl binary name into the headers of scripts. > If there is no such requirement, then we should use the non-PATH variants. Why? That risks failures of various sorts, and you have not stated any actual benefit of it. In cases where people do things like sticking non-default Perl builds into nonstandard places, failing to record the absolute path to the program configure saw is both a documentation fail and a clear hazard to build reproducibility. I think that "you can change your PATH and get a different Perl version without reconfiguring" is an anti-feature, because it poses a very high risk of not actually working. regards, tom lane
On 7/31/17 14:55, Tom Lane wrote: >> We use the "PATH" variants when we need a fully qualified name. For >> example, at some point or another, we needed to substitute a fully >> qualified perl binary name into the headers of scripts. > >> If there is no such requirement, then we should use the non-PATH variants. > > Why? That risks failures of various sorts, and you have not stated > any actual benefit of it. What I wrote is merely a description of the current practice. That practice was in turn developed out of ancient Autoconf standard practices. There are arguments to be made for doing it differently. One major PITA with the AC_PATH_* checks is that you can only override them with environment variables that are full paths; otherwise the environment variables are ignored. For example, currently, running ./configure PYTHON=python3 will result in the PYTHON setting being ignored. Currently, this only affects a small number of variables, but if we expanded that, it would be a pretty significant usability change. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 7/31/17 15:17, Peter Eisentraut wrote: > On 7/31/17 14:55, Tom Lane wrote: >>> We use the "PATH" variants when we need a fully qualified name. For >>> example, at some point or another, we needed to substitute a fully >>> qualified perl binary name into the headers of scripts. >> >>> If there is no such requirement, then we should use the non-PATH variants. >> >> Why? That risks failures of various sorts, and you have not stated >> any actual benefit of it. > > What I wrote is merely a description of the current practice. That > practice was in turn developed out of ancient Autoconf standard > practices. There are arguments to be made for doing it differently. > > One major PITA with the AC_PATH_* checks is that you can only override > them with environment variables that are full paths; otherwise the > environment variables are ignored. For example, currently, running > > ./configure PYTHON=python3 > > will result in the PYTHON setting being ignored. Currently, this only > affects a small number of variables, but if we expanded that, it would > be a pretty significant usability change. Plus certain special macros such as AC_PROG_CC don't have a PATH variant, so you're always going to have some inconsistencies. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > One major PITA with the AC_PATH_* checks is that you can only override > them with environment variables that are full paths; otherwise the > environment variables are ignored. For example, currently, running > ./configure PYTHON=python3 > will result in the PYTHON setting being ignored. Really? That seems pretty broken, independently of how many variables are affected. But the ones you'd be most likely to do that with are using AC_PATH_PROG already, I think. Having lesser-used program variables behave inconsistently doesn't seem like much of a win. I'd almost be inclined to say that we should override that behavior of AC_PATH_PROG. It is undocumented AFAICS, and it's not amazingly well thought out, either. regards, tom lane
On 7/31/17 15:38, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> One major PITA with the AC_PATH_* checks is that you can only override >> them with environment variables that are full paths; otherwise the >> environment variables are ignored. For example, currently, running > >> ./configure PYTHON=python3 > >> will result in the PYTHON setting being ignored. > > Really? That seems pretty broken, independently of how many variables > are affected. But the ones you'd be most likely to do that with are > using AC_PATH_PROG already, I think. Having lesser-used program variables > behave inconsistently doesn't seem like much of a win. Well, if we're fiddling around here, I would change them all to AC_CHECK_PROG if possible. Especially the PYTHON one annoys me all the time. CC is another one I set occasionally. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > On 7/31/17 15:38, Tom Lane wrote: >> Really? That seems pretty broken, independently of how many variables >> are affected. But the ones you'd be most likely to do that with are >> using AC_PATH_PROG already, I think. Having lesser-used program variables >> behave inconsistently doesn't seem like much of a win. > Well, if we're fiddling around here, I would change them all to > AC_CHECK_PROG if possible. Especially the PYTHON one annoys me all the > time. CC is another one I set occasionally. I will object really really strongly to that, as it is 180 degrees from where I think we need to go, and will make things a lot worse than before on the documentation aspect that I was concerned about to begin with. If we need to fix things so that AC_PATH_PROG will honor a non-path input value, then let's do that. But let's not make the build system shakier/less reproducible than it is already. I suggest that we could inject logic like this: if VARIABLE-is-set-and-value-isn't-already-absolute; then VARIABLE=`which $VARIABLE 2>/dev/null` fi in front of the existing logic for AC_PATH_PROG(VARIABLE,...). Maybe "which" isn't the best tool for the job, not sure. Another idea, which would probably require replacing _AC_PATH_PROG rather than just putting a wrapper around it, would be to let it perform its normal path walk but using the given word instead of $ac_word. regards, tom lane
I wrote: > Done. I have also reconfigured buildfarm member prairiedog to use > a non-MULTIPLICITY build of Perl 5.8.3, with the oldest Test::More > and IPC::Run versions I could lay my hands on. Although I'd gotten > through a manual "make check-world" with this configuration in HEAD > before touching the buildfarm configuration, I see that it just fell > over in the back branches. So there's still some more fixing to be > done, or else we'll need to change that claim again. Will investigate > once the buildfarm run finishes. The reason it works manually and not in the buildfarm is that the buildfarm injects my $pflags = "PROVE_FLAGS=--timer"; (run_build.pl:1609) and it turns out that 5.8.3's version of prove does not have the --timer switch. I see that --timer is there in the next oldest version I have at hand, 5.8.6. I doubt it is worth teaching the buildfarm scripts to autoconfigure this, but could we do something like my $pflags = "PROVE_FLAGS='" . ($ENV{PROVE_FLAGS} || "--timer") . "'"; to allow overriding this choice from the buildfarm config? FYI, I plan to keep the TAP tests enabled on prairiedog for HEAD, but probably not for the back branches after this run cycle finishes, because it's just too-darn-slow. regards, tom lane
I wrote: > If we need to fix things so that AC_PATH_PROG will honor a non-path > input value, then let's do that. But let's not make the build system > shakier/less reproducible than it is already. > I suggest that we could inject logic like this: > if VARIABLE-is-set-and-value-isn't-already-absolute; then > VARIABLE=`which $VARIABLE 2>/dev/null` > fi > in front of the existing logic for AC_PATH_PROG(VARIABLE,...). > Maybe "which" isn't the best tool for the job, not sure. Concretely, how about something like the attached? BTW, I haven't done it here, but I wonder whether we should not make PGAC_PATH_PROGS invoke AC_ARG_VAR on the target variable, so that configure knows that it should be treated as affecting results caching. regards, tom lane diff --git a/config/docbook.m4 b/config/docbook.m4 index c485eaf..f9307f3 100644 *** a/config/docbook.m4 --- b/config/docbook.m4 *************** *** 3,9 **** # PGAC_PROG_NSGMLS # ---------------- AC_DEFUN([PGAC_PROG_NSGMLS], ! [AC_PATH_PROGS([NSGMLS], [onsgmls nsgmls])]) # PGAC_CHECK_DOCBOOK(VERSION) --- 3,9 ---- # PGAC_PROG_NSGMLS # ---------------- AC_DEFUN([PGAC_PROG_NSGMLS], ! [PGAC_PATH_PROGS(NSGMLS, [onsgmls nsgmls])]) # PGAC_CHECK_DOCBOOK(VERSION) diff --git a/config/perl.m4 b/config/perl.m4 index 9706c4d..e44ca94 100644 *** a/config/perl.m4 --- b/config/perl.m4 *************** *** 4,13 **** # PGAC_PATH_PERL # -------------- AC_DEFUN([PGAC_PATH_PERL], ! [# Let the user override the search ! if test -z "$PERL"; then ! AC_PATH_PROG(PERL, perl) ! fi if test "$PERL"; then pgac_perl_version=`$PERL -v 2>/dev/null | sed -n ['s/This is perl.*v[a-z ]*\([0-9]\.[0-9][0-9.]*\).*$/\1/p']` --- 4,10 ---- # PGAC_PATH_PERL # -------------- AC_DEFUN([PGAC_PATH_PERL], ! [PGAC_PATH_PROGS(PERL, perl) if test "$PERL"; then pgac_perl_version=`$PERL -v 2>/dev/null | sed -n ['s/This is perl.*v[a-z ]*\([0-9]\.[0-9][0-9.]*\).*$/\1/p']` diff --git a/config/programs.m4 b/config/programs.m4 index b7deb86..0b0098e 100644 *** a/config/programs.m4 --- b/config/programs.m4 *************** *** 1,6 **** --- 1,23 ---- # config/programs.m4 + # PGAC_PATH_PROGS + # --------------- + # This wrapper for AC_PATH_PROGS behaves like that macro except in the case + # where VARIABLE is already set but is not set to an absolute path. We will + # attempt to make it into an absolute path using "which". If that fails, + # we ignore the existing value, which is the behavior of AC_PATH_PROGS. + + AC_DEFUN([PGAC_PATH_PROGS], + [# If user is trying to override the search for $1, make sure that + # the variable's value is an absolute path. + if test -n "$$1"; then + $1=`which "$$1" 2>/dev/null` + fi + AC_PATH_PROGS($@)dnl + ]) + + # PGAC_PATH_BISON # --------------- # Look for Bison, set the output variable BISON to its path if found. *************** *** 8,17 **** # Note we do not accept other implementations of yacc. AC_DEFUN([PGAC_PATH_BISON], ! [# Let the user override the search ! if test -z "$BISON"; then ! AC_PATH_PROGS(BISON, bison) ! fi if test "$BISON"; then pgac_bison_version=`$BISON --version 2>/dev/null | sed q` --- 25,31 ---- # Note we do not accept other implementations of yacc. AC_DEFUN([PGAC_PATH_BISON], ! [PGAC_PATH_PROGS(BISON, bison) if test "$BISON"; then pgac_bison_version=`$BISON --version 2>/dev/null | sed q` *************** if test -z "$BISON"; then *** 41,47 **** *** PostgreSQL then you do not need to worry about this, because the Bison *** output is pre-generated.)]) fi ! # We don't need AC_SUBST(BISON) because AC_PATH_PROG did it AC_SUBST(BISONFLAGS) ])# PGAC_PATH_BISON --- 55,61 ---- *** PostgreSQL then you do not need to worry about this, because the Bison *** output is pre-generated.)]) fi ! # We don't need AC_SUBST(BISON) because PGAC_PATH_PROGS did it AC_SUBST(BISONFLAGS) ])# PGAC_PATH_BISON *************** AC_DEFUN([PGAC_CHECK_GETTEXT], *** 229,235 **** [AC_MSG_ERROR([a gettext implementation is required for NLS])]) AC_CHECK_HEADER([libintl.h], [], [AC_MSG_ERROR([header file <libintl.h> is required for NLS])]) ! AC_PATH_PROGS(MSGFMT, msgfmt) if test -z "$MSGFMT"; then AC_MSG_ERROR([msgfmt is required for NLS]) fi --- 243,249 ---- [AC_MSG_ERROR([a gettext implementation is required for NLS])]) AC_CHECK_HEADER([libintl.h], [], [AC_MSG_ERROR([header file <libintl.h> is required for NLS])]) ! PGAC_PATH_PROGS(MSGFMT, msgfmt) if test -z "$MSGFMT"; then AC_MSG_ERROR([msgfmt is required for NLS]) fi *************** AC_DEFUN([PGAC_CHECK_GETTEXT], *** 238,245 **** pgac_cv_msgfmt_flags=-c fi]) AC_SUBST(MSGFMT_FLAGS, $pgac_cv_msgfmt_flags) ! AC_PATH_PROGS(MSGMERGE, msgmerge) ! AC_PATH_PROGS(XGETTEXT, xgettext) ])# PGAC_CHECK_GETTEXT --- 252,259 ---- pgac_cv_msgfmt_flags=-c fi]) AC_SUBST(MSGFMT_FLAGS, $pgac_cv_msgfmt_flags) ! PGAC_PATH_PROGS(MSGMERGE, msgmerge) ! PGAC_PATH_PROGS(XGETTEXT, xgettext) ])# PGAC_CHECK_GETTEXT diff --git a/config/python.m4 b/config/python.m4 index 953d709..f3c7642 100644 *** a/config/python.m4 --- b/config/python.m4 *************** *** 6,15 **** # PGAC_PATH_PYTHON # ---------------- ! # Look for Python and set the output variable 'PYTHON' ! # to 'python' if found, empty otherwise. AC_DEFUN([PGAC_PATH_PYTHON], ! [AC_PATH_PROG(PYTHON, python) if test x"$PYTHON" = x""; then AC_MSG_ERROR([Python not found]) fi --- 6,15 ---- # PGAC_PATH_PYTHON # ---------------- ! # Look for Python and set the output variable 'PYTHON' if found, ! # fail otherwise. AC_DEFUN([PGAC_PATH_PYTHON], ! [PGAC_PATH_PROGS(PYTHON, python) if test x"$PYTHON" = x""; then AC_MSG_ERROR([Python not found]) fi diff --git a/config/tcl.m4 b/config/tcl.m4 index 907deb9..a4bf231 100644 *** a/config/tcl.m4 --- b/config/tcl.m4 *************** *** 4,10 **** AC_DEFUN([PGAC_PATH_TCLSH], ! [AC_PATH_PROGS(TCLSH, [tclsh tcl tclsh8.6 tclsh86 tclsh8.5 tclsh85 tclsh8.4 tclsh84]) if test x"$TCLSH" = x""; then AC_MSG_ERROR([Tcl shell not found]) fi --- 4,10 ---- AC_DEFUN([PGAC_PATH_TCLSH], ! [PGAC_PATH_PROGS(TCLSH, [tclsh tcl tclsh8.6 tclsh86 tclsh8.5 tclsh85 tclsh8.4 tclsh84]) if test x"$TCLSH" = x""; then AC_MSG_ERROR([Tcl shell not found]) fi diff --git a/configure.in b/configure.in index abfc7b5..a0f0f85 100644 *** a/configure.in --- b/configure.in *************** PGAC_ARG_BOOL(enable, profiling, no, *** 218,232 **** # PGAC_ARG_BOOL(enable, coverage, no, [build with coverage testing instrumentation], ! [AC_PATH_PROGS(GCOV, gcov) if test -z "$GCOV"; then AC_MSG_ERROR([gcov not found]) fi ! AC_PATH_PROGS(LCOV, lcov) if test -z "$LCOV"; then AC_MSG_ERROR([lcov not found]) fi ! AC_PATH_PROGS(GENHTML, genhtml) if test -z "$GENHTML"; then AC_MSG_ERROR([genhtml not found]) fi]) --- 218,232 ---- # PGAC_ARG_BOOL(enable, coverage, no, [build with coverage testing instrumentation], ! [PGAC_PATH_PROGS(GCOV, gcov) if test -z "$GCOV"; then AC_MSG_ERROR([gcov not found]) fi ! PGAC_PATH_PROGS(LCOV, lcov) if test -z "$LCOV"; then AC_MSG_ERROR([lcov not found]) fi ! PGAC_PATH_PROGS(GENHTML, genhtml) if test -z "$GENHTML"; then AC_MSG_ERROR([genhtml not found]) fi]) *************** AC_SUBST(enable_coverage) *** 237,243 **** # PGAC_ARG_BOOL(enable, dtrace, no, [build with DTrace support], ! [AC_PATH_PROGS(DTRACE, dtrace) if test -z "$DTRACE"; then AC_MSG_ERROR([dtrace not found]) fi --- 237,243 ---- # PGAC_ARG_BOOL(enable, dtrace, no, [build with DTrace support], ! [PGAC_PATH_PROGS(DTRACE, dtrace) if test -z "$DTRACE"; then AC_MSG_ERROR([dtrace not found]) fi *************** PGAC_ARG_BOOL(with, libxml, no, [build w *** 816,822 **** [AC_DEFINE([USE_LIBXML], 1, [Define to 1 to build with XML support. (--with-libxml)])]) if test "$with_libxml" = yes ; then ! AC_PATH_PROGS(XML2_CONFIG, xml2-config) if test -n "$XML2_CONFIG"; then for pgac_option in `$XML2_CONFIG --cflags`; do case $pgac_option in --- 816,822 ---- [AC_DEFINE([USE_LIBXML], 1, [Define to 1 to build with XML support. (--with-libxml)])]) if test "$with_libxml" = yes ; then ! PGAC_PATH_PROGS(XML2_CONFIG, xml2-config) if test -n "$XML2_CONFIG"; then for pgac_option in `$XML2_CONFIG --cflags`; do case $pgac_option in *************** case $INSTALL in *** 912,918 **** esac AC_SUBST(install_bin) ! AC_PATH_PROG(TAR, tar) AC_PROG_LN_S AC_PROG_AWK AC_PROG_MKDIR_P --- 912,918 ---- esac AC_SUBST(install_bin) ! PGAC_PATH_PROGS(TAR, tar) AC_PROG_LN_S AC_PROG_AWK AC_PROG_MKDIR_P *************** if test "$with_python" = yes; then *** 948,954 **** fi if test "$cross_compiling" = yes && test -z "$with_system_tzdata"; then ! AC_PATH_PROG(ZIC, zic) if test -z "$ZIC"; then AC_MSG_ERROR([ When cross-compiling, either use the option --with-system-tzdata to use --- 948,954 ---- fi if test "$cross_compiling" = yes && test -z "$with_system_tzdata"; then ! PGAC_PATH_PROGS(ZIC, zic) if test -z "$ZIC"; then AC_MSG_ERROR([ When cross-compiling, either use the option --with-system-tzdata to use *************** fi *** 2119,2135 **** # PGAC_PROG_NSGMLS PGAC_CHECK_DOCBOOK(4.2) ! AC_PATH_PROGS(DBTOEPUB, dbtoepub) ! AC_PATH_PROGS(XMLLINT, xmllint) ! AC_PATH_PROGS(XSLTPROC, xsltproc) ! AC_PATH_PROGS(OSX, [osx sgml2xml sx]) ! AC_PATH_PROGS(FOP, fop) # # Check for test tools # if test "$enable_tap_tests" = yes; then ! AC_PATH_PROGS(PROVE, prove) if test -z "$PROVE"; then AC_MSG_ERROR([prove not found]) fi --- 2119,2135 ---- # PGAC_PROG_NSGMLS PGAC_CHECK_DOCBOOK(4.2) ! PGAC_PATH_PROGS(DBTOEPUB, dbtoepub) ! PGAC_PATH_PROGS(XMLLINT, xmllint) ! PGAC_PATH_PROGS(XSLTPROC, xsltproc) ! PGAC_PATH_PROGS(OSX, [osx sgml2xml sx]) ! PGAC_PATH_PROGS(FOP, fop) # # Check for test tools # if test "$enable_tap_tests" = yes; then ! PGAC_PATH_PROGS(PROVE, prove) if test -z "$PROVE"; then AC_MSG_ERROR([prove not found]) fi -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 7/31/17 16:54, Tom Lane wrote: > Maybe "which" isn't the best tool for the job, not sure. Yeah, "which" is not portable. This would need a bit more work and portability testing. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > On 7/31/17 16:54, Tom Lane wrote: >> Maybe "which" isn't the best tool for the job, not sure. > Yeah, "which" is not portable. This would need a bit more work and > portability testing. Fair enough. This late in beta is probably not the time to be adding new portability testing needs. However, I noticed that some places --- not consistently everywhere --- were solving this with the low-tech method of just skipping AC_PATH_PROG if the variable is already set. We could apply that hack more consistently by inventing a PGAC_PATH_PROGS wrapper macro as I previously sketched, but for now just defining it as # Let the user override the search for $1 if test -z "$$1"; then AC_PATH_PROGS($@) fi (untested, but you get the idea). In the long run I would like to improve this to force the supplied value into absolute-path form, but I'd be content to ship v10 like that. regards, tom lane
On Sun, July 30, 2017 4:35 pm, Tom Lane wrote: > "Tels" <nospam-pg-abuse@bloodgate.com> writes: >> On Sun, July 30, 2017 12:22 pm, Tom Lane wrote: >>> Yeah, I looked into that. The closest candidate I can find is that >>> perl 5.10.1 contains Test::More 0.92. However, it's not real clear >>> to me exactly which files I'd need to pull out of 5.10.1 and inject >>> into >>> an older tarball --- the layout seems a lot different from a standalone >>> package. > >> So basically the two files: >> http://search.cpan.org/src/EXODIST/Test-Simple-1.302086/lib/Test/More.pm >> http://search.cpan.org/src/EXODIST/Test-Simple-1.302086/lib/Test/Builder/Module.pm >> might do the trick. > > Thanks for the hint. I transplanted these files out of a 5.10.1 > tarball into 5.8.3, then built as usual: > lib/Test/Simple.pm > lib/Test/More.pm > lib/Test/Builder.pm > lib/Test/Builder/Module.pm > The result seems to work, although it fails a few of 5.8.3's tests, > probably because I didn't copy over the relevant test scripts. > It's good enough to run PG's tests though. Ah, cool. One more question: > 1. Can it run the build-related Perl scripts needed to build PG from > a bare git checkout, on non-Windows platforms? > 2. Can it run our MSVC build scripts? > 3. Does pl/perl compile (and pass its regression tests) against it? > 4. Can it run our TAP tests? > 5.8.3 does appear to succeed at points 1,3,4. Now, to get through > the TAP tests you need to install IPC::Run, and you also need to > update Test::More because the version shipped with 5.8.3 is too old. > But at least the failures that you get from lacking these are pretty > clear. So, for point 2, one would need Perl 5.8.3 + a newer Test::More and IPC::Run, or a newer Perl. I've toyed around with cpanminus: apt-get install cpanminuscpanm Test::Morecpanm IPC::Run and it successfully build (in a local path because I was not root) the latest Test::More and IPC::Run. I'm not sure if this would be enough to run the MSVC build scripts, tho. So, is the goal you are trying to achive here to be able to say "You need Perl 5.8.3; plus Module XYZ in vABC if you want point 2, otherwise skip this step" instead of saying "You need Perl 5.10.1?"? In that case it might be also possible to write instructions on how to obtain and use these modules, or how to just use a newer Perl in a temp. path. Best wishes, Tels
"Tels" <nospam-pg-abuse@bloodgate.com> writes: > So, is the goal you are trying to achive here to be able to say "You need > Perl 5.8.3; plus Module XYZ in vABC if you want point 2, otherwise skip > this step" instead of saying "You need Perl 5.10.1?"? I mainly want to be sure that if we say "it runs on 5.8.3", that's not a lie. The fact that some test scripts need a newer version of Test::More seems like a detail that can be left out. In the (quite improbable) case that someone runs into that situation in the field, the error message that they'll get is clear enough, and they should be able to figure out how to fix it without help from our docs. Anyone who's running a 15-year-old Perl installation has probably had to upgrade some of the modules before. But in reality, only developers are ever going to use --enable-tap-tests in the first place, so it's largely moot. What I was really annoyed by was that PL/Perl failed to build and/or pass regression tests on allegedly supported Perl versions, and that's sorted now. regards, tom lane
On 07/31/2017 06:54 PM, Tom Lane wrote: > but could we do something like > my $pflags = "PROVE_FLAGS='" . ($ENV{PROVE_FLAGS} || "--timer") . "'"; > > to allow overriding this choice from the buildfarm config? > > I have committed this in a slightly different form. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes: > On 07/31/2017 06:54 PM, Tom Lane wrote: >> but could we do something like >> my $pflags = "PROVE_FLAGS='" . ($ENV{PROVE_FLAGS} || "--timer") . "'"; >> to allow overriding this choice from the buildfarm config? > I have committed this in a slightly different form. Thanks. regards, tom lane