Horton

Thread

Commits

  1. Remove support for Visual Studio 2013

  1. Remove support for Visual Studio 2013

    Michael Paquier <michael@paquier.xyz> — 2022-05-16T06:58:40Z

    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

  2. Re: Remove support for Visual Studio 2013

    Thomas Munro <thomas.munro@gmail.com> — 2022-05-16T08:46:31Z

    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

  3. Re: Remove support for Visual Studio 2013

    Michael Paquier <michael@paquier.xyz> — 2022-05-16T10:34:31Z

    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

  4. Re: Remove support for Visual Studio 2013

    Andrew Dunstan <andrew@dunslane.net> — 2022-05-16T12:19:28Z

    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

  5. Re: Remove support for Visual Studio 2013

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-16T15:23:15Z

    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

  6. Re: Remove support for Visual Studio 2013

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2022-05-17T16:26:20Z

    On Mon, May 16, 2022 at 8:58 AM Michael Paquier <michael@paquier.xyz> wrote:

>
> 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.
>
> When building on MinGW with NLS enabled I get some errors:

c:/cirrus/src/backend/utils/adt/pg_locale.c: In function
'search_locale_enum':
c:/cirrus/src/backend/utils/adt/pg_locale.c:989:13: warning: implicit
declaration of function 'GetLocaleInfoEx'; did you mean 'GetLocaleInfoA'?
[-Wimplicit-function-declaration]
  989 |         if (GetLocaleInfoEx(pStr, LOCALE_SENGLISHLANGUAGENAME,
      |             ^~~~~~~~~~~~~~~
      |             GetLocaleInfoA

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

Regards,

Juan José Santamaría Flecha

  7. Re: Remove support for Visual Studio 2013

    Michael Paquier <michael@paquier.xyz> — 2022-05-18T00:27:03Z

    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

  8. Re: Remove support for Visual Studio 2013

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2022-05-18T08:06:50Z

    On Wed, May 18, 2022 at 2:27 AM Michael Paquier <michael@paquier.xyz> wrote:

>
> @@ -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?


Right now we are ifdefing that code out for MinGW, so it's not a visible
issue, but it'll be when we do.


> 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

Regards,

Juan José Santamaría Flecha

>
>

  9. Re: Remove support for Visual Studio 2013

    Michael Paquier <michael@paquier.xyz> — 2022-05-18T09:02:00Z

    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

  10. Re: Remove support for Visual Studio 2013

    Michael Paquier <michael@paquier.xyz> — 2022-05-26T01:18:42Z

    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

  11. Re: Remove support for Visual Studio 2013

    Justin Pryzby <pryzby@telsasoft.com> — 2022-05-26T15:43:11Z

    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()

  12. Re: Remove support for Visual Studio 2013

    Michael Paquier <michael@paquier.xyz> — 2022-05-26T20:57:39Z

    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

  13. Re: Remove support for Visual Studio 2013

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-26T21:50:40Z

    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

  14. Re: Remove support for Visual Studio 2013

    Michael Paquier <michael@paquier.xyz> — 2022-05-30T07:48:22Z

    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

  15. Re: Remove support for Visual Studio 2013

    Michael Paquier <michael@paquier.xyz> — 2022-07-07T22:38:23Z

    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

  16. Re: Remove support for Visual Studio 2013

    Michael Paquier <michael@paquier.xyz> — 2022-07-14T03:03:02Z

    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