Thread: Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

Hackers,

There was a bug report sent by Hao Lee about Windows build breakage,
"BUG #15167: error C2365: 'errcode' : redefinition; previous definition"
https://www.postgresql.org/message-id/152446498404.19807.4659286570762153837%40wrigleys.postgresql.org

Heikki was the only person who responded to Hao, AFAIK, though that
conversation did not go far.  Perhaps Heikki was unable to reproduce?

Hao did not specify which commit caused the problem, so I started
bisecting to find out.

With my Windows build setup, building from master, the builds break
staring with da9b580d89903fee871cf54845ffa2b26bda2e11 committed by
Stephen on Apr 7 2018.



Build FAILED.

"D:\jenkins\workspace\trunk\dist\msi\BUILD\postgresql\pgsql.sln" (default target) (1) ->
(postgres target) ->
  .\src\backend\replication\basebackup.c(1470): warning C4146: unary minus operator applied to unsigned type, result
stillunsigned 


"D:\jenkins\workspace\trunk\dist\msi\BUILD\postgresql\pgsql.sln" (default target) (1) ->
(postgres target) ->
  .\src\common\file_perm.c(18): error C2065: 'S_IRWXU' : undeclared identifier
  .\src\common\file_perm.c(18): error C2099: initializer is not a constant
  .\src\common\file_perm.c(19): error C2065: 'S_IRUSR' : undeclared identifier
  .\src\common\file_perm.c(19): error C2065: 'S_IWUSR' : undeclared identifier
  .\src\common\file_perm.c(19): error C2099: initializer is not a constant



The build breaks in a different way staring with
c37b3d08ca6873f9d4eaf24c72a90a550970cbb8 committed by Stephen later
that same day.



Microsoft (R) Build Engine Version 3.5.30729.5420
[Microsoft .NET Framework, Version 2.0.50727.8784]
Copyright (C) Microsoft Corporation 2007. All rights reserved.

Build started 5/15/2018 5:24:08 AM.
Project "D:\jenkins\workspace\trunk\dist\msi\BUILD\postgresql\pgsql.sln" on node 0 (default targets).
 Building solution configuration "Release|x64".
<http://aws-pmc-build-w2k8.int.port25.com:8282/job/trunk/ws/dist/msi/build/postgresql/src/include/utils/elog.h(131)>:
errorC2365: 'errcode' : redefinition; previous definition was 'typedef' 
Done Building Project "D:\jenkins\workspace\trunk\dist\msi\BUILD\postgresql\pgsql.sln" (default targets) -- FAILED.

Build FAILED.



Since I am able to reproduce the problem, I'd like to help debug the
problem.  I find it frustrating that the compiler is not specifying
*where* the prior typedef comes from.

My best guess at the moment is:

diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index c1f0441b08..0a3163398f 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -16,8 +16,11 @@
  *
  *-------------------------------------------------------------------------
  */
+#include <sys/stat.h>
+
 #include "postgres.h"

+#include "common/file_perm.h"
 #include "libpq/libpq-be.h"
 #include "libpq/pqcomm.h"
 #include "miscadmin.h"

Which makes me wonder if <sys/stat.h> on Windows declares a typedef for errcode?
The work-around in src/include/port/win32.h which attempts to deal with system
headers defining errcode looks like it won't work unless it gets included *before*
the offending system header, which appears not to be true for globals.c.

Indeed, the following change (shown here for illustrative purposes only; please
don't commit it this way) fixes the problem, at least in my build environment:

diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 9f1209323a..1622b0be62 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -16,7 +16,14 @@
  *
  *-------------------------------------------------------------------------
  */
+
+#if defined(_WIN32) || defined(WIN32) || defined(_MSC_VER) || defined(HAVE_CRTDEFS_H)
+#define errcode __msvc_errcode
+#include <sys/stat.h>
+#undef errcode
+#else
 #include <sys/stat.h>
+#endif

 #include "postgres.h"



Let me know if there are any tests you'd like me to perform to further
investigate.


mark

Mark Dilger <hornschnorter@gmail.com> writes:
> My best guess at the moment is:

> diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
> index c1f0441b08..0a3163398f 100644
> --- a/src/backend/utils/init/globals.c
> +++ b/src/backend/utils/init/globals.c
> @@ -16,8 +16,11 @@
>   *
>   *-------------------------------------------------------------------------
>   */
> +#include <sys/stat.h>
> +
>  #include "postgres.h"

> +#include "common/file_perm.h"

Yipes.  Frost, you didn't really do that did you?  That's a blatant
break of the "c.h must come first" rule.  Whether or not it broke the
Windows build, there are other platforms it'll break.

> Indeed, the following change (shown here for illustrative purposes only; please
> don't commit it this way) fixes the problem, at least in my build environment:

That's pretty ugly, but what happens if you just move the <sys/stat.h>
inclusion to immediately after postgres.h, as is our normal custom?

            regards, tom lane

> On May 15, 2018, at 8:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Mark Dilger <hornschnorter@gmail.com> writes:
>> My best guess at the moment is:
>
>> diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
>> index c1f0441b08..0a3163398f 100644
>> --- a/src/backend/utils/init/globals.c
>> +++ b/src/backend/utils/init/globals.c
>> @@ -16,8 +16,11 @@
>>  *
>>  *-------------------------------------------------------------------------
>>  */
>> +#include <sys/stat.h>
>> +
>> #include "postgres.h"
>
>> +#include "common/file_perm.h"
>
> Yipes.  Frost, you didn't really do that did you?  That's a blatant
> break of the "c.h must come first" rule.  Whether or not it broke the
> Windows build, there are other platforms it'll break.
>
>> Indeed, the following change (shown here for illustrative purposes only; please
>> don't commit it this way) fixes the problem, at least in my build environment:
>
> That's pretty ugly, but what happens if you just move the <sys/stat.h>
> inclusion to immediately after postgres.h, as is our normal custom?

That also works.

mark

Greetings,

* Mark Dilger (hornschnorter@gmail.com) wrote:
> > On May 15, 2018, at 8:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Mark Dilger <hornschnorter@gmail.com> writes:
> >> My best guess at the moment is:
> >
> >> diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
> >> index c1f0441b08..0a3163398f 100644
> >> --- a/src/backend/utils/init/globals.c
> >> +++ b/src/backend/utils/init/globals.c
> >> @@ -16,8 +16,11 @@
> >>  *
> >>  *-------------------------------------------------------------------------
> >>  */
> >> +#include <sys/stat.h>
> >> +
> >> #include "postgres.h"
> >
> >> +#include "common/file_perm.h"
> >
> > Yipes.  Frost, you didn't really do that did you?  That's a blatant
> > break of the "c.h must come first" rule.  Whether or not it broke the
> > Windows build, there are other platforms it'll break.

Evidently I managed to.

> >> Indeed, the following change (shown here for illustrative purposes only; please
> >> don't commit it this way) fixes the problem, at least in my build environment:
> >
> > That's pretty ugly, but what happens if you just move the <sys/stat.h>
> > inclusion to immediately after postgres.h, as is our normal custom?
>
> That also works.

Good, will fix.

Thanks!

Stephen

> On May 15, 2018, at 9:29 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Mark Dilger (hornschnorter@gmail.com) wrote:
>>> On May 15, 2018, at 8:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Mark Dilger <hornschnorter@gmail.com> writes:
>>>> My best guess at the moment is:
>>>
>>>> diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
>>>> index c1f0441b08..0a3163398f 100644
>>>> --- a/src/backend/utils/init/globals.c
>>>> +++ b/src/backend/utils/init/globals.c
>>>> @@ -16,8 +16,11 @@
>>>> *
>>>> *-------------------------------------------------------------------------
>>>> */
>>>> +#include <sys/stat.h>
>>>> +
>>>> #include "postgres.h"
>>>
>>>> +#include "common/file_perm.h"
>>>
>>> Yipes.  Frost, you didn't really do that did you?  That's a blatant
>>> break of the "c.h must come first" rule.  Whether or not it broke the
>>> Windows build, there are other platforms it'll break.
>
> Evidently I managed to.
>
>>>> Indeed, the following change (shown here for illustrative purposes only; please
>>>> don't commit it this way) fixes the problem, at least in my build environment:
>>>
>>> That's pretty ugly, but what happens if you just move the <sys/stat.h>
>>> inclusion to immediately after postgres.h, as is our normal custom?
>>
>> That also works.
>
> Good, will fix.

I'm curious why the Windows build farm members did not pick this up.  Or
perhaps they did?  (I don't get emails about that.)  If none of the animals
are configured to detect this bug, perhaps the community needs another
Windows animal configured along the lines of the build machine I am using?

Please advise...

mark

Mark Dilger <hornschnorter@gmail.com> writes:
> I'm curious why the Windows build farm members did not pick this up.  Or
> perhaps they did?  (I don't get emails about that.)

They did not, and I too was wondering why not.

> If none of the animals
> are configured to detect this bug, perhaps the community needs another
> Windows animal configured along the lines of the build machine I am using?

+1.  How do you have yours configured, anyway?

            regards, tom lane

> On May 15, 2018, at 9:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnorter@gmail.com> writes:
>> I'm curious why the Windows build farm members did not pick this up.  Or
>> perhaps they did?  (I don't get emails about that.)
> 
> They did not, and I too was wondering why not.
> 
>> If none of the animals
>> are configured to detect this bug, perhaps the community needs another
>> Windows animal configured along the lines of the build machine I am using?
> 
> +1.  How do you have yours configured, anyway?

I mostly develop on mac and linux and don't look at the windows system
too much:


Windows Server 2008 R2 Standard
Service Pack 1
Processor: Intel Xeon CPU E5-2676 v3 @ 2.40GHz
Installed memory:  8.00 GB
System type: 64 bit Operating System

Microsoft Visual Studio 2008 Beta2 x64 cross tools
Microsoft (R) C/C++ Optimizing Compiler Version 15.00.21022.08 for x64

Mark Dilger <hornschnorter@gmail.com> writes:
>>> If none of the animals
>>> are configured to detect this bug, perhaps the community needs another
>>> Windows animal configured along the lines of the build machine I am using?

>> +1.  How do you have yours configured, anyway?

> I mostly develop on mac and linux and don't look at the windows system
> too much:

> Windows Server 2008 R2 Standard
> Service Pack 1
> Microsoft Visual Studio 2008 Beta2 x64 cross tools

Hm.  I'm not sure what our nominal support range for Visual Studio is.
I see that mastodon (running VS2005) and currawong (running VS2008)
haven't reported on HEAD lately, and I think they may have been shut
down intentionally due to desupport?  But if your build still works
then it seems like we could continue to support VS2008.

            regards, tom lane

> On May 15, 2018, at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Mark Dilger <hornschnorter@gmail.com> writes:
>>>> If none of the animals
>>>> are configured to detect this bug, perhaps the community needs another
>>>> Windows animal configured along the lines of the build machine I am using?
>
>>> +1.  How do you have yours configured, anyway?
>
>> I mostly develop on mac and linux and don't look at the windows system
>> too much:
>
>> Windows Server 2008 R2 Standard
>> Service Pack 1
>> Microsoft Visual Studio 2008 Beta2 x64 cross tools
>
> Hm.  I'm not sure what our nominal support range for Visual Studio is.
> I see that mastodon (running VS2005) and currawong (running VS2008)
> haven't reported on HEAD lately, and I think they may have been shut
> down intentionally due to desupport?  But if your build still works
> then it seems like we could continue to support VS2008.

I don't have a strong opinion on that.  I could also look to upgrade
to a newer version.  Generally, I try to build using the oldest
supported version rather than the newest.  What is the next oldest
working test machine you have?

mark

Mark Dilger <hornschnorter@gmail.com> writes:
>> On May 15, 2018, at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hm.  I'm not sure what our nominal support range for Visual Studio is.
>> I see that mastodon (running VS2005) and currawong (running VS2008)
>> haven't reported on HEAD lately, and I think they may have been shut
>> down intentionally due to desupport?  But if your build still works
>> then it seems like we could continue to support VS2008.

> I don't have a strong opinion on that.  I could also look to upgrade
> to a newer version.  Generally, I try to build using the oldest
> supported version rather than the newest.  What is the next oldest
> working test machine you have?

thrips is described as running VS2010, though I'm not sure how to verify
that it's not been updated.  The make log shows

  c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\bin\AMD64\CL.exe

but is "10.0" the same thing as "2010"?

            regards, tom lane

On 05/15/2018 01:20 PM, Tom Lane wrote:
> Mark Dilger <hornschnorter@gmail.com> writes:
>>>> If none of the animals
>>>> are configured to detect this bug, perhaps the community needs another
>>>> Windows animal configured along the lines of the build machine I am using?
>>> +1.  How do you have yours configured, anyway?
>> I mostly develop on mac and linux and don't look at the windows system
>> too much:
>> Windows Server 2008 R2 Standard
>> Service Pack 1
>> Microsoft Visual Studio 2008 Beta2 x64 cross tools
> Hm.  I'm not sure what our nominal support range for Visual Studio is.
> I see that mastodon (running VS2005) and currawong (running VS2008)
> haven't reported on HEAD lately, and I think they may have been shut
> down intentionally due to desupport?  But if your build still works
> then it seems like we could continue to support VS2008.
>
>             


currawong and friends were shut down in January on >= 11 because of the 
huge pages issue on 32-bit XP. So they aren't going to be revived. Maybe 
we can make VS2008 work on a 64 bit machine. Personally I'm more 
interested in better getting coverage for the modern compilers.


cheers

andrew