Обсуждение: Remove MSVC scripts from the tree
Hi all, It has been mentioned a few times now that, as Meson has been integrated in the tree, the next step would be to get rid of the custom scripts in src/tools/msvc/ and moving forward only support Meson when building with VS compilers. As far as I understand, nobody has sent a patch to do that yet, so here is one. Please find attached a patch to move the needle in this sense. Here are some notes: - Meson depends on msvc/gendef.pl, so I've renamed it to msvc_gendef.pl in src/tools/. - install-windows.sgml could be OK if entirely gone, moving more detailed instructions to the meson page instead. - What to do with src/tools/msvc/dummylib/? It would still be useful for src/tools/win32tzlist.pl but it also seems to me that we should be able to live without it as perl's Win32 should have evolved quite a bit? I need to test this, but I'd like to think that we are OK with a removal of it. If people want to keep it. I'm OK with that as well. The documentation for Meson could be improved more, I think, when building with either MinGW's or VS compilers. Particularly, something that was itching me is how we can improve the instructions about the extra packages one would need to deploy to make the builds work as a straight removal of install-windows.sgml loses references, but we are OK if we rely on a packager to do the dependency job. For example, I've been having a good ride with Strawberry Perl and Chocolatey, linking my build of Meson with MinGW or Visual, but I also recall Andres being allergic to Strawberry, so I am not sure if folks are OK with directly mentioning it in the docs, for instance. One thing that we could do is add a subsection in the installation notes close to MinGW, but for Visual Studio that applies to Meson. One good thing with the removal of install-windows.sgml is that it is possible to clean up a few things that have rotted in the docs, and could be reworked from scratch. For example, we still recommend Active Perl, but it has become impossible to use in builds as perl commands cannot be involved without extra steps that we don't recommend, while an installation needs to be registered in their centralized system.. That's not user-friendly. I've notice that Andrew has been relying on strawberry perl as well with Chocolatey for some buildfarm members. As of today, I can see that the only buildfarm members relying on these scripts are bowerbird and hamerkop, so these two would fail if the patch attached were to be applied today. I am adding Andrew D. and the maintainers of hamerkop (SRA-OSS) in CC for comments. Thoughts? -- Michael
Вложения
On 22.09.23 03:12, Michael Paquier wrote: > It has been mentioned a few times now that, as Meson has been > integrated in the tree, the next step would be to get rid of the > custom scripts in src/tools/msvc/ and moving forward only support > Meson when building with VS compilers. First we need to fix things so that we can build using meson from a distribution tarball, which is the subject of <https://commitfest.postgresql.org/44/4357/>.
On Fri, Sep 22, 2023 at 08:06:57AM +0200, Peter Eisentraut wrote: > First we need to fix things so that we can build using meson from a > distribution tarball, which is the subject of > <https://commitfest.postgresql.org/44/4357/>. Thanks, missed this one. -- Michael
Вложения
On 2023-09-21 Th 21:12, Michael Paquier wrote:
As of today, I can see that the only buildfarm members relying on these scripts are bowerbird and hamerkop, so these two would fail if the patch attached were to be applied today. I am adding Andrew D. and the maintainers of hamerkop (SRA-OSS) in CC for comments.
Changing bowerbird to use meson should not be difficult, just needs some TUITs.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On Fri, 22 Sep 2023 10:12:29 +0900 Michael Paquier <michael@paquier.xyz> wrote: > As of today, I can see that the only buildfarm members relying on > these scripts are bowerbird and hamerkop, so these two would fail if > the patch attached were to be applied today. I am adding Andrew > D. and the maintainers of hamerkop (SRA-OSS) in CC for comments. hamerkop is not yet prepared for Meson builds, but we plan to work on this support soon. If we go with Meson builds exclusively right now, we will have to temporarily remove the master/HEAD for a while. Best Regards. -- SRA OSS LLC Chen Ningwei<chen@sraoss.co.jp>
On 2023-09-26 Tu 01:25, NINGWEI CHEN wrote:
On Fri, 22 Sep 2023 10:12:29 +0900 Michael Paquier <michael@paquier.xyz> wrote:As of today, I can see that the only buildfarm members relying on these scripts are bowerbird and hamerkop, so these two would fail if the patch attached were to be applied today. I am adding Andrew D. and the maintainers of hamerkop (SRA-OSS) in CC for comments.hamerkop is not yet prepared for Meson builds, but we plan to work on this support soon. If we go with Meson builds exclusively right now, we will have to temporarily remove the master/HEAD for a while. Best Regards.
I don't think we should switch to that until you're ready.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On 22.09.23 03:12, Michael Paquier wrote: > It has been mentioned a few times now that, as Meson has been > integrated in the tree, the next step would be to get rid of the > custom scripts in src/tools/msvc/ and moving forward only support > Meson when building with VS compilers. As far as I understand, nobody > has sent a patch to do that yet, so here is one. > > Please find attached a patch to move the needle in this sense. Your patch still leaves various mentions of Mkvcbuild.pm and Project.pm in other files, including in config/perl.m4 meson.build src/bin/pg_basebackup/Makefile src/bin/pgevent/meson.build src/common/Makefile src/common/meson.build src/interfaces/libpq/Makefile src/port/Makefile A few of these comments are like "see $otherfile for the reason", which means that if we delete $otherfile, we should move that information to a new site somehow.
On Fri, Sep 29, 2023 at 11:26:55AM +0200, Peter Eisentraut wrote: > Your patch still leaves various mentions of Mkvcbuild.pm and Project.pm in > other files, including in Indeed, thanks. I didn't think to check for references to these modules. > config/perl.m4 Here is the thing: # switches for symbols not beginning with underscore. Some exceptions are the # Windows-specific -D_USE_32BIT_TIME_T and -D__MINGW_USE_VC2005_COMPAT; see # Mkvcbuild.pm for details. And we'd lose quite some information here. meson.build loops back to perl.m4 for this part. Reformating the comments perl.m4 should do the job. > meson.build These were in Project::_new() and WriteItemDefinitionGroup(). Just removing the reference does not remove any information. > src/bin/pg_basebackup/Makefile -# If you add or remove files here, also update Mkvcbuild.pm, which only knows -# about OBJS, not BBOBJS, and thus has to be manually updated to stay in sync -# with this list. This can be removed, I guess. > src/bin/pgevent/meson.build The reference can be removed. The original says nothing about the use of DisableLinkerWarnings() in this case. > src/common/Makefile > src/common/meson.build These two have the same copy-pasted comment, and the reference can be removed. > src/interfaces/libpq/Makefile Can be removed once the MSVC files are gone. > src/port/Makefile Can be removed. Attached is a v2 with these adjustments, for now. -- Michael
Вложения
On 02.10.23 09:38, Michael Paquier wrote:
> Attached is a v2 with these adjustments, for now.
General comments:
- I think we can't just delete install-windows.sgml. Some of that
content needs to be moved over to installation.sgml. As a simple
example, install-windows.sgml shows which MSVC versions are supported.
That information should surely be kept.
- Is src/backend/utils/README.Gen_dummy_probes still correct after this?
AFAICT, the Perl-based MSVC build system uses Gen_dummy_probes.pl, but
the meson build uses Gen_dummy_probes.sed even on Windows. Is that
correct, intended?
- src/port/pgstrsignal.c contains a comment that it is not "built in
MSVC builds", but AFAICT, that is only correct for the legacy Perl-based
build system, not for meson. Again, is that correct, intended?
Detail comments:
(Btw., whatever orderfile you used for the diff, I find that confusing.)
* config/perl.m4: This now contains all the required information, but
maybe break the text into paragraphs a bit?
* doc/src/sgml/installation.sgml:
I think this paragraph should just be removed altogether:
<para>
If you are building <productname>PostgreSQL</productname> for Microsoft
- Windows, read this chapter if you intend to build with MinGW or Cygwin;
- but if you intend to build with Microsoft's <productname>Visual
- C++</productname>, see <xref linkend="install-windows"/> instead.
+ Windows, read this chapter if you intend to build with Meson, MinGW or
+ Cygwin.
</para>
Here
<para>
PostgreSQL can be built using Cygwin, a Linux-like environment for
Windows, but that method is inferior to the native Windows build
- <phrase condition="standalone-ignore">(see <xref
linkend="install-windows"/>)</phrase> and
- running a server under Cygwin is no longer recommended.
+ with Meson, and running a server under Cygwin is no longer recommended.
</para>
I think "with Meson" should be removed. The tradeoff is Cygwin vs.
native, it doesn't have anything to do with Meson.
Also, I think this paragraph needs a complete revision, along with
however install-windows.sgml gets integrated:
<para>
- PostgreSQL for Windows can be built using MinGW, a Unix-like build
[...]
* meson.build: I think these comments are unnecessary and can be removed:
-# From Project.pm
+# MSVC flags
+ # Preprocessor definitions.
* src/bin/pgevent/meson.build: After consideration, I think this
comment should just be removed:
-# FIXME: copied from Mkvcbuild.pm, but I don't think that's the right
approach
+# FIXME: this may not not the right approach..
The original site in Mkvcbuild.pm does not contain a comment, so we
should accept that as canonical. It doesn't help much if we carry
around a comment like "this might be wrong" indefinitely without any
further supporting material.
* src/common/Makefile and src/common/meson.build: This change is losing
the period at the end of the first sentence:
# A few files are currently only built for frontend, not server
-# (Mkvcbuild.pm has a copy of this list, too). logging.c is excluded
-# from OBJS_FRONTEND_SHLIB (shared library) as a matter of policy,
-# because it is not appropriate for general purpose libraries such
-# as libpq to report errors directly.
+# logging.c is excluded from OBJS_FRONTEND_SHLIB (shared library) as
+# a matter of policy, because it is not appropriate for general purpose
+# libraries such as libpq to report errors directly.
On Thu, Oct 05, 2023 at 09:38:51AM +0200, Peter Eisentraut wrote: > - I think we can't just delete install-windows.sgml. Some of that content > needs to be moved over to installation.sgml. As a simple example, > install-windows.sgml shows which MSVC versions are supported. That > information should surely be kept. I've been thinking about the whole structure for a bit, but with the MSVC scripts gone and the fact that we would rely on meson, moving this information to the section under the platform-specific notes is feeling kind of natural here. Here is a possible split of the information across several sections: - The requirements: -- ActiveState Perl could be entirely removed, IMO. Perhaps we should replace that to a reference to raspberry-perl, chocolatey or similar? I am not sure about the best approach here, so for now I've kept the bits about active perl. -- bison and flex, which would become hard requirements on Windows with Visual Studio now. Perhaps this could be unified with the patch for distprep later on, but here we have specifics for Windows. -- All the other optional requirements, tcl, etc. - MinGW notes. - Visual Studio notes, with the versions of visual supported, download links, and a bit more. - Notes specific about 64b builds. The attached is a bit crude and requires adjustments, but it shows the idea. > - Is src/backend/utils/README.Gen_dummy_probes still correct after this? > AFAICT, the Perl-based MSVC build system uses Gen_dummy_probes.pl, but the > meson build uses Gen_dummy_probes.sed even on Windows. Is that correct, > intended? Interesting point. This may depend on the environment at the end? As far as I can see, sed is currently a hard requirement in the meson build and we'd fail if the command cannot be used. The buildfarm machines that test meson are able to find sed, making Gen_dummy_probes.pl not necessary: https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=drongo&dt=2023-10-11%2020%3A21%3A17&stg=configure So the $1000 question is: could there be a point in keeping the perl script around if sed cannot be found? The buildfarm coverage is currently saying no thanks to chocolatey, at least. The VM images compiled by Andres for the CI seem to have the same opinion. > - src/port/pgstrsignal.c contains a comment that it is not "built in MSVC > builds", but AFAICT, that is only correct for the legacy Perl-based build > system, not for meson. Again, is that correct, intended? Indeed, it's built under meson for WIN32. Good find. > Detail comments: > > (Btw., whatever orderfile you used for the diff, I find that confusing.) Here is my configuration for that: https://github.com/michaelpq/home/blob/main/.gitconfig_orderfile > * config/perl.m4: This now contains all the required information, but maybe > break the text into paragraphs a bit? Sure. I've attempted something here. > * doc/src/sgml/installation.sgml: > > I think this paragraph should just be removed altogether: > > <para> > If you are building <productname>PostgreSQL</productname> for Microsoft > - Windows, read this chapter if you intend to build with MinGW or Cygwin; > - but if you intend to build with Microsoft's <productname>Visual > - C++</productname>, see <xref linkend="install-windows"/> instead. > + Windows, read this chapter if you intend to build with Meson, MinGW or > + Cygwin. > </para> Okay. > Here > > <para> > PostgreSQL can be built using Cygwin, a Linux-like environment for > Windows, but that method is inferior to the native Windows build > - <phrase condition="standalone-ignore">(see <xref > linkend="install-windows"/>)</phrase> and > - running a server under Cygwin is no longer recommended. > + with Meson, and running a server under Cygwin is no longer recommended. > </para> > > I think "with Meson" should be removed. The tradeoff is Cygwin vs. native, > it doesn't have anything to do with Meson. Okay. > Also, I think this paragraph needs a complete revision, along with however > install-windows.sgml gets integrated: > > <para> > - PostgreSQL for Windows can be built using MinGW, a Unix-like build > [...] Sure, see above for details. > * meson.build: I think these comments are unnecessary and can be removed: > > -# From Project.pm > +# MSVC flags > > + # Preprocessor definitions. Okay. > * src/bin/pgevent/meson.build: After consideration, I think this comment > should just be removed: > > -# FIXME: copied from Mkvcbuild.pm, but I don't think that's the right > approach > +# FIXME: this may not not the right approach.. > > The original site in Mkvcbuild.pm does not contain a comment, so we should > accept that as canonical. It doesn't help much if we carry around a comment > like "this might be wrong" indefinitely without any further supporting > material. Hmm, okay. I was not sure about this one but fine for me to drop it. > * src/common/Makefile and src/common/meson.build: This change is losing the > period at the end of the first sentence: > > # A few files are currently only built for frontend, not server > -# (Mkvcbuild.pm has a copy of this list, too). logging.c is excluded > -# from OBJS_FRONTEND_SHLIB (shared library) as a matter of policy, > -# because it is not appropriate for general purpose libraries such > -# as libpq to report errors directly. > +# logging.c is excluded from OBJS_FRONTEND_SHLIB (shared library) as > +# a matter of policy, because it is not appropriate for general purpose > +# libraries such as libpq to report errors directly. Fixed. -- Michael
Вложения
On 12.10.23 07:23, Michael Paquier wrote: >> - Is src/backend/utils/README.Gen_dummy_probes still correct after this? >> AFAICT, the Perl-based MSVC build system uses Gen_dummy_probes.pl, but the >> meson build uses Gen_dummy_probes.sed even on Windows. Is that correct, >> intended? > Interesting point. This may depend on the environment at the end? As > far as I can see, sed is currently a hard requirement in the meson > build and we'd fail if the command cannot be used. The buildfarm > machines that test meson are able to find sed, making > Gen_dummy_probes.pl not necessary: > https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=drongo&dt=2023-10-11%2020%3A21%3A17&stg=configure > > So the $1000 question is: could there be a point in keeping the perl > script around if sed cannot be found? The buildfarm coverage is > currently saying no thanks to chocolatey, at least. The VM images > compiled by Andres for the CI seem to have the same opinion. I don't think we should rely on sed being there on Windows. Maybe it's true now on the handful of buildfarm/CI machines and early adopters, but do we have any indication that that is systematic or just an accident? Since we definitely require Perl now, we could just as well use the Perl script and avoid this issue. Attached is a Perl version of the sed script, converted by hand (so not the super-verbose s2p thing). It's basically just the sed script with semicolons added and the backslashes in the regular expressions moved around. I think we could use something like that for all platforms now.
Вложения
On 2023-11-08 We 03:41, Peter Eisentraut wrote: > On 12.10.23 07:23, Michael Paquier wrote: >>> - Is src/backend/utils/README.Gen_dummy_probes still correct after >>> this? >>> AFAICT, the Perl-based MSVC build system uses Gen_dummy_probes.pl, >>> but the >>> meson build uses Gen_dummy_probes.sed even on Windows. Is that >>> correct, >>> intended? >> Interesting point. This may depend on the environment at the end? As >> far as I can see, sed is currently a hard requirement in the meson >> build and we'd fail if the command cannot be used. The buildfarm >> machines that test meson are able to find sed, making >> Gen_dummy_probes.pl not necessary: >> https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=drongo&dt=2023-10-11%2020%3A21%3A17&stg=configure >> >> >> So the $1000 question is: could there be a point in keeping the perl >> script around if sed cannot be found? The buildfarm coverage is >> currently saying no thanks to chocolatey, at least. The VM images >> compiled by Andres for the CI seem to have the same opinion. > > I don't think we should rely on sed being there on Windows. Maybe > it's true now on the handful of buildfarm/CI machines and early > adopters, but do we have any indication that that is systematic or > just an accident? > > Since we definitely require Perl now, we could just as well use the > Perl script and avoid this issue. > > Attached is a Perl version of the sed script, converted by hand (so > not the super-verbose s2p thing). It's basically just the sed script > with semicolons added and the backslashes in the regular expressions > moved around. I think we could use something like that for all > platforms now. I think it's alright, but please don't use literal tabs, use \t, even in a character class. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Wed, Nov 08, 2023 at 09:41:19AM +0100, Peter Eisentraut wrote: > I don't think we should rely on sed being there on Windows. Maybe it's true > now on the handful of buildfarm/CI machines and early adopters, but do we > have any indication that that is systematic or just an accident? Or both? When doing builds based on MinGW in the past I vaguely recall getting annoyed that I needed to look for sed as one thing, so your suggestion could simplify the experience a bit. > Since we definitely require Perl now, we could just as well use the Perl > script and avoid this issue. > > Attached is a Perl version of the sed script, converted by hand (so not the > super-verbose s2p thing). It's basically just the sed script with > semicolons added and the backslashes in the regular expressions moved > around. I think we could use something like that for all platforms now. Sounds like a good idea to me now that perl is a hard requirement. +1. -- Michael
Вложения
On 09.11.23 00:05, Michael Paquier wrote: >> Attached is a Perl version of the sed script, converted by hand (so not the >> super-verbose s2p thing). It's basically just the sed script with >> semicolons added and the backslashes in the regular expressions moved >> around. I think we could use something like that for all platforms now. > > Sounds like a good idea to me now that perl is a hard requirement. > +1. How about this patch as a comprehensive solution?
Вложения
On Fri, Nov 10, 2023 at 08:38:21AM +0100, Peter Eisentraut wrote: > How about this patch as a comprehensive solution? > 8 files changed, 26 insertions(+), 339 deletions(-) Thanks for the patch. The numbers are here, and the patch looks sensible. The contents of probes.h without --enable-trace are exactly the same before and after the patch. In short, +1 to switch to what you are proposing here. -- Michael
Вложения
On 13.11.23 06:30, Michael Paquier wrote: > On Fri, Nov 10, 2023 at 08:38:21AM +0100, Peter Eisentraut wrote: >> How about this patch as a comprehensive solution? >> 8 files changed, 26 insertions(+), 339 deletions(-) > > Thanks for the patch. The numbers are here, and the patch looks > sensible. > > The contents of probes.h without --enable-trace are exactly the same > before and after the patch. > > In short, +1 to switch to what you are proposing here. done
Other than the documentation details and the business about Gen_dummy_probes, which has been dealt with separately, this patch looks solid to me. On 12.10.23 07:23, Michael Paquier wrote: > On Thu, Oct 05, 2023 at 09:38:51AM +0200, Peter Eisentraut wrote: >> - I think we can't just delete install-windows.sgml. Some of that content >> needs to be moved over to installation.sgml. As a simple example, >> install-windows.sgml shows which MSVC versions are supported. That >> information should surely be kept. > > I've been thinking about the whole structure for a bit, but with the > MSVC scripts gone and the fact that we would rely on meson, moving > this information to the section under the platform-specific notes is > feeling kind of natural here. Here is a possible split of the > information across several sections: > - The requirements: > -- ActiveState Perl could be entirely removed, IMO. Perhaps we should > replace that to a reference to raspberry-perl, chocolatey or similar? > I am not sure about the best approach here, so for now I've kept the > bits about active perl. > -- bison and flex, which would become hard requirements on Windows > with Visual Studio now. Perhaps this could be unified with the patch > for distprep later on, but here we have specifics for Windows. > -- All the other optional requirements, tcl, etc. > - MinGW notes. > - Visual Studio notes, with the versions of visual supported, download > links, and a bit more. > - Notes specific about 64b builds. > > The attached is a bit crude and requires adjustments, but it shows the > idea. It's tricky. Eventually, we would like to reduce some of the duplication, like the whole list of requirements. But there are some Windows-specific details in there, so I don't know. My suggestion would be: Make a new <sect2 id="installation-notes-windows"> titled "Windows" at the end of installation.sgml (after the Solaris section). Dump most of the content from install-windows.sgml in there (except the stuff about the old build system). Rename the existing section "MinGW/Native Windows" to just "MinGW" and make some minor adjustments, similar to your patch. That way, we can move forward, and we can adjust and trim the details of the documentation over time.
On Tue, Nov 14, 2023 at 11:18:52AM +0100, Peter Eisentraut wrote: > Other than the documentation details and the business about > Gen_dummy_probes, which has been dealt with separately, this patch looks > solid to me. Thanks. > It's tricky. Eventually, we would like to reduce some of the duplication, > like the whole list of requirements. But there are some Windows-specific > details in there, so I don't know. Yes, that's something I was considering but polluting the meson dependency list with Windows-specific links was not the best way forward to me, because there are few users who care about knowing where Active Perl or an equivalent is located. > My suggestion would be: > > Make a new <sect2 id="installation-notes-windows"> titled "Windows" at the > end of installation.sgml (after the Solaris section). Dump most of the > content from install-windows.sgml in there (except the stuff about the old > build system). Rename the existing section "MinGW/Native Windows" to just > "MinGW" and make some minor adjustments, similar to your patch. > > That way, we can move forward, and we can adjust and trim the details of the > documentation over time. The latest patch I have sent is close to that, actually. Instead of creating a new section, I have integrated the contents of install-windows.sgml into the existing section dedicated to MinGW and native Windows because some parts apply to both of them, like the crash reporting facility. So this gave the following structure: - sect2 MinGW/Native Windows -- sect3 Requirements -- sect3 MinGW -- sect3 Visual Studio -- sect3 Special Considerations for 64-Bit Windows -- sect3 Collecting Crash Dumps on Windows The last parts affects both MinGW and VS builds, while the first requirement part applies only to native (references to MinGW are only there to handle dependencies for the builds). So I'm OK to live with a bit of duplication across two sect2 rather than attempt to unify them, while renaming the current MinGW/native section. With the requirements and the SDK-related guidelines, all the information seems from the original install-windows.sgml seems to be around. Hopefully I did not miss a spot. Attached is a v4. hamerkop and bowerbird still rely on that in the buildfarm today. -- Michael
Вложения
On 15.11.23 05:49, Michael Paquier wrote: > Attached is a v4. I'm happy with that. (Note, however, that your rebase didn't pick up commits e7814b40d0 and b41b1a7f49 that I did yesterday. Please check that again.)
Hi,
On 2023-11-15 13:49:06 +0900, Michael Paquier wrote:
> The latest patch I have sent is close to that, actually. Instead of
> creating a new section, I have integrated the contents of
> install-windows.sgml into the existing section dedicated to MinGW and
> native Windows because some parts apply to both of them, like the
> crash reporting facility. So this gave the following structure:
> - sect2 MinGW/Native Windows
> -- sect3 Requirements
> -- sect3 MinGW
> -- sect3 Visual Studio
> -- sect3 Special Considerations for 64-Bit Windows
> -- sect3 Collecting Crash Dumps on Windows
It doesn't seem like your patch has it quite that way? I see
<sect2 id="installation-notes-mingw">
<title>MinGW</title>
...
<sect2 id="installation-notes-windows">
<title>Windows</title>
Where "Windows" actually seems to solely describe visual studio? That seems
confusing.
> diff --git a/src/port/pgstrsignal.c b/src/port/pgstrsignal.c
> index 7d76d1cca9..8c10a760c6 100644
> --- a/src/port/pgstrsignal.c
> +++ b/src/port/pgstrsignal.c
> @@ -6,9 +6,6 @@
> * On platforms compliant with modern POSIX, this just wraps strsignal(3).
> * Elsewhere, we do the best we can.
> *
> - * This file is not currently built in MSVC builds, since it's useless
> - * on non-Unix platforms.
> - *
> * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
> * Portions Copyright (c) 1994, Regents of the University of California
> *
Huh, so this was wrong since the code was added? For a moment I thought I'd
unintentionally promoted it to be built by default, but ...
> index eca930ae47..14c9905b60 100644
> --- a/src/bin/pgevent/meson.build
> +++ b/src/bin/pgevent/meson.build
> @@ -14,7 +14,6 @@ pgevent_sources += rc_bin_gen.process(win32ver_rc, extra_args: [
>
> pgevent_sources += windows.compile_resources('pgmsgevent.rc')
>
> -# FIXME: copied from Mkvcbuild.pm, but I don't think that's the right approach
> pgevent_link_args = []
> if cc.get_id() == 'msvc'
> pgevent_link_args += '/ignore:4104'
I think it's worth leaving a trail indicating that adding this
warning-suppression is dubious at best. It seems to pretty obviously paper
over us exporting the symbols the wrong way:
https://learn.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-warning-lnk4104?view=msvc-170
Which pretty clearly explains that pgevent.def is wrong.
I just can't really test it, nor does it have test. Otherwise I might have
fixed it.
> @@ -53,10 +53,25 @@ AC_DEFUN([PGAC_CHECK_PERL_CONFIGS],
> # would be fatal to try to compile PL/Perl to a different libc ABI than core
> # Postgres uses. The available information says that most symbols that affect
> # Perl's own ABI begin with letters, so it's almost sufficient to adopt -D
> -# switches for symbols not beginning with underscore. Some exceptions are the
> -# Windows-specific -D_USE_32BIT_TIME_T and -D__MINGW_USE_VC2005_COMPAT; see
> -# Mkvcbuild.pm for details. We absorb the former when Perl reports it. Perl
> -# never reports the latter, and we don't attempt to deduce when it's needed.
> +# switches for symbols not beginning with underscore.
> +
> +# Some exceptions are the Windows-specific -D_USE_32BIT_TIME_T and
> +# -D__MINGW_USE_VC2005_COMPAT. To be exact, Windows offers several 32-bit ABIs.
> +# Perl is sensitive to sizeof(time_t), one of the ABI dimensions. To get
> +# 32-bit time_t, use "cl -D_USE_32BIT_TIME_T" or plain "gcc". For 64-bit
> +# time_t, use "gcc -D__MINGW_USE_VC2005_COMPAT" or plain "cl". Before MSVC
> +# 2005, plain "cl" chose 32-bit time_t. PostgreSQL doesn't support building
> +# with pre-MSVC-2005 compilers, but it does support linking to Perl built with
> +# such a compiler. MSVC-built Perl 5.13.4 and later report -D_USE_32BIT_TIME_T
> +# in $Config{ccflags} if applicable, but MinGW-built Perl never reports
> +# -D_USE_32BIT_TIME_T despite typically needing it.
Hm, it's pretty odd to have comments about cl.exe here, given that it can't
even be used with msvc.
My impression from testing this is that absorbing the flag from perl suffices
with strawberry perl and mingw perl, both when building with mingw and msvc.
> +# Ignore the $Config{ccflags} opinion about -D_USE_32BIT_TIME_T, and use a
> +# runtime test to deduce the ABI Perl expects. Specifically, test use of
> +# PL_modglobal, which maps to a PerlInterpreter field whose position depends
> +# on sizeof(time_t). We absorb the former when Perl reports it. Perl never
> +# reports the latter, and we don't attempt to deduce when it's needed.
I don't think this is implemented anywhere now?
> + <para>
> + PostgreSQL for Windows can be built using meson, as described
> + in <xref linkend="install-meson"/>.
> + The native Windows port requires a 32 or 64-bit version of Windows
> + 2000 or later. Earlier operating systems do
> + not have sufficient infrastructure (but Cygwin may be used on
> + those).
> + </para>
Is this actually true? I don't think we build on win2k...
> + <para>
> + Native builds of <application>psql</application> don't support command
> + line editing. The <productname>Cygwin</productname> build does support
> + command line editing, so it should be used where psql is needed for
> + interactive use on <productname>Windows</productname>.
> + </para>
FWIW, the last time I tested it, readline worked.
https://postgr.es/m/20221124023251.k4dnbmxuxmqzq7w3%40awork3.anarazel.de
> + <para>
> + PostgreSQL can be built using the Visual C++ compiler suite from Microsoft.
> + These compilers can be either from <productname>Visual Studio</productname>,
> + <productname>Visual Studio Express</productname> or some versions of the
> + <productname>Microsoft Windows SDK</productname>. If you do not already have a
> + <productname>Visual Studio</productname> environment set up, the easiest
> + ways are to use the compilers from
> + <productname>Visual Studio 2022</productname> or those in the
> + <productname>Windows SDK 10</productname>, which are both free downloads
> + from Microsoft.
> + </para>
I think we need a reference to mingw somewhere around here. I don't think
everybody can be expected to just know that they should not have navigated to
"Windows" but "MinGW".
> + <variablelist>
> + <varlistentry>
> + <term><productname>ActiveState Perl</productname></term>
> + <listitem><para>
> + ActiveState Perl is required to run the build generation scripts. MinGW
> + or Cygwin Perl will not work. It must also be present in the PATH.
> + Binaries can be downloaded from
> + <ulink url="https://www.activestate.com"></ulink>
> + (Note: version 5.14 or later is required,
> + the free Standard Distribution is sufficient).
> + </para></listitem>
> + </varlistentry>
Continuing to recommend ActiveState perl seems dubious, but I guess that's
material for another patch.
> + <varlistentry>
> + <term><productname>Bison</productname> and
> + <productname>Flex</productname></term>
> + <listitem>
> + <para>
> + <productname>Bison</productname> and <productname>Flex</productname> are
> + required to build from Git, but not required when building from a release
> + file. Only <productname>Bison</productname> versions 2.3 and later
> + will work. <productname>Flex</productname> must be version 2.5.35 or later.
> + </para>
> +
> + <para>
> + Both <productname>Bison</productname> and <productname>Flex</productname>
> + are included in the <productname>msys</productname> tool suite, available
> + from <ulink url="http://www.mingw.org/wiki/MSYS"></ulink> as part of the
> + <productname>MinGW</productname> compiler suite.
> + </para>
> +
> + <para>
> + You will need to add the directory containing
> + <filename>flex.exe</filename> and <filename>bison.exe</filename> to the
> + PATH environment variable. In the case of MinGW, the directory is the
> + <filename>\msys\1.0\bin</filename> subdirectory of your MinGW
> + installation directory.
> + </para>
I found it a lot easier to use https://github.com/lexxmark/winflexbison
> + <varlistentry>
> + <term><productname>MIT Kerberos</productname></term>
> + <listitem><para>
> + Required for GSSAPI authentication support. MIT Kerberos can be
> + downloaded from
> + <ulink url="https://web.mit.edu/Kerberos/dist/index.html"></ulink>.
> + </para></listitem>
> + </varlistentry>
> +
> + <varlistentry>
> + <term><productname>libxml2</productname> and
> + <productname>libxslt</productname></term>
> + <listitem><para>
> + Required for XML support. Binaries can be downloaded from
> + <ulink url="https://zlatkovic.com/pub/libxml"></ulink> or source from
> + <ulink url="http://xmlsoft.org"></ulink>. Note that libxml2 requires iconv,
> + which is available from the same download location.
> + </para></listitem>
> + </varlistentry>
> +
> + <varlistentry>
> + <term><productname>LZ4</productname></term>
> + <listitem><para>
> + Required for supporting <productname>LZ4</productname> compression.
> + Binaries and source can be downloaded from
> + <ulink url="https://github.com/lz4/lz4/releases"></ulink>.
> + </para></listitem>
> + </varlistentry>
> +
> + <varlistentry>
> + <term><productname>Zstandard</productname></term>
> + <listitem><para>
> + Required for supporting <productname>Zstandard</productname> compression.
> + Binaries and source can be downloaded from
> + <ulink url="https://github.com/facebook/zstd/releases"></ulink>.
> + </para></listitem>
> + </varlistentry>
> +
> + <varlistentry>
> + <term><productname>OpenSSL</productname></term>
> + <listitem><para>
> + Required for SSL support. Binaries can be downloaded from
> + <ulink url="https://slproweb.com/products/Win32OpenSSL.html"></ulink>
> + or source from <ulink url="https://www.openssl.org"></ulink>.
> + </para></listitem>
> + </varlistentry>
> +
> + <varlistentry>
> + <term><productname>ossp-uuid</productname></term>
> + <listitem><para>
> + Required for UUID-OSSP support (contrib only). Source can be
> + downloaded from
> + <ulink url="http://www.ossp.org/pkg/lib/uuid/"></ulink>.
> + </para></listitem>
> + </varlistentry>
> +
> + <varlistentry>
> + <term><productname>Python</productname></term>
> + <listitem><para>
> + Required for building <application>PL/Python</application>. Binaries can
> + be downloaded from <ulink url="https://www.python.org"></ulink>.
> + </para></listitem>
> + </varlistentry>
> +
> + <varlistentry>
> + <term><productname>zlib</productname></term>
> + <listitem><para>
> + Required for compression support in <application>pg_dump</application>
> + and <application>pg_restore</application>. Binaries can be downloaded
> + from <ulink url="https://www.zlib.net"></ulink>.
> + </para></listitem>
> + </varlistentry>
Except for openssl, where the link is somewhat valuable, the rest don't really
seem to be specific to windows.
> + <sect3 id="install-windows-full-64-bit">
> + <title>Special Considerations for 64-Bit Windows</title>
> + <para>
> + PostgreSQL will only build for the x64 architecture on 64-bit Windows.
> + </para>
> + <para>
> + Mixing 32- and 64-bit versions in the same build tree is not supported.
> + The build system will automatically detect if it's running in a 32- or
> + 64-bit environment, and build PostgreSQL accordingly. For this reason, it
> + is important to start the correct command prompt before building.
> + </para>
Isn't this directly contradicting the earlier
> + The native Windows port requires a 32 or 64-bit version of Windows
> + 2000 or later. Earlier operating systems do
?
> + <para>
> + To use a server-side third party library such as <productname>Python</productname> or
> + <productname>OpenSSL</productname>, this library <emphasis>must</emphasis> also be
> + 64-bit. There is no support for loading a 32-bit library in a 64-bit
> + server. Several of the third party libraries that PostgreSQL supports may
> + only be available in 32-bit versions, in which case they cannot be used with
> + 64-bit PostgreSQL.
> + </para>
> + </sect3>
I.e. cannot be used with postgres at all.
Thank you for working on this!
- Andres
On Wed, Nov 15, 2023 at 11:27:13AM +0100, Peter Eisentraut wrote: > (Note, however, that your rebase didn't pick up commits e7814b40d0 and > b41b1a7f49 that I did yesterday. Please check that again.) Indeed. I need to absorb that properly. -- Michael
Вложения
On Wed, Nov 15, 2023 at 05:07:03PM -0800, Andres Freund wrote: > On 2023-11-15 13:49:06 +0900, Michael Paquier wrote: > Where "Windows" actually seems to solely describe visual studio? That seems > confusing. Yeah, switch that to Visual. > Huh, so this was wrong since the code was added? For a moment I thought I'd > unintentionally promoted it to be built by default, but ... Yes, I was wondering if there could be an argument for simplifying some code here by pushing more logic into this wrapper, but I'm finding that a bit unappealing, and building it under Visual has no actual consequence: it seems that we never call pg_strsignal() under WIN32. >> -# FIXME: copied from Mkvcbuild.pm, but I don't think that's the right approach >> pgevent_link_args = [] >> if cc.get_id() == 'msvc' >> pgevent_link_args += '/ignore:4104' > > I think it's worth leaving a trail indicating that adding this > warning-suppression is dubious at best. It seems to pretty obviously paper > over us exporting the symbols the wrong way: > https://learn.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-warning-lnk4104?view=msvc-170 > > Which pretty clearly explains that pgevent.def is wrong. > > I just can't really test it, nor does it have test. Otherwise I might have > fixed it. Agreed that there is a good argument for removing it at some point, with a separate investigation. I've just added a XXX comment for now. >> @@ -53,10 +53,25 @@ AC_DEFUN([PGAC_CHECK_PERL_CONFIGS], >> # would be fatal to try to compile PL/Perl to a different libc ABI than core >> # Postgres uses. The available information says that most symbols that affect >> # Perl's own ABI begin with letters, so it's almost sufficient to adopt -D >> -# switches for symbols not beginning with underscore. Some exceptions are the >> -# Windows-specific -D_USE_32BIT_TIME_T and -D__MINGW_USE_VC2005_COMPAT; see >> -# Mkvcbuild.pm for details. We absorb the former when Perl reports it. Perl >> -# never reports the latter, and we don't attempt to deduce when it's needed. >> +# switches for symbols not beginning with underscore. >> + >> +# Some exceptions are the Windows-specific -D_USE_32BIT_TIME_T and >> +# -D__MINGW_USE_VC2005_COMPAT. To be exact, Windows offers several 32-bit ABIs. >> +# Perl is sensitive to sizeof(time_t), one of the ABI dimensions. To get >> +# 32-bit time_t, use "cl -D_USE_32BIT_TIME_T" or plain "gcc". For 64-bit >> +# time_t, use "gcc -D__MINGW_USE_VC2005_COMPAT" or plain "cl". Before MSVC >> +# 2005, plain "cl" chose 32-bit time_t. PostgreSQL doesn't support building >> +# with pre-MSVC-2005 compilers, but it does support linking to Perl built with >> +# such a compiler. MSVC-built Perl 5.13.4 and later report -D_USE_32BIT_TIME_T >> +# in $Config{ccflags} if applicable, but MinGW-built Perl never reports >> +# -D_USE_32BIT_TIME_T despite typically needing it. > > Hm, it's pretty odd to have comments about cl.exe here, given that it can't > even be used with msvc. > > My impression from testing this is that absorbing the flag from perl suffices > with strawberry perl and mingw perl, both when building with mingw and msvc. I was a bit uncomfortable with removing these references, but I suspect that you are right and that they're outdated artifacts of the past. So I'm OK to remove the cl and gcc parts as the flags come from $PERL. >> +# Ignore the $Config{ccflags} opinion about -D_USE_32BIT_TIME_T, and use a >> +# runtime test to deduce the ABI Perl expects. Specifically, test use of >> +# PL_modglobal, which maps to a PerlInterpreter field whose position depends >> +# on sizeof(time_t). We absorb the former when Perl reports it. Perl never >> +# reports the latter, and we don't attempt to deduce when it's needed. > > I don't think this is implemented anywhere now? Indeed, that's now gone. >> + <para> >> + PostgreSQL for Windows can be built using meson, as described >> + in <xref linkend="install-meson"/>. >> + The native Windows port requires a 32 or 64-bit version of Windows >> + 2000 or later. Earlier operating systems do >> + not have sufficient infrastructure (but Cygwin may be used on >> + those). >> + </para> > > Is this actually true? I don't think we build on win2k... Nah, this is a reference outdated for ages. 495ed0ef2d72 has even bumped _WIN32_WINNT to require Windows 10 as the minimal runtime version supported, so this needs to be updated and backpatched. The first two sentences can be simplified like that: - The native Windows port requires a 32 or 64-bit version of Windows - 2000 or later. Earlier operating systems do - not have sufficient infrastructure (but Cygwin may be used on - those). + The native Windows port requires a 32 or 64-bit version of Windows + 10 or later. Earlier operating systems do not have sufficient + infrastructure. Even the second sentence could be entirely removed, I don't see much advantage in keeping it. Would you be OK with that, as a separate patch? I've updated the refernce in the attached. >> + <para> >> + Native builds of <application>psql</application> don't support command >> + line editing. The <productname>Cygwin</productname> build does support >> + command line editing, so it should be used where psql is needed for >> + interactive use on <productname>Windows</productname>. >> + </para> > > FWIW, the last time I tested it, readline worked. > > https://postgr.es/m/20221124023251.k4dnbmxuxmqzq7w3%40awork3.anarazel.de Okay. I couldn't really make it work, FWIW. Perhaps this is just something that could be tweaked in a different patch. What you are mentioning requires quite a few steps, and I am not sure if this is the safest and/or the easiest way to achieve that, TBH. I'd keep that as a separate investigation for now. >> + <para> >> + PostgreSQL can be built using the Visual C++ compiler suite from Microsoft. >> + These compilers can be either from <productname>Visual Studio</productname>, >> + <productname>Visual Studio Express</productname> or some versions of the >> + <productname>Microsoft Windows SDK</productname>. If you do not already have a >> + <productname>Visual Studio</productname> environment set up, the easiest >> + ways are to use the compilers from >> + <productname>Visual Studio 2022</productname> or those in the >> + <productname>Windows SDK 10</productname>, which are both free downloads >> + from Microsoft. >> + </para> > > I think we need a reference to mingw somewhere around here. I don't think > everybody can be expected to just know that they should not have navigated to > "Windows" but "MinGW". Hmm. But if this is a section only for Visual, it doesn't make sense to me to mention MinGW here? I am not sure to follow how this is in line with the previous comments. > Continuing to recommend ActiveState perl seems dubious, but I guess that's > material for another patch. I want to see this reference entirely gone at the end with more stuff trimmed. For now I'm focusing on a simpler restructure. >> + <varlistentry> >> + <term><productname>Bison</productname> and >> + <productname>Flex</productname></term> >> + <listitem> >> + <para> >> + <productname>Bison</productname> and <productname>Flex</productname> are >> + required to build from Git, but not required when building from a release >> + file. Only <productname>Bison</productname> versions 2.3 and later >> + will work. <productname>Flex</productname> must be version 2.5.35 or later. >> + </para> >> + >> + <para> >> + Both <productname>Bison</productname> and <productname>Flex</productname> >> + are included in the <productname>msys</productname> tool suite, available >> + from <ulink url="http://www.mingw.org/wiki/MSYS"></ulink> as part of the >> + <productname>MinGW</productname> compiler suite. >> + </para> >> + >> + <para> >> + You will need to add the directory containing >> + <filename>flex.exe</filename> and <filename>bison.exe</filename> to the >> + PATH environment variable. In the case of MinGW, the directory is the >> + <filename>\msys\1.0\bin</filename> subdirectory of your MinGW >> + installation directory. >> + </para> > > I found it a lot easier to use https://github.com/lexxmark/winflexbison And I've been using chocolatey to fetch some dependencies. I think that trimming this stuff should be discussed in a separate patch. > Except for openssl, where the link is somewhat valuable, the rest don't really > seem to be specific to windows. Yeah, these are historic. Still they can be useful for the Visual builds in some cases, I guess? I am not sure if it's worth pushing these dependencies to the main meson page, somewhat polluting it for references that most people don't really care about. Anyway, I'm tempted to be less ambitious in a first step and just move that in the compatibility section. >> + <sect3 id="install-windows-full-64-bit"> >> + <title>Special Considerations for 64-Bit Windows</title> >> + <para> >> + PostgreSQL will only build for the x64 architecture on 64-bit Windows. >> + </para> >> + <para> >> + Mixing 32- and 64-bit versions in the same build tree is not supported. >> + The build system will automatically detect if it's running in a 32- or >> + 64-bit environment, and build PostgreSQL accordingly. For this reason, it >> + is important to start the correct command prompt before building. >> + </para> > >> Isn't this directly contradicting the earlier >> + The native Windows port requires a 32 or 64-bit version of Windows >> + 2000 or later. Earlier operating systems do > ? How it that? Mixing 32b and 64b libraries is not related to the minimal runtime version. This is just telling to not mix both. -- Michael
Вложения
On 16.11.23 02:07, Andres Freund wrote: > It doesn't seem like your patch has it quite that way? I see > > <sect2 id="installation-notes-mingw"> > <title>MinGW</title> > ... > <sect2 id="installation-notes-windows"> > <title>Windows</title> > > Where "Windows" actually seems to solely describe visual studio? That seems > confusing. I had suggested this arrangement as a way to reduce churn in this patch set. We'd just move over the existing separate chapter into a new section, and then later consider further rearrangements. It's not always clear where all of these things should go, as there are so many dimensions. For example, the existing sentence "After you have everything installed, it is suggested that you run psql under CMD.EXE, as the MSYS console has buffering issues.", does that apply to MinGW, or really MSYS, or does it also apply if you build with Visual Something? Ultimately, I don't think MinGW needs to be its own section.
On Mon, Nov 20, 2023 at 08:00:09AM +0100, Peter Eisentraut wrote: > I had suggested this arrangement as a way to reduce churn in this patch set. > We'd just move over the existing separate chapter into a new section, and > then later consider further rearrangements. > > It's not always clear where all of these things should go, as there are so > many dimensions. For example, the existing sentence "After you have > everything installed, it is suggested that you run psql under CMD.EXE, as > the MSYS console has buffering issues.", does that apply to MinGW, or really > MSYS, or does it also apply if you build with Visual Something? Even for this specific one, are you sure that it still applies? :D > Ultimately, I don't think MinGW needs to be its own section. Yes, agreed. The end result should be one single sect2 for Windows divided into multiple sect3, perhaps themselves divided into more sect4 for each build method. As a whole, before refactoring all that, I'd be in favor of a slightly different strategy once the MSVC scripts and install-windows.sgml with its stuff specific to src/tools/msvc are gone: - Review all this section from the docs and trim them from everything that we think is now irrelevant. - Look at the rest and see how it can be efficiently refactored into balanced sections. Your suggestion to create a new sect2 for "Windows" as much as Andres' suggestion are OK by as an intermediate step, and I suspect that the end result will likely not be that. -- Michael
Вложения
On Tue, Sep 26, 2023 at 12:17:04PM -0400, Andrew Dunstan wrote: > On 2023-09-26 Tu 01:25, NINGWEI CHEN wrote: >> hamerkop is not yet prepared for Meson builds, but we plan to work on this support soon. >> If we go with Meson builds exclusively right now, we will have to temporarily remove the master/HEAD for a while. > > I don't think we should switch to that until you're ready. Agreed that it would just be breaking a build for the sake of breaking it. Saying that, the last exchange that we had about hamerkop switching to meson was two months ago. Are there any plans to do the switch? -- Michael
Вложения
On 2023-12-04 Mo 03:05, Michael Paquier wrote: > On Tue, Sep 26, 2023 at 12:17:04PM -0400, Andrew Dunstan wrote: >> On 2023-09-26 Tu 01:25, NINGWEI CHEN wrote: >>> hamerkop is not yet prepared for Meson builds, but we plan to work on this support soon. >>> If we go with Meson builds exclusively right now, we will have to temporarily remove the master/HEAD for a while. >> I don't think we should switch to that until you're ready. > Agreed that it would just be breaking a build for the sake of breaking > it. Saying that, the last exchange that we had about hamerkop > switching to meson was two months ago. Are there any plans to do the > switch? I just had a look at shifting bowerbird to use meson, and it got stymied at the c99 test, which apparently doesn't compile with anything less than VS2019. I can upgrade bowerbird, but that will take rather longer. It looks like hamerkop is in th4e same boat. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Mon, Dec 04, 2023 at 03:11:47PM -0500, Andrew Dunstan wrote: > I just had a look at shifting bowerbird to use meson, and it got stymied at > the c99 test, which apparently doesn't compile with anything less than > VS2019. > > I can upgrade bowerbird, but that will take rather longer. It looks like > hamerkop is in th4e same boat. Okay. Thanks for the update. -- Michael
Вложения
On 04.12.23 21:11, Andrew Dunstan wrote: > I just had a look at shifting bowerbird to use meson, and it got stymied > at the c99 test, which apparently doesn't compile with anything less > than VS2019. If that is the case, then wouldn't that invalidate the documented claim that you can build with VS2015 or newer?
On Fri, Nov 17, 2023 at 6:31 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Nov 15, 2023 at 05:07:03PM -0800, Andres Freund wrote: > > On 2023-11-15 13:49:06 +0900, Michael Paquier wrote: > > Where "Windows" actually seems to solely describe visual studio? That seems > > confusing. > > Yeah, switch that to Visual. > > > Huh, so this was wrong since the code was added? For a moment I thought I'd > > unintentionally promoted it to be built by default, but ... > > Yes, I was wondering if there could be an argument for simplifying > some code here by pushing more logic into this wrapper, but I'm > finding that a bit unappealing, and building it under Visual has no > actual consequence: it seems that we never call pg_strsignal() under > WIN32. > > >> -# FIXME: copied from Mkvcbuild.pm, but I don't think that's the right approach > >> pgevent_link_args = [] > >> if cc.get_id() == 'msvc' > >> pgevent_link_args += '/ignore:4104' > > > > I think it's worth leaving a trail indicating that adding this > > warning-suppression is dubious at best. It seems to pretty obviously paper > > over us exporting the symbols the wrong way: > > https://learn.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-warning-lnk4104?view=msvc-170 > > > > Which pretty clearly explains that pgevent.def is wrong. > > > > I just can't really test it, nor does it have test. Otherwise I might have > > fixed it. > > Agreed that there is a good argument for removing it at some point, > with a separate investigation. I've just added a XXX comment for now. > > >> @@ -53,10 +53,25 @@ AC_DEFUN([PGAC_CHECK_PERL_CONFIGS], > >> # would be fatal to try to compile PL/Perl to a different libc ABI than core > >> # Postgres uses. The available information says that most symbols that affect > >> # Perl's own ABI begin with letters, so it's almost sufficient to adopt -D > >> -# switches for symbols not beginning with underscore. Some exceptions are the > >> -# Windows-specific -D_USE_32BIT_TIME_T and -D__MINGW_USE_VC2005_COMPAT; see > >> -# Mkvcbuild.pm for details. We absorb the former when Perl reports it. Perl > >> -# never reports the latter, and we don't attempt to deduce when it's needed. > >> +# switches for symbols not beginning with underscore. > >> + > >> +# Some exceptions are the Windows-specific -D_USE_32BIT_TIME_T and > >> +# -D__MINGW_USE_VC2005_COMPAT. To be exact, Windows offers several 32-bit ABIs. > >> +# Perl is sensitive to sizeof(time_t), one of the ABI dimensions. To get > >> +# 32-bit time_t, use "cl -D_USE_32BIT_TIME_T" or plain "gcc". For 64-bit > >> +# time_t, use "gcc -D__MINGW_USE_VC2005_COMPAT" or plain "cl". Before MSVC > >> +# 2005, plain "cl" chose 32-bit time_t. PostgreSQL doesn't support building > >> +# with pre-MSVC-2005 compilers, but it does support linking to Perl built with > >> +# such a compiler. MSVC-built Perl 5.13.4 and later report -D_USE_32BIT_TIME_T > >> +# in $Config{ccflags} if applicable, but MinGW-built Perl never reports > >> +# -D_USE_32BIT_TIME_T despite typically needing it. > > > > Hm, it's pretty odd to have comments about cl.exe here, given that it can't > > even be used with msvc. > > > > My impression from testing this is that absorbing the flag from perl suffices > > with strawberry perl and mingw perl, both when building with mingw and msvc. > > I was a bit uncomfortable with removing these references, but I > suspect that you are right and that they're outdated artifacts of the > past. So I'm OK to remove the cl and gcc parts as the flags come from > $PERL. > > >> +# Ignore the $Config{ccflags} opinion about -D_USE_32BIT_TIME_T, and use a > >> +# runtime test to deduce the ABI Perl expects. Specifically, test use of > >> +# PL_modglobal, which maps to a PerlInterpreter field whose position depends > >> +# on sizeof(time_t). We absorb the former when Perl reports it. Perl never > >> +# reports the latter, and we don't attempt to deduce when it's needed. > > > > I don't think this is implemented anywhere now? > > Indeed, that's now gone. > > >> + <para> > >> + PostgreSQL for Windows can be built using meson, as described > >> + in <xref linkend="install-meson"/>. > >> + The native Windows port requires a 32 or 64-bit version of Windows > >> + 2000 or later. Earlier operating systems do > >> + not have sufficient infrastructure (but Cygwin may be used on > >> + those). > >> + </para> > > > > Is this actually true? I don't think we build on win2k... > > Nah, this is a reference outdated for ages. 495ed0ef2d72 has even > bumped _WIN32_WINNT to require Windows 10 as the minimal runtime > version supported, so this needs to be updated and backpatched. The > first two sentences can be simplified like that: > - The native Windows port requires a 32 or 64-bit version of Windows > - 2000 or later. Earlier operating systems do > - not have sufficient infrastructure (but Cygwin may be used on > - those). > + The native Windows port requires a 32 or 64-bit version of Windows > + 10 or later. Earlier operating systems do not have sufficient > + infrastructure. > > Even the second sentence could be entirely removed, I don't see much > advantage in keeping it. Would you be OK with that, as a separate > patch? I've updated the refernce in the attached. > > >> + <para> > >> + Native builds of <application>psql</application> don't support command > >> + line editing. The <productname>Cygwin</productname> build does support > >> + command line editing, so it should be used where psql is needed for > >> + interactive use on <productname>Windows</productname>. > >> + </para> > > > > FWIW, the last time I tested it, readline worked. > > > > https://postgr.es/m/20221124023251.k4dnbmxuxmqzq7w3%40awork3.anarazel.de > > Okay. I couldn't really make it work, FWIW. Perhaps this is just > something that could be tweaked in a different patch. What you are > mentioning requires quite a few steps, and I am not sure if this is > the safest and/or the easiest way to achieve that, TBH. I'd keep that > as a separate investigation for now. > > >> + <para> > >> + PostgreSQL can be built using the Visual C++ compiler suite from Microsoft. > >> + These compilers can be either from <productname>Visual Studio</productname>, > >> + <productname>Visual Studio Express</productname> or some versions of the > >> + <productname>Microsoft Windows SDK</productname>. If you do not already have a > >> + <productname>Visual Studio</productname> environment set up, the easiest > >> + ways are to use the compilers from > >> + <productname>Visual Studio 2022</productname> or those in the > >> + <productname>Windows SDK 10</productname>, which are both free downloads > >> + from Microsoft. > >> + </para> > > > > I think we need a reference to mingw somewhere around here. I don't think > > everybody can be expected to just know that they should not have navigated to > > "Windows" but "MinGW". > > Hmm. But if this is a section only for Visual, it doesn't make sense > to me to mention MinGW here? I am not sure to follow how this is in > line with the previous comments. > > > Continuing to recommend ActiveState perl seems dubious, but I guess that's > > material for another patch. > > I want to see this reference entirely gone at the end with more > stuff trimmed. For now I'm focusing on a simpler restructure. > > >> + <varlistentry> > >> + <term><productname>Bison</productname> and > >> + <productname>Flex</productname></term> > >> + <listitem> > >> + <para> > >> + <productname>Bison</productname> and <productname>Flex</productname> are > >> + required to build from Git, but not required when building from a release > >> + file. Only <productname>Bison</productname> versions 2.3 and later > >> + will work. <productname>Flex</productname> must be version 2.5.35 or later. > >> + </para> > >> + > >> + <para> > >> + Both <productname>Bison</productname> and <productname>Flex</productname> > >> + are included in the <productname>msys</productname> tool suite, available > >> + from <ulink url="http://www.mingw.org/wiki/MSYS"></ulink> as part of the > >> + <productname>MinGW</productname> compiler suite. > >> + </para> > >> + > >> + <para> > >> + You will need to add the directory containing > >> + <filename>flex.exe</filename> and <filename>bison.exe</filename> to the > >> + PATH environment variable. In the case of MinGW, the directory is the > >> + <filename>\msys\1.0\bin</filename> subdirectory of your MinGW > >> + installation directory. > >> + </para> > > > > I found it a lot easier to use https://github.com/lexxmark/winflexbison > > And I've been using chocolatey to fetch some dependencies. I think > that trimming this stuff should be discussed in a separate patch. > > > Except for openssl, where the link is somewhat valuable, the rest don't really > > seem to be specific to windows. > > Yeah, these are historic. Still they can be useful for the Visual > builds in some cases, I guess? I am not sure if it's worth pushing > these dependencies to the main meson page, somewhat polluting it for > references that most people don't really care about. Anyway, I'm > tempted to be less ambitious in a first step and just move that in the > compatibility section. > > >> + <sect3 id="install-windows-full-64-bit"> > >> + <title>Special Considerations for 64-Bit Windows</title> > >> + <para> > >> + PostgreSQL will only build for the x64 architecture on 64-bit Windows. > >> + </para> > >> + <para> > >> + Mixing 32- and 64-bit versions in the same build tree is not supported. > >> + The build system will automatically detect if it's running in a 32- or > >> + 64-bit environment, and build PostgreSQL accordingly. For this reason, it > >> + is important to start the correct command prompt before building. > >> + </para> > > > >> Isn't this directly contradicting the earlier > >> + The native Windows port requires a 32 or 64-bit version of Windows > >> + 2000 or later. Earlier operating systems do > > ? > > How it that? Mixing 32b and 64b libraries is not related to the > minimal runtime version. This is just telling to not mix both. > -- > Patch is not applying. Please share the Rebased Version. Please find the error: D:\Project\Postgres>git am D:\Project\Patch\v5-0001-Remove-MSVC-scripts.patch error: patch failed: doc/src/sgml/filelist.sgml:38 error: doc/src/sgml/filelist.sgml: patch does not apply error: patch failed: src/tools/msvc/Mkvcbuild.pm:1 error: src/tools/msvc/Mkvcbuild.pm: patch does not apply error: patch failed: src/tools/msvc/Solution.pm:1 error: src/tools/msvc/Solution.pm: patch does not apply hint: Use 'git am --show-current-patch=diff' to see the failed patch Applying: Remove MSVC scripts Patch failed at 0001 Remove MSVC scripts When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Thanks and Regards, Shubham Khanna.
On Wed, Dec 06, 2023 at 12:15:50PM +0530, Shubham Khanna wrote: > Patch is not applying. Please share the Rebased Version. Please find the error: Thanks. Here you go with a v6. > D:\Project\Postgres>git am D:\Project\Patch\v5-0001-Remove-MSVC-scripts.patch > error: patch failed: doc/src/sgml/filelist.sgml:38 > error: doc/src/sgml/filelist.sgml: patch does not apply This is caused by the recent addition of targets-meson. > error: patch failed: src/tools/msvc/Mkvcbuild.pm:1 > error: src/tools/msvc/Mkvcbuild.pm: patch does not apply > error: patch failed: src/tools/msvc/Solution.pm:1 > error: src/tools/msvc/Solution.pm: patch does not apply And some stuff because these have been updated. -- Michael
Вложения
On 2023-12-06 We 01:18, Peter Eisentraut wrote: > On 04.12.23 21:11, Andrew Dunstan wrote: >> I just had a look at shifting bowerbird to use meson, and it got >> stymied at the c99 test, which apparently doesn't compile with >> anything less than VS2019. > > If that is the case, then wouldn't that invalidate the documented > claim that you can build with VS2015 or newer? Indeed it would. Here's what the Microsoft site says at <https://learn.microsoft.com/en-us/cpp/build/reference/std-specify-language-standard-version?view=msvc-170>: > You can invoke the Microsoft C compiler by using the /TC or /Tc > compiler option. It's used by default for code that has a .c file > extension, unless overridden by a /TP or /Tp option. The default C > compiler (that is, the compiler when /std:c11 or /std:c17 isn't > specified) implements ANSI C89, but includes several Microsoft > extensions, some of which are part of ISO C99. Some Microsoft > extensions to C89 can be disabled by using the /Za compiler option, > but others remain in effect. It isn't possible to specify strict C89 > conformance. The compiler doesn't implement several required features > of C99, so it isn't possible to specify C99 conformance, either. But the VS2019 compiler implements enough of C99 to pass our meson test, unlike VS2017. Maybe the test is too strict. After all, we know we can in fact build with the earlier versions. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 06.12.23 17:27, Andrew Dunstan wrote: > But the VS2019 compiler implements enough of C99 to pass our meson test, > unlike VS2017. Maybe the test is too strict. After all, we know we can > in fact build with the earlier versions. I just realized that the C99 test is actually our own, not provided by meson. (See "c99_test" in meson.build.) Can you try disabling a few bits of that to see what makes it pass for you? I suspect it's the structfunc() call.
On 2023-12-06 We 12:24, Peter Eisentraut wrote: > On 06.12.23 17:27, Andrew Dunstan wrote: >> But the VS2019 compiler implements enough of C99 to pass our meson >> test, unlike VS2017. Maybe the test is too strict. After all, we know >> we can in fact build with the earlier versions. > > I just realized that the C99 test is actually our own, not provided by > meson. (See "c99_test" in meson.build.) > > Can you try disabling a few bits of that to see what makes it pass for > you? I suspect it's the structfunc() call. Yes, if I comment out the call to structfunc() the test passes on VS2017 (compiler version 19.15.26726) cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 06.12.23 21:52, Andrew Dunstan wrote: > > On 2023-12-06 We 12:24, Peter Eisentraut wrote: >> On 06.12.23 17:27, Andrew Dunstan wrote: >>> But the VS2019 compiler implements enough of C99 to pass our meson >>> test, unlike VS2017. Maybe the test is too strict. After all, we know >>> we can in fact build with the earlier versions. >> >> I just realized that the C99 test is actually our own, not provided by >> meson. (See "c99_test" in meson.build.) >> >> Can you try disabling a few bits of that to see what makes it pass for >> you? I suspect it's the structfunc() call. > > > Yes, if I comment out the call to structfunc() the test passes on VS2017 > (compiler version 19.15.26726) This is strange, because we use code like that in the tree. There must be some small detail that trips it up here. Perhaps try moving the definition of struct named_init_test outside of the function, or make it a typedef.
On 2023-Dec-07, Peter Eisentraut wrote:
> On 06.12.23 21:52, Andrew Dunstan wrote:
> > Yes, if I comment out the call to structfunc() the test passes on VS2017
> > (compiler version 19.15.26726)
>
> This is strange, because we use code like that in the tree. There must be
> some small detail that trips it up here.
Well, We have things like these
typedef struct _archiveOpts
{
...
} ArchiveOpts;
#define ARCHIVE_OPTS(...) &(ArchiveOpts){__VA_ARGS__}
XL_ROUTINE is quite similar.
These are then used like
ARCHIVE_OPTS(.tag = "pg_largeobject",
.description = "pg_largeobject",
.section = SECTION_PRE_DATA,
.createStmt = loOutQry->data));
so the difference is that we're passing a pointer to a struct, not
the struct bare, which is what c99_test is doing:
struct named_init_test {
int a;
int b;
};
int main() {
...
structfunc((struct named_init_test){1, 0});
}
Maybe this would work if the function received the pointer too?
extern void structfunc(struct named_init_test *);
structfunc(&(struct named_init_test){1, 0});
The fact that this is called "structfunc" makes me wonder if the author
did indeed want to test passing a struct to the function. That'd be
odd, since the interesting thing in this line is the expression used to
initialize the struct argument. (We do pass structs, eg. ObjectAddress
to check_object_ownership; old code.)
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"No renuncies a nada. No te aferres a nada."
Hi, On 2023-12-04 15:11:47 -0500, Andrew Dunstan wrote: > I just had a look at shifting bowerbird to use meson, and it got stymied at > the c99 test, which apparently doesn't compile with anything less than > VS2019. What error or warning is being raised by msvc? Andres
Hi,
On 2023-12-07 12:33:35 +0100, Alvaro Herrera wrote:
> Well, We have things like these
>
> typedef struct _archiveOpts
> {
> ...
> } ArchiveOpts;
> #define ARCHIVE_OPTS(...) &(ArchiveOpts){__VA_ARGS__}
>
> XL_ROUTINE is quite similar.
>
> These are then used like
> ARCHIVE_OPTS(.tag = "pg_largeobject",
> .description = "pg_largeobject",
> .section = SECTION_PRE_DATA,
> .createStmt = loOutQry->data));
>
> so the difference is that we're passing a pointer to a struct, not
> the struct bare, which is what c99_test is doing:
>
> struct named_init_test {
> int a;
> int b;
> };
>
> int main() {
> ...
> structfunc((struct named_init_test){1, 0});
> }
>
> Maybe this would work if the function received the pointer too?
>
> extern void structfunc(struct named_init_test *);
>
> structfunc(&(struct named_init_test){1, 0});
>
> The fact that this is called "structfunc" makes me wonder if the author
> did indeed want to test passing a struct to the function. That'd be
> odd, since the interesting thing in this line is the expression used to
> initialize the struct argument. (We do pass structs, eg. ObjectAddress
> to check_object_ownership; old code.)
It seems like both might be interesting? But I think there's no reason to not
evolve this test if we need to. I think I wrote it testing with a few old *nix
compilers to see where -std=c99 was needed, not more. It's not too surprising
that it might need some massaging for older msvc...
However: I used godbolt to compile the test code on msvc, and it seems to
build with 19.15 (which is the version Andrew referenced upthread), with a
warning that's triggered independent of the structfunc bit.
https://godbolt.org/z/j99E9MeEK
Andrew, could you attach meson.log from the failed build?
Greetings,
Andres Freund
On 2023-12-07 Th 12:34, Andres Freund wrote:
> Hi,
>
> On 2023-12-07 12:33:35 +0100, Alvaro Herrera wrote:
>> Well, We have things like these
>>
>> typedef struct _archiveOpts
>> {
>> ...
>> } ArchiveOpts;
>> #define ARCHIVE_OPTS(...) &(ArchiveOpts){__VA_ARGS__}
>>
>> XL_ROUTINE is quite similar.
>>
>> These are then used like
>> ARCHIVE_OPTS(.tag = "pg_largeobject",
>> .description = "pg_largeobject",
>> .section = SECTION_PRE_DATA,
>> .createStmt = loOutQry->data));
>>
>> so the difference is that we're passing a pointer to a struct, not
>> the struct bare, which is what c99_test is doing:
>>
>> struct named_init_test {
>> int a;
>> int b;
>> };
>>
>> int main() {
>> ...
>> structfunc((struct named_init_test){1, 0});
>> }
>>
>> Maybe this would work if the function received the pointer too?
>>
>> extern void structfunc(struct named_init_test *);
>>
>> structfunc(&(struct named_init_test){1, 0});
>>
>> The fact that this is called "structfunc" makes me wonder if the author
>> did indeed want to test passing a struct to the function. That'd be
>> odd, since the interesting thing in this line is the expression used to
>> initialize the struct argument. (We do pass structs, eg. ObjectAddress
>> to check_object_ownership; old code.)
> It seems like both might be interesting? But I think there's no reason to not
> evolve this test if we need to. I think I wrote it testing with a few old *nix
> compilers to see where -std=c99 was needed, not more. It's not too surprising
> that it might need some massaging for older msvc...
>
>
> However: I used godbolt to compile the test code on msvc, and it seems to
> build with 19.15 (which is the version Andrew referenced upthread), with a
> warning that's triggered independent of the structfunc bit.
>
> https://godbolt.org/z/j99E9MeEK
>
>
> Andrew, could you attach meson.log from the failed build?
>
>
The odd thing is I tried to reproduce the issue and instead it's now
compiling with VS2017. The only thing I have changed on the machine was
to install VS2022 alongside VS2017, as well as switching which perl to
link to, which should have no effect on this.
So never mind, we make progress.
Not sure about VS 2015 though.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Hi, On 2023-12-07 13:49:44 -0500, Andrew Dunstan wrote: > On 2023-12-07 Th 12:34, Andres Freund wrote: > > However: I used godbolt to compile the test code on msvc, and it seems to > > build with 19.15 (which is the version Andrew referenced upthread), with a > > warning that's triggered independent of the structfunc bit. > > > > https://godbolt.org/z/j99E9MeEK > > > > > > Andrew, could you attach meson.log from the failed build? > > > > > > The odd thing is I tried to reproduce the issue and instead it's now > compiling with VS2017. The only thing I have changed on the machine was to > install VS2022 alongside VS2017, as well as switching which perl to link to, > which should have no effect on this. The error might have been due to an older C runtime. I think installing visual studio 2022 might have lead to updating the C runtime associated with VS 2017 to a newer release of VS 2017. Or even updated the version of VS 2017 - I find the version numbers of msvc vs visual studio incomprehensible, but I think 19.15.26726 is from 2017, missing a lot of bugfixes that were made to VS 2017 since. Greetings, Andres Freund
On Thu, Dec 7, 2023 at 5:27 AM Andrew Dunstan <andrew@dunslane.net> wrote:
> But the VS2019 compiler implements enough of C99 to pass our meson test,
> unlike VS2017. Maybe the test is too strict. After all, we know we can
> in fact build with the earlier versions.
. o O { I wish master would systematically drop support for compilers
that were out of 'mainstream' vendor support. }
On Fri, Dec 08, 2023 at 08:50:47AM +1300, Thomas Munro wrote:
> . o O { I wish master would systematically drop support for compilers
> that were out of 'mainstream' vendor support. }
Calling for a patch once, twice ;p
FWIW, I would not mind marking VS 2019 as the minimum requirement on
HEAD once the MSVC scripts are gone. The oldest VS version tested in
the buildfarm is hamerkop with VS2017, still under the MSVC scripts.
I'd like to believe that a switch to meson implies a newer version of
VS installed there.
--
Michael
Вложения
On Tue, Dec 05, 2023 at 07:29:59AM +0900, Michael Paquier wrote: > Okay. Thanks for the update. While in Prague, Andres and Peter E. have mentioned me that we perhaps had better move on with this patch sooner than later, without waiting for the two buildfarm members to do the switch because much more cleanup is required for the documentation once the scripts are removed. So, any objections with the patch as presented to remove the scripts while moving the existing doc blocks from install-windows.sgml that still need more discussion? -- Michael
Вложения
On 2023-12-13 We 09:23, Michael Paquier wrote: > On Tue, Dec 05, 2023 at 07:29:59AM +0900, Michael Paquier wrote: >> Okay. Thanks for the update. > While in Prague, Andres and Peter E. have mentioned me that we perhaps > had better move on with this patch sooner than later, without waiting > for the two buildfarm members to do the switch because much more > cleanup is required for the documentation once the scripts are > removed. > > So, any objections with the patch as presented to remove the scripts > while moving the existing doc blocks from install-windows.sgml that > still need more discussion? TBH I'd prefer to wait. But I have had a couple more urgent things on my plate. I hope to get back to it before New Year. In the meantime I have switched bowerbird to building only STABLE branches. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Mon, 4 Dec 2023 17:05:24 +0900 Michael Paquier <michael@paquier.xyz> wrote: > On Tue, Sep 26, 2023 at 12:17:04PM -0400, Andrew Dunstan wrote: > > On 2023-09-26 Tu 01:25, NINGWEI CHEN wrote: > >> hamerkop is not yet prepared for Meson builds, but we plan to work on this support soon. > >> If we go with Meson builds exclusively right now, we will have to temporarily remove the master/HEAD for a while. > > > > I don't think we should switch to that until you're ready. > > Agreed that it would just be breaking a build for the sake of breaking > it. Saying that, the last exchange that we had about hamerkop > switching to meson was two months ago. Are there any plans to do the > switch? > -- > Michael Sorry for the delayed response. We are currently working on transitioning to meson build at hamerkop and anticipating that this can be accomplished by no later than January. If the old build scripts are removed before that, hamerkop will be temporarily taken off the master branch, and will rejoin once the adjustment is done. Best Regards. -- SRA OSS LLC Chen Ningwei<chen@sraoss.co.jp>
On Thu, Dec 14, 2023 at 11:43:14AM +0900, NINGWEI CHEN wrote: > Sorry for the delayed response. > We are currently working on transitioning to meson build at hamerkop and > anticipating that this can be accomplished by no later than January. > > If the old build scripts are removed before that, hamerkop will be temporarily > taken off the master branch, and will rejoin once the adjustment is done. Thanks for the update. Let's move on with that on HEAD then. I've wanted some room to work on improving the set of docs for v17. -- Michael
Вложения
On Wed, 6 Dec 2023 at 12:59, Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Dec 06, 2023 at 12:15:50PM +0530, Shubham Khanna wrote: > > Patch is not applying. Please share the Rebased Version. Please find the error: > > Thanks. Here you go with a v6. Few comments: 1) Now that the MSVC build scripts are removed, should we have the reference to "MSVC build scripts" here? ltree.h: ..... /* * LOWER_NODE used to be defined in the Makefile via the compile flags. * However the MSVC build scripts neglected to do the same which resulted in * MSVC builds not using LOWER_NODE. Since then, the MSVC scripts have been * modified to look for -D compile flags in Makefiles, so here, in order to * get the historic behavior of LOWER_NODE not being defined on MSVC, we only * define it when not building in that environment. This is important as we * want to maintain the same LOWER_NODE behavior after a pg_upgrade. */ #ifndef _MSC_VER #define LOWER_NODE #endif ..... 2) I had seen that if sed/gzip is not available meson build will fail: 2.a) Program gsed sed found: NO meson.build:334:6: ERROR: Program 'gsed sed' not found or not executable 2.b) Program gzip found: NO meson.build:337:7: ERROR: Program 'gzip' not found or not executable Should we mention sed and gzip here? + <varlistentry> + <term><productname>Bison</productname> and + <productname>Flex</productname></term> + <listitem> + <para> + <productname>Bison</productname> and <productname>Flex</productname> are + required. Only <productname>Bison</productname> versions 2.3 and later + will work. <productname>Flex</productname> must be version 2.5.35 or later. + </para> Regards, Vignesh
On 18.12.23 11:49, vignesh C wrote: > Few comments: > 1) Now that the MSVC build scripts are removed, should we have the > reference to "MSVC build scripts" here? > ltree.h: I think this note is correct and can be kept, as it explains the historical context. > 2) I had seen that if sed/gzip is not available meson build will fail: > 2.a) > Program gsed sed found: NO > meson.build:334:6: ERROR: Program 'gsed sed' not found or not executable Yes, this would need to be improved. Currently, sed is only required if either selinux or dtrace is enabled, which isn't supported on Windows. But we should adjust the build scripts to not fail the top-level setup run unless those options are enabled. > 2.b) > Program gzip found: NO > meson.build:337:7: ERROR: Program 'gzip' not found or not executable gzip is only required for certain test suites, so again we should adjust the build scripts to not fail the build but instead skip the tests as appropriate.
On 18.12.23 14:52, Peter Eisentraut wrote: >> 2) I had seen that if sed/gzip is not available meson build will fail: >> 2.a) >> Program gsed sed found: NO >> meson.build:334:6: ERROR: Program 'gsed sed' not found or not executable > > Yes, this would need to be improved. Currently, sed is only required if > either selinux or dtrace is enabled, which isn't supported on Windows. > But we should adjust the build scripts to not fail the top-level setup > run unless those options are enabled. > >> 2.b) >> Program gzip found: NO >> meson.build:337:7: ERROR: Program 'gzip' not found or not executable > > gzip is only required for certain test suites, so again we should adjust > the build scripts to not fail the build but instead skip the tests as > appropriate. Here are patches for these two issues. More testing would be appreciated.
Вложения
On Tue Dec 19, 2023 at 9:24 AM CST, Peter Eisentraut wrote: > On 18.12.23 14:52, Peter Eisentraut wrote: > >> 2) I had seen that if sed/gzip is not available meson build will fail: > >> 2.a) > >> Program gsed sed found: NO > >> meson.build:334:6: ERROR: Program 'gsed sed' not found or not executable > > > > Yes, this would need to be improved. Currently, sed is only required if > > either selinux or dtrace is enabled, which isn't supported on Windows. > > But we should adjust the build scripts to not fail the top-level setup > > run unless those options are enabled. > > > >> 2.b) > >> Program gzip found: NO > >> meson.build:337:7: ERROR: Program 'gzip' not found or not executable > > > > gzip is only required for certain test suites, so again we should adjust > > the build scripts to not fail the build but instead skip the tests as > > appropriate. > > Here are patches for these two issues. More testing would be appreciated. Meson looks good to me! -- Tristan Partin Neon (https://neon.tech)
Hi,
On Tue, 19 Dec 2023 at 18:24, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 18.12.23 14:52, Peter Eisentraut wrote:
> >> 2) I had seen that if sed/gzip is not available meson build will fail:
> >> 2.a)
> >> Program gsed sed found: NO
> >> meson.build:334:6: ERROR: Program 'gsed sed' not found or not executable
> >
> > Yes, this would need to be improved. Currently, sed is only required if
> > either selinux or dtrace is enabled, which isn't supported on Windows.
> > But we should adjust the build scripts to not fail the top-level setup
> > run unless those options are enabled.
> >
> >> 2.b)
> >> Program gzip found: NO
> >> meson.build:337:7: ERROR: Program 'gzip' not found or not executable
> >
> > gzip is only required for certain test suites, so again we should adjust
> > the build scripts to not fail the build but instead skip the tests as
> > appropriate.
>
> Here are patches for these two issues. More testing would be appreciated.
0001-meson-Require-sed-only-when-needed:
+sed = find_program(get_option('SED'), 'sed', native: true,
+ required: get_option('dtrace').enabled() or
get_option('selinux').enabled())
dtrace is disabled as default but selinux is set to auto. So, meson
could find selinux ( because of the auto ) and fail to find sed, then
compilation will fail with:
contrib/sepgsql/meson.build:34:19: ERROR: Tried to use not-found
external program in "command"
I think we need to require sed when dtrace or selinux is found, not by
looking at the return value of the get_option().enabled().
Second patch looks good to me.
--
Regards,
Nazir Bilal Yavuz
Microsoft
On Mon, Dec 18, 2023 at 02:52:41PM +0100, Peter Eisentraut wrote: > On 18.12.23 11:49, vignesh C wrote: >> Few comments: >> 1) Now that the MSVC build scripts are removed, should we have the >> reference to "MSVC build scripts" here? >> ltree.h: > > I think this note is correct and can be kept, as it explains the historical > context. Yeah, that's something I was pondering about for a bit a few weeks ago but keeping the historical context is still the most important piece to me. >> 2) I had seen that if sed/gzip is not available meson build will fail: >> 2.a) >> Program gsed sed found: NO >> meson.build:334:6: ERROR: Program 'gsed sed' not found or not executable > > Yes, this would need to be improved. Currently, sed is only required if > either selinux or dtrace is enabled, which isn't supported on Windows. But > we should adjust the build scripts to not fail the top-level setup run > unless those options are enabled. > >> 2.b) >> Program gzip found: NO >> meson.build:337:7: ERROR: Program 'gzip' not found or not executable > > gzip is only required for certain test suites, so again we should adjust the > build scripts to not fail the build but instead skip the tests as > appropriate. Oops. -- Michael
Вложения
On Mon, Nov 20, 2023 at 05:03:28PM +0900, Michael Paquier wrote: > Your suggestion to create a new sect2 for "Windows" as much as Andres' > suggestion are OK by as an intermediate step, and I suspect that the > end result will likely not be that. It took me some time to get back to this one, and just applied the patch removing the scripts. At the end, I have gone with the addition of a subsection named "Visual" for now in the platform-specific notes, keeping all the information originally in install-windows.sgml the same. A proposal of patch to clean up the docs is on my TODO list for the next CF. -- Michael
Вложения
On Tue, Dec 19, 2023 at 04:24:02PM +0100, Peter Eisentraut wrote:
> Here are patches for these two issues. More testing would be appreciated.
>
> --- a/contrib/basebackup_to_shell/meson.build
> +++ b/contrib/basebackup_to_shell/meson.build
> @@ -24,7 +24,7 @@ tests += {
> 'tests': [
> 't/001_basic.pl',
> ],
> - 'env': {'GZIP_PROGRAM': gzip.path(),
> - 'TAR': tar.path()},
> + 'env': {'GZIP_PROGRAM': gzip.found() ? gzip.path() : '',
> + 'TAR': tar.found() ? tar.path() : '' },
> },
Hmm. Interesting. So this basically comes down to the fact that GZIP
and TAR are required in ./configure because distcheck has a hard
dependency on both, but we don't support this target in meson. Is
that right?
--
Michael
Вложения
On 19.12.23 17:44, Nazir Bilal Yavuz wrote:
> I think we need to require sed when dtrace or selinux is found, not by
> looking at the return value of the get_option().enabled().
Right. I think the correct condition would be
sed = find_program(get_option('SED'), 'sed', native: true,
required: dtrace.found() or selinux.found())
I was trying to avoid that because it would require moving the
find_program() to somewhere later in the top-level meson.build, but I
suppose we have to do it that way.
On 20.12.23 02:14, Michael Paquier wrote: > Hmm. Interesting. So this basically comes down to the fact that GZIP > and TAR are required in ./configure because distcheck has a hard > dependency on both, but we don't support this target in meson. Is > that right? No, the issue is that gzip and tar are not required by configure (it will proceed if they are not found), but they are currently required by meson.build (it will error if they are not found). They are used in two different areas. One is for "make dist", but that doesn't affect meson anyway. The other is various test suites. The test suites are already set up to skip tests when gzip and tar are not found.
On 2023-12-20 09:48:37 +0900, Michael Paquier wrote: > On Mon, Nov 20, 2023 at 05:03:28PM +0900, Michael Paquier wrote: > > Your suggestion to create a new sect2 for "Windows" as much as Andres' > > suggestion are OK by as an intermediate step, and I suspect that the > > end result will likely not be that. > > It took me some time to get back to this one, and just applied the > patch removing the scripts. Wohooo!
On Tue, 19 Dec 2023 at 20:54, Peter Eisentraut <peter@eisentraut.org> wrote: > > On 18.12.23 14:52, Peter Eisentraut wrote: > >> 2) I had seen that if sed/gzip is not available meson build will fail: > >> 2.a) > >> Program gsed sed found: NO > >> meson.build:334:6: ERROR: Program 'gsed sed' not found or not executable > > > > Yes, this would need to be improved. Currently, sed is only required if > > either selinux or dtrace is enabled, which isn't supported on Windows. > > But we should adjust the build scripts to not fail the top-level setup > > run unless those options are enabled. > > > >> 2.b) > >> Program gzip found: NO > >> meson.build:337:7: ERROR: Program 'gzip' not found or not executable > > > > gzip is only required for certain test suites, so again we should adjust > > the build scripts to not fail the build but instead skip the tests as > > appropriate. > > Here are patches for these two issues. More testing would be appreciated. Thanks for the patches, Windows build is successful without these binaries. In linux when I try with Dtrace enabled, it throws the following error: Compiler for C supports arguments -fPIC: YES Compiler for C supports link arguments -Wl,--as-needed: YES Configuring pg_config_paths.h using configuration src/include/utils/meson.build:39:2: ERROR: Tried to use not-found external program in "command" With Dtrace enabled we should throw the original error that we were getting i.e.: ERROR: Program sed not found or not executable Another observation is that we could include the executable name in this case something like: ERROR: Tried to use not-found external program "sed" in "command" Regards, Vignesh
On 2023-12-20 16:15:55 +0530, vignesh C wrote: > On Tue, 19 Dec 2023 at 20:54, Peter Eisentraut <peter@eisentraut.org> wrote: > > > > On 18.12.23 14:52, Peter Eisentraut wrote: > > >> 2) I had seen that if sed/gzip is not available meson build will fail: > > >> 2.a) > > >> Program gsed sed found: NO > > >> meson.build:334:6: ERROR: Program 'gsed sed' not found or not executable > > > > > > Yes, this would need to be improved. Currently, sed is only required if > > > either selinux or dtrace is enabled, which isn't supported on Windows. > > > But we should adjust the build scripts to not fail the top-level setup > > > run unless those options are enabled. > > > > > >> 2.b) > > >> Program gzip found: NO > > >> meson.build:337:7: ERROR: Program 'gzip' not found or not executable > > > > > > gzip is only required for certain test suites, so again we should adjust > > > the build scripts to not fail the build but instead skip the tests as > > > appropriate. > > > > Here are patches for these two issues. More testing would be appreciated. > > Thanks for the patches, Windows build is successful without these binaries. > In linux when I try with Dtrace enabled, it throws the following error: > Compiler for C supports arguments -fPIC: YES > Compiler for C supports link arguments -Wl,--as-needed: YES > Configuring pg_config_paths.h using configuration > > src/include/utils/meson.build:39:2: ERROR: Tried to use not-found > external program in "command" > > With Dtrace enabled we should throw the original error that we were > getting i.e.: > ERROR: Program sed not found or not executable I think the problem is that the current formulation in the patches doesn't with deal with dtrace=auto. I think we ought to make that in a proper feature checking block instead of just checking the presence of the dtrace binary. Hm, or perhaps we should just get rid of sed use altogether. The sepgsql case is trivially translateable to perl, and postprocess_dtrace.sed isn't much harder. OTOH, I actually don't think it's valid to not have sed when you have dtrace. Erroring out in a weird way in such an artificially constructed test doesn't really seem like a problem. > Another observation is that we could include the executable name in > this case something like: > ERROR: Tried to use not-found external program "sed" in "command" It's a meson generated message, so you'd need to open a bug report / feature request for it. Greetings, Andres Freund
> On 20 Dec 2023, at 01:48, Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Nov 20, 2023 at 05:03:28PM +0900, Michael Paquier wrote: >> Your suggestion to create a new sect2 for "Windows" as much as Andres' >> suggestion are OK by as an intermediate step, and I suspect that the >> end result will likely not be that. > > It took me some time to get back to this one, and just applied the > patch removing the scripts. The Buildfarm complains that Win32::Registry can't be found: Can't locate Win32/Registry.pm in @INC (you may need to install the Win32::Registry module) (@INC entries checked: src/test/perlsrc/tools/msvc src/backend/catalog src/backend/utils/mb/Unicode src/bin/pg_rewind src/test/ssl/t src/tools/msvc/dummylib/usr/local/lib64/perl5/5.38 /usr/local/share/perl5/5.38 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl/usr/lib64/perl5 /usr/share/perl5) at ./src/tools/win32tzlist.pl line 21. BEGIN failed--compilation aborted at ./src/tools/win32tzlist.pl line 21. https://brekka.postgresql.org/cgi-bin/show_log.pl?nm=koel&dt=2023-12-20%2013%3A19%3A04 This could perhaps be related to this patch removing the module in src/tools/msvc/dummylib/Win32/Registry.pm ? -- Daniel Gustafsson
On 20.12.23 12:40, Andres Freund wrote: > Hm, or perhaps we should just get rid of sed use altogether. The sepgsql case > is trivially translateable to perl, and postprocess_dtrace.sed isn't > much harder. Maybe yeah, but also it seems fine as is and we can easily fix the present issue ... > OTOH, I actually don't think it's valid to not have sed when you have > dtrace. Erroring out in a weird way in such an artificially constructed test > doesn't really seem like a problem. Agreed. So let's just make it not-required, and that should work. Updated patch set attached.
Вложения
On 2023-12-20 We 08:31, Daniel Gustafsson wrote: >> On 20 Dec 2023, at 01:48, Michael Paquier <michael@paquier.xyz> wrote: >> >> On Mon, Nov 20, 2023 at 05:03:28PM +0900, Michael Paquier wrote: >>> Your suggestion to create a new sect2 for "Windows" as much as Andres' >>> suggestion are OK by as an intermediate step, and I suspect that the >>> end result will likely not be that. >> It took me some time to get back to this one, and just applied the >> patch removing the scripts. > The Buildfarm complains that Win32::Registry can't be found: > > Can't locate Win32/Registry.pm in @INC (you may need to install the Win32::Registry module) (@INC entries checked: src/test/perlsrc/tools/msvc src/backend/catalog src/backend/utils/mb/Unicode src/bin/pg_rewind src/test/ssl/t src/tools/msvc/dummylib/usr/local/lib64/perl5/5.38 /usr/local/share/perl5/5.38 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl/usr/lib64/perl5 /usr/share/perl5) at ./src/tools/win32tzlist.pl line 21. > BEGIN failed--compilation aborted at ./src/tools/win32tzlist.pl line 21. > > https://brekka.postgresql.org/cgi-bin/show_log.pl?nm=koel&dt=2023-12-20%2013%3A19%3A04 > > This could perhaps be related to this patch removing the module in > src/tools/msvc/dummylib/Win32/Registry.pm ? > It is. I've fixed the buildfarm to stop checking this script. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Wed, Dec 20, 2023 at 11:03 AM Andrew Dunstan <andrew@dunslane.net> wrote: > > This could perhaps be related to this patch removing the module in > > src/tools/msvc/dummylib/Win32/Registry.pm ? > > It is. I've fixed the buildfarm to stop checking this script. Thanks! But I wonder whether the script itself also needs to be changed? Are we expecting that the 'use Win32::Registry' in win32tzlist.pl would be satisfied externally in some case? -- Robert Haas EDB: http://www.enterprisedb.com
On 2023-12-20 We 11:32, Robert Haas wrote: > On Wed, Dec 20, 2023 at 11:03 AM Andrew Dunstan <andrew@dunslane.net> wrote: >>> This could perhaps be related to this patch removing the module in >>> src/tools/msvc/dummylib/Win32/Registry.pm ? >> It is. I've fixed the buildfarm to stop checking this script. > Thanks! But I wonder whether the script itself also needs to be > changed? Are we expecting that the 'use Win32::Registry' in > win32tzlist.pl would be satisfied externally in some case? > Yes, the module will normally be present on a Windows perl. The only reason we had dummylib was so we could check the perl scripts on Unix. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
> On 20 Dec 2023, at 18:22, Andrew Dunstan <andrew@dunslane.net> wrote: > On 2023-12-20 We 11:32, Robert Haas wrote: >> On Wed, Dec 20, 2023 at 11:03 AM Andrew Dunstan <andrew@dunslane.net> wrote: >>>> This could perhaps be related to this patch removing the module in >>>> src/tools/msvc/dummylib/Win32/Registry.pm ? >>> It is. I've fixed the buildfarm to stop checking this script. >> Thanks! But I wonder whether the script itself also needs to be >> changed? Are we expecting that the 'use Win32::Registry' in >> win32tzlist.pl would be satisfied externally in some case? > > Yes, the module will normally be present on a Windows perl. The only reason we had dummylib was so we could check the perlscripts on Unix. Thanks for taking care of it! -- Daniel Gustafsson
On Wed, Dec 20, 2023 at 07:00:50PM +0100, Daniel Gustafsson wrote: > On 20 Dec 2023, at 18:22, Andrew Dunstan <andrew@dunslane.net> wrote: >> Yes, the module will normally be present on a Windows perl. The >> only reason we had dummylib was so we could check the perl scripts on >> Unix. > > Thanks for taking care of it! Thanks! -- Michael
Вложения
On Wed, 20 Dec 2023 at 21:13, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 20.12.23 12:40, Andres Freund wrote:
> > Hm, or perhaps we should just get rid of sed use altogether. The sepgsql case
> > is trivially translateable to perl, and postprocess_dtrace.sed isn't
> > much harder.
>
> Maybe yeah, but also it seems fine as is and we can easily fix the
> present issue ...
>
> > OTOH, I actually don't think it's valid to not have sed when you have
> > dtrace. Erroring out in a weird way in such an artificially constructed test
> > doesn't really seem like a problem.
>
> Agreed. So let's just make it not-required, and that should work.
>
> Updated patch set attached.
Thanks for the patches.
I noticed one issue about the flex 2.5.35 version mentioned.
I noticed some warning with meson build in windows with flex 2.5.35
for several files:
python = find_program(get_option('PYTHON'), required: true, native: true)
flex = find_program(get_option('FLEX'), native: true, version: '>= 2.5.35')
bison = find_program(get_option('BISON'), native: true, version: '>= 2.3')
-sed = find_program(get_option('SED'), 'sed', native: true)
+sed = find_program(get_option('SED'), 'sed', native: true, required: false)
prove = find_program(get_option('PROVE'), native: true, required: false)
tar = find_program(get_option('TAR'), native: true)
Compiling C object
src/test/isolation/isolationtester.exe.p/meson-generated_.._specscanner.c.obj
src/test/isolation/specscanner.c(2): warning C4129: 'W': unrecognized
character escape sequence
src/test/isolation/specscanner.c(2): warning C4129: 'P': unrecognized
character escape sequence
src/test/isolation/specscanner.c(2): warning C4129: 'p': unrecognized
character escape sequence
src/test/isolation/specscanner.c(2): warning C4129: 's': unrecognized
character escape sequence
src/test/isolation/specscanner.c(2): warning C4129: 'i': unrecognized
character escape sequence
I noticed this is because the lex file getting added without escape
characters in the C file:
#line 2 "D:\postgres\pg_meson\src\backend\utils\adt\jsonpath_scan.l"
There were no warnings when I used flex 2.6.4.
Did anyone else get these warnings with the flex 2.5.35 version?
Regards,
Vignesh
On 20.12.23 18:22, Andrew Dunstan wrote:
>
> On 2023-12-20 We 11:32, Robert Haas wrote:
>> On Wed, Dec 20, 2023 at 11:03 AM Andrew Dunstan <andrew@dunslane.net>
>> wrote:
>>>> This could perhaps be related to this patch removing the module in
>>>> src/tools/msvc/dummylib/Win32/Registry.pm ?
>>> It is. I've fixed the buildfarm to stop checking this script.
>> Thanks! But I wonder whether the script itself also needs to be
>> changed? Are we expecting that the 'use Win32::Registry' in
>> win32tzlist.pl would be satisfied externally in some case?
>>
>
> Yes, the module will normally be present on a Windows perl. The only
> reason we had dummylib was so we could check the perl scripts on Unix.
But this use case still exists. Right now, running
./src/tools/perlcheck/pgperlsyncheck .
fails because this module is missing. So I think we need to put the
dummy module back somehow.
On 21.12.23 07:35, vignesh C wrote: > I noticed this is because the lex file getting added without escape > characters in the C file: > #line 2 "D:\postgres\pg_meson\src\backend\utils\adt\jsonpath_scan.l" > > There were no warnings when I used flex 2.6.4. > > Did anyone else get these warnings with the flex 2.5.35 version? It appears that this is an issue related to building in a separate build directory, not something specific to meson. The solution would be to use an appropriately new flex, as you have done.
Hi, On 2023-12-21 08:31:57 +0100, Peter Eisentraut wrote: > On 20.12.23 18:22, Andrew Dunstan wrote: > > > > On 2023-12-20 We 11:32, Robert Haas wrote: > > > On Wed, Dec 20, 2023 at 11:03 AM Andrew Dunstan > > > <andrew@dunslane.net> wrote: > > > > > This could perhaps be related to this patch removing the module in > > > > > src/tools/msvc/dummylib/Win32/Registry.pm ? > > > > It is. I've fixed the buildfarm to stop checking this script. > > > Thanks! But I wonder whether the script itself also needs to be > > > changed? Are we expecting that the 'use Win32::Registry' in > > > win32tzlist.pl would be satisfied externally in some case? > > > > > > > Yes, the module will normally be present on a Windows perl. The only > > reason we had dummylib was so we could check the perl scripts on Unix. > > But this use case still exists. Right now, running > > ./src/tools/perlcheck/pgperlsyncheck . > > fails because this module is missing. So I think we need to put the dummy > module back somehow. Can't we teach the tool that it should not validate src/tools/win32tzlist.pl on !windows? It's obviously windows specific code, and it's special case enough that there doesn't seem like a need to develop it on !windows. Greetings, Andres Freund
On Wed, Dec 20, 2023 at 11:39:15PM -0800, Andres Freund wrote: > Can't we teach the tool that it should not validate src/tools/win32tzlist.pl > on !windows? It's obviously windows specific code, and it's special case > enough that there doesn't seem like a need to develop it on !windows. I am not really excited about keeping a dummy library for the sake of a script checking if this WIN32-only file is correctly written, and I've never used pgperlsyncheck, TBH, since it exists in af616ce48347. Anyway, we could just tweak the list of files returned by find_perl_files as win32tzlist.pl is valid for perltidy and perlcritic. Andrew, was the original target of pgperlsyncheck committers and hackers who played with the MSVC scripts but could not run sanity checks on Windows (see [1])? There are a few more cases like the Unicode scripts or some of the stuff in src/tools/ where that can be useful still these are not touched on a daily basis. The rest of the pm files are for TAP tests, one for Unicode. I'm OK to tweak the script, still, if its main purpose is gone.. [1]: https://www.postgresql.org/message-id/f3c12e2c-618f-cb6f-082b-a2f604dbe73f@2ndQuadrant.com -- Michael
Вложения
On 20.12.23 16:43, Peter Eisentraut wrote: > On 20.12.23 12:40, Andres Freund wrote: >> Hm, or perhaps we should just get rid of sed use altogether. The >> sepgsql case >> is trivially translateable to perl, and postprocess_dtrace.sed isn't >> much harder. > > Maybe yeah, but also it seems fine as is and we can easily fix the > present issue ... > >> OTOH, I actually don't think it's valid to not have sed when you have >> dtrace. Erroring out in a weird way in such an artificially >> constructed test >> doesn't really seem like a problem. > > Agreed. So let's just make it not-required, and that should work. > > Updated patch set attached. I have committed these two.
On 2023-12-21 Th 03:01, Michael Paquier wrote: > On Wed, Dec 20, 2023 at 11:39:15PM -0800, Andres Freund wrote: >> Can't we teach the tool that it should not validate src/tools/win32tzlist.pl >> on !windows? It's obviously windows specific code, and it's special case >> enough that there doesn't seem like a need to develop it on !windows. > I am not really excited about keeping a dummy library for the sake of > a script checking if this WIN32-only file is correctly written, and > I've never used pgperlsyncheck, TBH, since it exists in af616ce48347. > Anyway, we could just tweak the list of files returned by > find_perl_files as win32tzlist.pl is valid for perltidy and > perlcritic. > > Andrew, was the original target of pgperlsyncheck committers and > hackers who played with the MSVC scripts but could not run sanity > checks on Windows (see [1])? yes. > There are a few more cases like the > Unicode scripts or some of the stuff in src/tools/ where that can be > useful still these are not touched on a daily basis. The rest of the > pm files are for TAP tests, one for Unicode. I'm OK to tweak the > script, still, if its main purpose is gone.. > > [1]: https://www.postgresql.org/message-id/f3c12e2c-618f-cb6f-082b-a2f604dbe73f@2ndQuadrant.com I'm actually a bit dubious about win32tzlist.pl. Win32::Registry is not present in a recent Strawberry Perl installation, and its latest version says it is obsolete, although it's still included in the cpan bundle libwin32. I wonder who has actually run the script any time recently? In any case, we can probably work around the syncheck issue by making the module a runtime requirement rather than a compile time requirement, by using "require" instead of "use". cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Thu, Dec 21, 2023 at 03:43:32PM -0500, Andrew Dunstan wrote:
> On 2023-12-21 Th 03:01, Michael Paquier wrote:
>> Andrew, was the original target of pgperlsyncheck committers and
>> hackers who played with the MSVC scripts but could not run sanity
>> checks on Windows (see [1])?
>
>
> yes.
Okay, thanks. Wouldn't it be better to remove it at the end? With
the main use case behind its introduction being gone, it is less
attractive to keep maintaining it. If some people have been using it
in their workflows, I'm OK to keep it but the rest of the tree can be
checked at runtime as well.
> I'm actually a bit dubious about win32tzlist.pl. Win32::Registry is not
> present in a recent Strawberry Perl installation, and its latest version
> says it is obsolete, although it's still included in the cpan bundle
> libwin32.
>
> I wonder who has actually run the script any time recently?
Hmm... I've never run it with meson on Win32.
> In any case, we can probably work around the syncheck issue by making the
> module a runtime requirement rather than a compile time requirement, by
> using "require" instead of "use".
Interesting. Another trick would be needed for HKEY_LOCAL_MACHINE,
like what the dummylib but local to win32tzlist.pl. Roughly among
these lines:
-use Win32::Registry;
+use Config;
+
+require Win32::Registry;
my $tzfile = 'src/bin/initdb/findtimezone.c';
+if ($Config{osname} ne 'MSWin32' && $Config{osname} ne 'msys')
+{
+ use vars qw($HKEY_LOCAL_MACHINE);
+}
--
Michael
Вложения
On 2023-12-21 Th 18:20, Michael Paquier wrote:
> On Thu, Dec 21, 2023 at 03:43:32PM -0500, Andrew Dunstan wrote:
>> On 2023-12-21 Th 03:01, Michael Paquier wrote:
>>> Andrew, was the original target of pgperlsyncheck committers and
>>> hackers who played with the MSVC scripts but could not run sanity
>>> checks on Windows (see [1])?
>>
>> yes.
> Okay, thanks. Wouldn't it be better to remove it at the end? With
> the main use case behind its introduction being gone, it is less
> attractive to keep maintaining it. If some people have been using it
> in their workflows, I'm OK to keep it but the rest of the tree can be
> checked at runtime as well.
>
>> I'm actually a bit dubious about win32tzlist.pl. Win32::Registry is not
>> present in a recent Strawberry Perl installation, and its latest version
>> says it is obsolete, although it's still included in the cpan bundle
>> libwin32.
>>
>> I wonder who has actually run the script any time recently?
> Hmm... I've never run it with meson on Win32.
Turns out I was wrong - Windows sometimes doesn't find files nicely. It
is present in my Strawberry installation.
>
>> In any case, we can probably work around the syncheck issue by making the
>> module a runtime requirement rather than a compile time requirement, by
>> using "require" instead of "use".
> Interesting. Another trick would be needed for HKEY_LOCAL_MACHINE,
> like what the dummylib but local to win32tzlist.pl. Roughly among
> these lines:
> -use Win32::Registry;
> +use Config;
> +
> +require Win32::Registry;
>
> my $tzfile = 'src/bin/initdb/findtimezone.c';
>
> +if ($Config{osname} ne 'MSWin32' && $Config{osname} ne 'msys')
> +{
> + use vars qw($HKEY_LOCAL_MACHINE);
> +}
I've done it a bit differently, but the same idea. I have tested that
what I committed passes checks on Unix and works on Windows.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Fri, Dec 22, 2023 at 09:07:21AM -0500, Andrew Dunstan wrote: > I've done it a bit differently, but the same idea. I have tested that what I > committed passes checks on Unix and works on Windows. Sounds fine by me. Thanks for the quick turnaround! -- Michael