Обсуждение: Remove support for Visual Studio 2013

Hi all,

Cutting support for now-unsupported versions of Windows is in the air
for a couple of months, and while looking at the code a first cleanup
that looked rather obvious to me is the removal of support for VS
2013, as of something to do for v16~.

The website of Microsoft has only documentation for VS >= 2015 as far
as I can see.  Note also that VS can be downloaded down to 2012 on
their official website, and that the buildfarm members only use VS >=
2017.

The patch attached cleans up the following things proper to VS 2013:
- Locale handling.
- MIN_WINNT assignment.
- Some strtof() business, as of win32_port.h.
- Removal of _set_FMA3_enable() in main.c related to floating-point
operations.
- MSVC scripts, but that's less interesting considering the work done
with meson.

A nice result is that this completely removes all the checks related
to the version number of _MSC_VER from the core code, making the code
depend only on the definition if the flag.

Thanks,
--
Michael

On Mon, May 16, 2022 at 6:58 PM Michael Paquier <michael@paquier.xyz> wrote:
> Cutting support for now-unsupported versions of Windows is in the air
> for a couple of months, and while looking at the code a first cleanup
> that looked rather obvious to me is the removal of support for VS
> 2013, as of something to do for v16~.

Not a Windows person so I couldn't comment on the details without more
research, but this general concept seems good to me.  That's a nice
reduction in (practically) untestable/dead code (no CI, no build
farm).

For comparison, I picked 3 random C/C++ (OK, C++) projects I could
think of to see how they deal with VS versions, and all require 2017+:

* MariaDB supports the last two major versions, so currently VS 2019
and VS 2022, 2022 is preferred[1]
* Chrome requires VS 2017+ but currently 2019 is preferred[2]
* OpenJDK requires VS 2017+[3]

Looking at the published lifecycle info, 2017 is the oldest still in
'mainstream' support[4], so it wouldn't be too crazy to drop VS 2015
too, just like those other projects.  That said, it sounds like there
is no practical benefit to being more aggressive than you are
suggesting currently (as in, we wouldn't get to delete any more crufty
untestable dead code by dropping 2015, right?), so maybe that'd be
enough for now.

[1] https://mariadb.com/kb/en/Building_MariaDB_on_Windows/
[2] https://chromium.googlesource.com/chromium/src/+/main/docs/windows_build_instructions.md#Visual-Studio
[3] https://openjdk.java.net/groups/build/doc/building.html
[4] https://docs.microsoft.com/en-us/visualstudio/productinfo/vs-servicing

On Mon, May 16, 2022 at 08:46:31PM +1200, Thomas Munro wrote:
> Looking at the published lifecycle info, 2017 is the oldest still in
> 'mainstream' support[4], so it wouldn't be too crazy to drop VS 2015
> too, just like those other projects.  That said, it sounds like there
> is no practical benefit to being more aggressive than you are
> suggesting currently (as in, we wouldn't get to delete any more crufty
> untestable dead code by dropping 2015, right?), so maybe that'd be
> enough for now.

FWIW, one of my environments is using VS2015, because I have set it up
years ago and I am lazy to do this setup except if I really have to :)

The code works as far as I know, still I am not really excited about
cutting support for more versions than necessary, particularly as this
does not simplify the C code more.
--
Michael

On 2022-05-16 Mo 06:34, Michael Paquier wrote:
> On Mon, May 16, 2022 at 08:46:31PM +1200, Thomas Munro wrote:
>> Looking at the published lifecycle info, 2017 is the oldest still in
>> 'mainstream' support[4], so it wouldn't be too crazy to drop VS 2015
>> too, just like those other projects.  That said, it sounds like there
>> is no practical benefit to being more aggressive than you are
>> suggesting currently (as in, we wouldn't get to delete any more crufty
>> untestable dead code by dropping 2015, right?), so maybe that'd be
>> enough for now.
> FWIW, one of my environments is using VS2015, because I have set it up
> years ago and I am lazy to do this setup except if I really have to :)
>
> The code works as far as I know, still I am not really excited about
> cutting support for more versions than necessary, particularly as this
> does not simplify the C code more.


Yeah, I'm ok with this. The only older version I have is currawong, but
it runs on NT and so only builds release 10 and will probably be retired
around the end of the year.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, May 16, 2022 at 08:46:31PM +1200, Thomas Munro wrote:
>> Looking at the published lifecycle info, 2017 is the oldest still in
>> 'mainstream' support[4], so it wouldn't be too crazy to drop VS 2015
>> too, just like those other projects.  That said, it sounds like there
>> is no practical benefit to being more aggressive than you are
>> suggesting currently (as in, we wouldn't get to delete any more crufty
>> untestable dead code by dropping 2015, right?), so maybe that'd be
>> enough for now.

> FWIW, one of my environments is using VS2015, because I have set it up
> years ago and I am lazy to do this setup except if I really have to :)

> The code works as far as I know, still I am not really excited about
> cutting support for more versions than necessary, particularly as this
> does not simplify the C code more.

The argument about removing untested code doesn't apply if there is
no code to remove, so it seems like continuing to support VS2015 is
reasonable.  Of course, if anyone came and complained that it's broken,
we'd probably just drop the support claim ...

            regards, tom lane

On Tue, May 17, 2022 at 06:26:20PM +0200, Juan José Santamaría Flecha wrote:
> This is because current MinGW defaults to Windows 2003 [1], maybe we should
> fix Windows' minimal version to Vista (0x0600) unconditionally also. I have
> seen a couple of compilation warnings while testing that setting on MinGW,
> please find attached a patch for so.
>
> [1]
> https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/include/sdkddkver.h

Ah, right.  I have forgotten about this business with MinGW.

@@ -1757,7 +1757,7 @@ get_collation_actual_version(char collprovider, const char *collcollate)
                             collcollate,
                             GetLastError())));
         }
-        collversion = psprintf("%d.%d,%d.%d",
+        collversion = psprintf("%ld.%ld,%ld.%ld",
                                (version.dwNLSVersion >> 8) & 0xFFFF,
                                version.dwNLSVersion & 0xFF,
                                (version.dwDefinedVersion >> 8) & 0xFFFF,

Is this change still required even if we bump MIN_WINNT to 0x0600 for
all the environments that include win32.h?  At the end, this would
mean dropping support for Windows XP and Windows Server 2003 as
run-time environments as listed in [1], which are not supported
officially since 2014 (even if there have been some patches for
some critical issues).  So I'd be fine to raise the bar for v16~,
particularly as this would allow us to get rid of this code related to
locales.

[1]: https://docs.microsoft.com/en-us/cpp/porting/modifying-winver-and-win32-winnt?view=msvc-170
--
Michael

On Wed, May 18, 2022 at 10:06:50AM +0200, Juan José Santamaría Flecha wrote:
> Right now we are ifdefing that code out for MinGW, so it's not a visible
> issue, but it'll be when we do.

OK.  Thanks, got it.
--
Michael

On Wed, May 18, 2022 at 10:06:50AM +0200, Juan José Santamaría Flecha wrote:
> On Wed, May 18, 2022 at 2:27 AM Michael Paquier <michael@paquier.xyz> wrote:
>> At the end, this would
>> mean dropping support for Windows XP and Windows Server 2003 as
>> run-time environments as listed in [1], which are not supported
>> officially since 2014 (even if there have been some patches for
>> some critical issues).  So I'd be fine to raise the bar for v16~,
>> particularly as this would allow us to get rid of this code related to
>> locales.
>
> Even Windows Server 2008 [1] is at its End of Life, so this should surprise
> no one.
>
> [1]
>
https://docs.microsoft.com/en-us/troubleshoot/windows-server/windows-server-eos-faq/end-of-support-windows-server-2008-2008r2

Btw, I am going to spawn a new thread for this specific change rather
than forcing people to dig into this one as it is independent.  Better
to do that a bit in advance of the development cycle for v16, as it
is usually good to clean up such things sooner than later..
--
Michael

Maybe consider removing this workaround?  The original problem report indicated
that it didn't affect later versions:

src/backend/optimizer/path/costsize.c:  /* This apparently-useless variable dodges a compiler bug in VS2013: */

I'm not sure if it's worth removing this one, though:

src/port/strtof.c: * On Windows, there's a slightly different problem: VS2013 has a strtof()

On Thu, May 26, 2022 at 10:43:11AM -0500, Justin Pryzby wrote:

Ah, thanks.  I forgot to grep for those patterns.  Good catches.

> Maybe consider removing this workaround?  The original problem report indicated
> that it didn't affect later versions:
>
> src/backend/optimizer/path/costsize.c:  /* This apparently-useless variable dodges a compiler bug in VS2013: */

Hence reverting 3154e16.  Sure!

> I'm not sure if it's worth removing this one, though:
>
> src/port/strtof.c: * On Windows, there's a slightly different problem: VS2013 has a strtof()

Yeah..  I am not completely sure if all the patterns mentioned for
VS2013 apply to Cygwin/Mingw, so keeping it around could be more
beneficial.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Thu, May 26, 2022 at 10:43:11AM -0500, Justin Pryzby wrote:
>> Maybe consider removing this workaround?  The original problem report indicated
>> that it didn't affect later versions:
>>
>> src/backend/optimizer/path/costsize.c:  /* This apparently-useless variable dodges a compiler bug in VS2013: */

> Hence reverting 3154e16.  Sure!

+1

>> I'm not sure if it's worth removing this one, though:
>>
>> src/port/strtof.c: * On Windows, there's a slightly different problem: VS2013 has a strtof()

> Yeah..  I am not completely sure if all the patterns mentioned for
> VS2013 apply to Cygwin/Mingw, so keeping it around could be more
> beneficial.

The comments about that in win32_port.h and cygwin.h only date back
to 2019, so it seems unlikely that the situation has changed much.
We could try removing HAVE_BUGGY_STRTOF to see if the buildfarm
complains, but I wouldn't bet money on that succeeding.  What we
*do* need to do is update the #if tests and comments to make clear
that HAVE_BUGGY_STRTOF is only needed for Mingw and Cygwin, not
for any supported MSVC release.

            regards, tom lane

On Thu, May 26, 2022 at 05:50:40PM -0400, Tom Lane wrote:
> The comments about that in win32_port.h and cygwin.h only date back
> to 2019, so it seems unlikely that the situation has changed much.
> We could try removing HAVE_BUGGY_STRTOF to see if the buildfarm
> complains, but I wouldn't bet money on that succeeding.  What we
> *do* need to do is update the #if tests and comments to make clear
> that HAVE_BUGGY_STRTOF is only needed for Mingw and Cygwin, not
> for any supported MSVC release.

After looking at that again, the whole comment related to VS in
strtof.c can be removed.  I have noticed while on it more places that
still referred to VS2013 in ./configure[.ac] and win32_langinfo() got
an overall incorrect description.  This leads to v2 as of the
attached.
--
Michael

On Mon, May 30, 2022 at 04:48:22PM +0900, Michael Paquier wrote:
> After looking at that again, the whole comment related to VS in
> strtof.c can be removed.  I have noticed while on it more places that
> still referred to VS2013 in ./configure[.ac] and win32_langinfo() got
> an overall incorrect description.  This leads to v2 as of the
> attached.

And with 495ed0e now in place, attached is a rebased version.
--
Michael

On Fri, Jul 08, 2022 at 07:38:23AM +0900, Michael Paquier wrote:
> And with 495ed0e now in place, attached is a rebased version.

Hearing nothing about this one, and because it is a nice cleanup
overall, I have gone ahead and applied it:
14 files changed, 24 insertions(+), 177 deletions(-)

This removes the dependency on the value of _MSC_VER.
--
Michael