Feedback-ID: ie0a040ee:Fastmail
Message-ID: <d8c7c426-ca50-86be-ce74-d0a8110c40fe@eisentraut.org>
Date: Thu, 5 Oct 2023 09:38:51 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.15.1
Subject: Re: Remove MSVC scripts from the tree
Content-Language: en-US
To: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
References: <ZQzp_VMJcerM1Cs_@paquier.xyz>
 <a08e14cf-23c0-b4b6-37f5-dec671e85e7f@eisentraut.org>
 <ZRpzY4Yz9FcW1ZKV@paquier.xyz>
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <ZRpzY4Yz9FcW1ZKV@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/d8c7c426-ca50-86be-ce74-d0a8110c40fe%40eisentraut.org>
Precedence: bulk

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.