Re: Remove MSVC scripts from the tree

On Fri, Nov 17, 2023 at 6:31 AM Michael Paquier <michael(at)paquier(dot)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.