Re: Remove MSVC scripts from the tree - Mailing list pgsql-hackers

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