Thread: back branches vs. VS 2008
The following patch allows me to build the 8.3 and 8.4 branches using
Visual Studio 2008, once the build system is patched. But I don't really
know why. HEAD and 9.0 build fine without it. But those branches
branches fail with a complaint about IPPROTO_IPV6 being undefined.
The patch seems harmless enough. But I'd like to know why it's
happening. Does anyone have a clue?
cheers
andrew
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 06aece3..c1775ea 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -383,7 +383,7 @@ StreamServerPort(int family, char *hostName,
unsigned short portNumber, } #endif
-#ifdef IPV6_V6ONLY
+#if defined(IPV6_V6ONLY) && defined(IPPROTO_IPV6) if (addr->ai_family == AF_INET6) { if
(setsockopt(fd,IPPROTO_IPV6, IPV6_V6ONLY,
On Mon, Jan 3, 2011 at 18:15, Andrew Dunstan <andrew@dunslane.net> wrote: > > The following patch allows me to build the 8.3 and 8.4 branches using Visual > Studio 2008, once the build system is patched. But I don't really know why. > HEAD and 9.0 build fine without it. But those branches branches fail with a > complaint about IPPROTO_IPV6 being undefined. > > The patch seems harmless enough. But I'd like to know why it's happening. > Does anyone have a clue? Umm. Since when do we backpatch new features/platforms? I don't know exactly why that is happening, but it's a good indicator that backpatching it isn't necessarily safe - what else can be missed? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On 01/03/2011 12:43 PM, Magnus Hagander wrote: > On Mon, Jan 3, 2011 at 18:15, Andrew Dunstan<andrew@dunslane.net> wrote: >> The following patch allows me to build the 8.3 and 8.4 branches using Visual >> Studio 2008, once the build system is patched. But I don't really know why. >> HEAD and 9.0 build fine without it. But those branches branches fail with a >> complaint about IPPROTO_IPV6 being undefined. >> >> The patch seems harmless enough. But I'd like to know why it's happening. >> Does anyone have a clue? > Umm. Since when do we backpatch new features/platforms? > > I don't know exactly why that is happening, but it's a good indicator > that backpatching it isn't necessarily safe - what else can be missed? > This isn't a new platform, any more than a new version of gcc is a new platform. And I certainly don't understand your reference to new features. I'm not suggesting backporting one. I'm not going to maintain more than one buildfarm member doing MSVC, and and if we were to adopt your policy I would not be able to use a modern-ish version of the compiler/SDK and also build all the live branches. That seems quite unnecessary. If we'd backported the changes to support VS2008 when they were made a year or two ago, as we should have (the changes are pretty trivial), we'd probably have discovered this back then. I'm putting in this effort because Tom complained about lack of buildfarm coverage that occurred when I recently lost the machine my buildfarm members were running on, and I'm trying to get back the coverage they had. cheers andrew
On Mon, Jan 3, 2011 at 19:08, Andrew Dunstan <andrew@dunslane.net> wrote: > > > On 01/03/2011 12:43 PM, Magnus Hagander wrote: >> >> On Mon, Jan 3, 2011 at 18:15, Andrew Dunstan<andrew@dunslane.net> wrote: >>> >>> The following patch allows me to build the 8.3 and 8.4 branches using >>> Visual >>> Studio 2008, once the build system is patched. But I don't really know >>> why. >>> HEAD and 9.0 build fine without it. But those branches branches fail with >>> a >>> complaint about IPPROTO_IPV6 being undefined. >>> >>> The patch seems harmless enough. But I'd like to know why it's happening. >>> Does anyone have a clue? >> >> Umm. Since when do we backpatch new features/platforms? >> >> I don't know exactly why that is happening, but it's a good indicator >> that backpatching it isn't necessarily safe - what else can be missed? >> > > This isn't a new platform, any more than a new version of gcc is a new > platform. And I certainly don't understand your reference to new features. > I'm not suggesting backporting one. It most definitely is a new platform in a *lot* more ways than a new version of gcc. It's the whole PlatformSDK. Why else did it require patches to the code? And it is a new feature *to the msvc build system*. > I'm not going to maintain more than one buildfarm member doing MSVC, and and > if we were to adopt your policy I would not be able to use a modern-ish > version of the compiler/SDK and also build all the live branches. That seems > quite unnecessary. If we'd backported the changes to support VS2008 when > they were made a year or two ago, as we should have (the changes are pretty > trivial), we'd probably have discovered this back then. Well, it's perfectly possible to have more tha none version of MSVC on the machine. And we're not going to be changing the version that's actually used for the official binary builds, so all you'll accomplish then is to have the buildfarm test something different form what we're shipping. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> On Mon, Jan 3, 2011 at 19:08, Andrew Dunstan <andrew@dunslane.net> wrote:
>> I'm not going to maintain more than one buildfarm member doing MSVC, and and
>> if we were to adopt your policy I would not be able to use a modern-ish
>> version of the compiler/SDK and also build all the live branches.
> Well, it's perfectly possible to have more tha none version of MSVC on
> the machine.
> And we're not going to be changing the version that's actually used
> for the official binary builds, so all you'll accomplish then is to
> have the buildfarm test something different form what we're shipping.
Are you speaking for EDB on that? Do you even know what they're using
to build the Windows installers?
We've made backpatches before to support building/running older branches
on newer platforms. We do it all the time in fact. (The latest
instance was hacking the Linux wal_sync_method defaults. If you think
this isn't a necessary activity, try building a 7.1 or so branch with a
modern gcc.) It might be reasonable to argue that this particular patch
is too invasive to be safe to back-patch, but I don't agree with the
premise that it isn't a reasonable topic for a back-patch.
I do have some concern about loss of buildfarm coverage for older VS
versions, but if Andrew isn't going to cover those, perhaps someone else
will step up for that.
regards, tom lane
On Mon, Jan 3, 2011 at 19:50, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On Mon, Jan 3, 2011 at 19:08, Andrew Dunstan <andrew@dunslane.net> wrote: >>> I'm not going to maintain more than one buildfarm member doing MSVC, and and >>> if we were to adopt your policy I would not be able to use a modern-ish >>> version of the compiler/SDK and also build all the live branches. > >> Well, it's perfectly possible to have more tha none version of MSVC on >> the machine. > >> And we're not going to be changing the version that's actually used >> for the official binary builds, so all you'll accomplish then is to >> have the buildfarm test something different form what we're shipping. > > Are you speaking for EDB on that? Do you even know what they're using > to build the Windows installers? Yes and yes. Well, I'm not actually speaking for them, so I guess we'll need a +1 from Dave. But given that the principle has held for all the previous releases made, I assume it still does. The second yes is not pending a +1, I know exaclty what they use. > We've made backpatches before to support building/running older branches > on newer platforms. We do it all the time in fact. (The latest > instance was hacking the Linux wal_sync_method defaults. If you think > this isn't a necessary activity, try building a 7.1 or so branch with a > modern gcc.) It might be reasonable to argue that this particular patch > is too invasive to be safe to back-patch, but I don't agree with the > premise that it isn't a reasonable topic for a back-patch. Fair enough. I don't care enough to object more :-) > I do have some concern about loss of buildfarm coverage for older VS > versions, but if Andrew isn't going to cover those, perhaps someone else > will step up for that. I'd be worried if we don't have coverage for the versions that are actually used to build the installers. But those may be what Dave has covered on his animals already - Dave? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On 01/03/2011 01:50 PM, Tom Lane wrote: > It might be reasonable to argue that this particular patch > is too invasive to be safe to back-patch, but I don't agree with the > premise that it isn't a reasonable topic for a back-patch. The patch for the non-buildsystem code is one line. The rest is about 20 lines. > I do have some concern about loss of buildfarm coverage for older VS > versions, but if Andrew isn't going to cover those, perhaps someone else > will step up for that. > The machine involved already has three buildfarm critters. If I have to have three versions of VS installed (since we're now talking about installing a new one) that will grow to five, on one VM currently running on a small not very powerful Athlon X2 machine. It's already a pain in the neck to manage. Some time in the future I might have resources to run more, but right now I do not. Incidentally, I just went looking for VS2005/Express on microsoft.com. I don't know if they still make it available, but if they do it's fairly well hidden. I could find VS2008/Express and VS2010/Express very easily. ISTM that having support on the live branches for the compilers/SDKs that Microsoft apparently actually supports and distributes is not a bad thing to have. cheers andrew
On Mon, Jan 3, 2011 at 2:23 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > Incidentally, I just went looking for VS2005/Express on microsoft.com. I > don't know if they still make it available, but if they do it's fairly well > hidden. I could find VS2008/Express and VS2010/Express very easily. ISTM > that having support on the live branches for the compilers/SDKs that > Microsoft apparently actually supports and distributes is not a bad thing to > have. I agree. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Jan 3, 2011 at 6:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On Mon, Jan 3, 2011 at 19:08, Andrew Dunstan <andrew@dunslane.net> wrote: >>> I'm not going to maintain more than one buildfarm member doing MSVC, and and >>> if we were to adopt your policy I would not be able to use a modern-ish >>> version of the compiler/SDK and also build all the live branches. > >> Well, it's perfectly possible to have more tha none version of MSVC on >> the machine. > >> And we're not going to be changing the version that's actually used >> for the official binary builds, so all you'll accomplish then is to >> have the buildfarm test something different form what we're shipping. > > Are you speaking for EDB on that? He's not speaking *for* us, but he's absolutely right. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Dave Page <dpage@pgadmin.org> writes:
> On Mon, Jan 3, 2011 at 6:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Magnus Hagander <magnus@hagander.net> writes:
>>> And we're not going to be changing the version that's actually used
>>> for the official binary builds, so all you'll accomplish then is to
>>> have the buildfarm test something different form what we're shipping.
>> Are you speaking for EDB on that?
> He's not speaking *for* us, but he's absolutely right.
OK, so what about the next question: is EDB running buildfarm members
that will test the VS version(s) you are using? I don't think Andrew
is under any obligation to do that for you.
regards, tom lane
On 01/03/2011 03:04 PM, Tom Lane wrote: > Dave Page<dpage@pgadmin.org> writes: >> On Mon, Jan 3, 2011 at 6:50 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >>> Magnus Hagander<magnus@hagander.net> writes: >>>> And we're not going to be changing the version that's actually used >>>> for the official binary builds, so all you'll accomplish then is to >>>> have the buildfarm test something different form what we're shipping. >>> Are you speaking for EDB on that? >> He's not speaking *for* us, but he's absolutely right. > OK, so what about the next question: is EDB running buildfarm members > that will test the VS version(s) you are using? I don't think Andrew > is under any obligation to do that for you. > > They have baiji (Vista/MSVC 2005 Pro) and mastodon (WS2K3R2/MSVC 2005 Express). EDB have been pretty good about buildfarm support. But more importantly, the buildfarm is about more than just "official build platform" support. Suppose that you're running 8.4 in your enterprise, and you want to run a patched Postgres, or one with an extra module you wrote. You want to be able to build it with current tools, no matter what the "official" builds are made with, and you don't want to be forced to upgrade before you're ready. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> But more importantly, the buildfarm is about more than just "official
> build platform" support.
Agreed, we should be testing as many build platforms as possible. I was
just concerned about whether we were losing any coverage.
regards, tom lane
On Mon, Jan 3, 2011 at 8:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Dave Page <dpage@pgadmin.org> writes: >> On Mon, Jan 3, 2011 at 6:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Magnus Hagander <magnus@hagander.net> writes: >>>> And we're not going to be changing the version that's actually used >>>> for the official binary builds, so all you'll accomplish then is to >>>> have the buildfarm test something different form what we're shipping. > >>> Are you speaking for EDB on that? > >> He's not speaking *for* us, but he's absolutely right. > > OK, so what about the next question: is EDB running buildfarm members > that will test the VS version(s) you are using? I don't think Andrew > is under any obligation to do that for you. Yes, except for VC++ 2008/Win7 which we use for 9.0. That one remains on my todo list. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 01/03/2011 12:15 PM, I wrote:
>
> The following patch allows me to build the 8.3 and 8.4 branches using
> Visual Studio 2008, once the build system is patched. But I don't
> really know why. HEAD and 9.0 build fine without it. But those
> branches branches fail with a complaint about IPPROTO_IPV6 being
> undefined.
>
> The patch seems harmless enough. But I'd like to know why it's
> happening. Does anyone have a clue?
>
>
> -#ifdef IPV6_V6ONLY
> +#if defined(IPV6_V6ONLY) && defined(IPPROTO_IPV6)
> if (addr->ai_family == AF_INET6)
> {
> if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY,
OK, what's going here is that, in the newer SDK, IPV6_V6ONLY is defined
unconditionally, but IPPROTO_IPV6 is only defined if _WIN32_WINNT is set
to 0x0501 or higher. We defined _WIN32_WINNT as 0x0500 until 9.0, when
we changed it specifically to allow use of the right IPV6 settings.
This seems to me like a clear error in the MS headers. I don't think it
makes any sense to define the settings constant but not the context
constant. The fix I have suggested above doesn't seem unreasonable or
terribly unsafe in these circumstances. The code clearly contemplates
the setsockopt() call in question not having been run, as shown in this
comment:
/* * Note: This might fail on some OS's, like Linux older than * 2.4.21-pre3, that don't have
theIPV6_V6ONLY socket option,
and map * ipv4 addresses to ipv6. It will show ::ffff:ipv4 for all ipv4 * connections. */
cheers
andrew
On Tue, Jan 4, 2011 at 04:49, Andrew Dunstan <andrew@dunslane.net> wrote:
>
> On 01/03/2011 12:15 PM, I wrote:
>>
>> The following patch allows me to build the 8.3 and 8.4 branches using
>> Visual Studio 2008, once the build system is patched. But I don't really
>> know why. HEAD and 9.0 build fine without it. But those branches branches
>> fail with a complaint about IPPROTO_IPV6 being undefined.
>>
>> The patch seems harmless enough. But I'd like to know why it's happening.
>> Does anyone have a clue?
>>
>>
>> -#ifdef IPV6_V6ONLY
>> +#if defined(IPV6_V6ONLY) && defined(IPPROTO_IPV6)
>> if (addr->ai_family == AF_INET6)
>> {
>> if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY,
>
>
> OK, what's going here is that, in the newer SDK, IPV6_V6ONLY is defined
> unconditionally, but IPPROTO_IPV6 is only defined if _WIN32_WINNT is set to
> 0x0501 or higher. We defined _WIN32_WINNT as 0x0500 until 9.0, when we
> changed it specifically to allow use of the right IPV6 settings.
I wonder if anything else changed with that #define, though.
> This seems to me like a clear error in the MS headers. I don't think it
> makes any sense to define the settings constant but not the context
> constant. The fix I have suggested above doesn't seem unreasonable or
> terribly unsafe in these circumstances. The code clearly contemplates the
> setsockopt() call in question not having been run, as shown in this comment:
Yeah, it seems reasonable - I assume you tested it and it doesn't fail
in some *different* way than the one we expect in the code?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On 01/04/2011 04:43 AM, Magnus Hagander wrote: > >> OK, what's going here is that, in the newer SDK, IPV6_V6ONLY is defined >> unconditionally, but IPPROTO_IPV6 is only defined if _WIN32_WINNT is set to >> 0x0501 or higher. We defined _WIN32_WINNT as 0x0500 until 9.0, when we >> changed it specifically to allow use of the right IPV6 settings. > I wonder if anything else changed with that #define, though. Probably. I'm not going to suggest turning it on at this stage. There are lots of references to this specific OS level in the headers. > >> This seems to me like a clear error in the MS headers. I don't think it >> makes any sense to define the settings constant but not the context >> constant. The fix I have suggested above doesn't seem unreasonable or >> terribly unsafe in these circumstances. The code clearly contemplates the >> setsockopt() call in question not having been run, as shown in this comment: > Yeah, it seems reasonable - I assume you tested it and it doesn't fail > in some *different* way than the one we expect in the code? > Yes, I enabled IPV6 and set listen_addresses to * an no untoward events appeared. cheers andrew