postgresql should support --universal

[erg]

When I naively enable universal support, it dies in compilation. Having a universal libpq.dylib is integral to testing my language since we support 32/64 bit libraries. I believe a universal build works with port, but I'm trying to only use brew and this is the last hurdle.

[MikeMcQuaid]

Mountain Lion and Lion both are 64-bit only so we're probably not going to invest much time into getting this working. Patches welcome here.

[erg]

I'm not sure what you mean that they're 64-bit only.

modern:~ erg$ uname -a
Darwin modern.local 12.0.0 Darwin Kernel Version 12.0.0: Sun Jun 24 23:00:16 PDT 2012; root:xnu-2050.7.9~1/RELEASE_X86_64 x86_64
modern:~ erg$ cat test.c 
int main() { return sizeof(long); }
modern:~ erg$ clang -m32 test.c 
modern:~ erg$ ./a.out 
modern:~ erg$ echo $?
4
modern:~ erg$ clang test.c 
modern:~ erg$ ./a.out 
modern:~ erg$ echo $?
8

[adamv]

@MikeMcQuaid

On my Lion box:

$ file /usr/bin/python
/usr/bin/python: Mach-O universal binary with 2 architectures
/usr/bin/python (for architecture x86_64):  Mach-O 64-bit executable x86_64
/usr/bin/python (for architecture i386):    Mach-O executable i386

By 64-bit only, you mean kernel arch, right?

[MikeMcQuaid]

I mean both kernel arch and that they only support 64-bit processors.

[adamv]

But 64-bit processors support 32-bit code.

[jacknagel]

And there are good reasons to build 32-bit binaries (think data cache for a 32- vs 64-bit address space, for example).

[MikeMcQuaid]

And good reasons to build 64-bit binaries. Anyway, I don't mind what you do but I personally don't see the point in working to make --universal work better. @jacknagel and @adamv disagree so maybe they'll implement this.

[jacknagel]

Well obviously I wasn't suggesting that 32-bit binaries are always better. That would be silly. As with most things it depends on the situation.

The interests of our users vary widely. Some are using their machines to build Rails apps and couldn't care less about these details, others are involved in scientific computing where the pros and cons of 32-bit/64-bit/universal binaries are significant. I think we should try to cater to these interests as best we can. They certainly deserve more consideration than to be dismissed as "stupid" at this point.

[MikeMcQuaid]

I'm aware of the differences between 32-bit and 64-bit; I've done high-performance computing projects for telecoms companies and heavy data processing for oil and gas companies. I've spent most of my career writing C++ where performance is a critical factor. Let's all just assume the other Homebrew maintainers aren't ignorant about these benefits.

I hold by that us doing 32-bit only support for packages is stupid because of the benefits being speculative rather than actually proposed or measured. I don't think this fits into Homebrew's remit; at best it's a niche. I'd frankly be amazed if 32-bit vs 64-bit on OSX is going to offer better performance over a highly-tuned Linux or other BSD at that point.

Like my former complaints about duplicates I'm vocal about this partly because I think it's a waste of time and partly so people know that I won't fix these issues as I don't care.

[jacknagel]

Let's all just assume the other Homebrew maintainers aren't ignorant about these benefits.

I don't recall questioning your knowledge or credentials, and I'm not interested in getting into a pissing match. But could you really blame me for assuming otherwise when you regularly dismiss these issues with little or no discussion?

I still don't understand why you have adopted this attitude towards this topic, and frankly I don't care, but it's a definite turn off to users when their concerns and requests are routinely dismissed as not important enough to you personally.

[MikeMcQuaid]

Let's continue this privately.

[mrjbq7]

@MikeMcQuaid Wouldn't it be better for you to stay out of these discussions if you think they are a waste of time and let others respond?

[mrjbq7]

FWIW, I would like to see --universal for all projects that support it, and I have contributed a few already.

[MikeMcQuaid]

I don't think the discussion is a waste of time. For some of these I reply because I feel people would rather have someone say "this isn't a priority" than not reply to their issue (which is what sometimes happens). I'll happily bow out of this one.

[jacknagel]

I hope you can understand why I think your stance of "I don't care, but I'm going to argue against this anyway" is toxic. I don't really have anything else to say.

[mrjbq7]

Looks like it might require a patch to fixup a switch statement:

/usr/bin/clang -Os -w -pipe -march=native -Qunused-arguments -mmacosx-version-min=10.8 -arch i386 -arch x86_64 -I/usr/local/Cellar/ossp-uuid/1.6.2/include -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -I../../../../src/include -I/usr/local/Cellar/readline/6.2.4/include -isystem /usr/local/include -I/usr/include/libxml2 -I/usr/include/libxml2   -c -o heaptuple.o heaptuple.c
heaptuple.c:196:4: error: duplicate case value '4'
                        store_att_byval(data, values[i], att[i]->attlen);
                        ^
../../../../src/include/access/tupmacs.h:211:9: note: expanded from macro 'store_att_byval'
                        case sizeof(Datum): \
                             ^
heaptuple.c:196:4: note: previous case defined here
                        store_att_byval(data, values[i], att[i]->attlen);
                        ^
../../../../src/include/access/tupmacs.h:208:9: note: expanded from macro 'store_att_byval'
                        case sizeof(int32): \
                             ^
1 error generated.
make[4]: *** [heaptuple.o] Error 1
make[3]: *** [common-recursive] Error 2
make[2]: *** [access-recursive] Error 2
make[2]: *** Waiting for unfinished jobs....

[mrjbq7]

A discussion from pgsql-general about it:

http://postgresql.1045698.n5.nabble.com/Build-universal-binary-on-Mac-OS-X-10-6-td1857669.html

[MikeMcQuaid]

To clarify one thing before I head: I shouldn't have used the phrase "I don't care" (and I don't believe in editing what I've said after the fact). What I meant to say is that I think it is not a good idea but if others want to contribute this I'm not going to do anything silly like try and block it's inclusion. Ultimately if stuff like this can be added and maintained with very little overhead that seems fair enough; apologies to all involved for stating otherwise.

[mrjbq7]

@MikeMcQuaid I know you mean well, and emotions sometimes communicate poorly over text. I would note that Fink, MacPorts, and OS X Server all support universal builds in this case, so I think we should as well.

[adamv]

Closing in favor of #14111.

[erg]

@MikeMcQuaid

What I meant to say is that I think it is not a good idea but if others want to contribute this I'm not going to do anything silly like try and block it's inclusion.

Isn't that what you did by closing #14775?

/grumble

[MikeMcQuaid]

I know this seems like it's our fault but it's actually the fault of upstream. I suggest blaming them for not accepting this patch. postgresql doesn't support universal builds and won't accept patches to fix that so there's not much we can do about it.

[MikeMcQuaid]

I suggest if you need universal support you maintain it in your own tap or fork using one of the previously supplied patches.

[erg]

I did apply the patch, but when I did a brew update it went back to non-universal because I didn't reapply the patch. It's also annoying because I have several computers that need this patch.

The patch is a really simple, albeit unorthodox solution and macports implements it the same way. Maybe rather than downloading the patch we could just include it in the brew github repository?

[mrjbq7]

@MikeMcQuaid Can you look at #14111 (comment) and suggest a better way to do the universal builds?

[MikeMcQuaid]

You can brew edit the formula (or brew pull the closed pull request) to add the patch yourself and commit it locally in another branch or create a tap to be used with brew tap. The documentation for the above should be on the wiki, in man brew or in the code. Beyond this I suggest trying to convince upstream to accept a universal patch.

If you're wanting to make the patch better (which probably isn't any more likely to be accepted) it needs to be something in the def patches block instead of downloading it at installation time.

Just to reiterate to be completely unambiguous: the problem is not the particular patch or the approach; the problem is that upstream (i.e. the PostgreSQL developers) will apparently not accept a universal patch and we don't want to maintain patches long-term for things upstream have explicitly said they do not want or support as this means if we break �postgresql for users of --universal then upstream will be pissed at us and so will our users.

[Sharpie]

Agreed that we don't want to be in the business of maintaining patches that upstream is not interested in.

[mrjbq7]

The "patches" here are very minor and entirely in simple header files.

I rewrote them as ruby string replacements, for those of you allergic to ed:

    if build.universal?
      inreplace "./src/include/pg_config.h" do |s|
        s.gsub!(/^.* ALIGNOF_DOUBLE. *$/, "#ifdef __LP64__\n#define ALIGNOF_DOUBLE 8\n#else\n#define ALIGNOF_DOUBLE 4\n#endif")
        s.gsub!(/^.* ALIGNOF_LONG .*$/, "#ifdef __LP64__\n#define ALIGNOF_LONG 8\n#else\n#define ALIGNOF_LONG 4\n#endif")
        s.gsub!(/^.* ALIGNOF_LONG_LONG_INT .*$/, "#ifdef __LP64__\n/* #undef ALIGNOF_LONG_LONG_INT */\n#else\n#define ALIGNOF_LONG_LONG_INT 4\n#endif")
        s.gsub!(/^.* FLOAT8PASSBYVAL .*$/, "#ifdef __LP64__\n#define FLOAT8PASSBYVAL true\n#else\n#define FLOAT8PASSBYVAL false\n#endif")
        s.gsub!(/^.* HAVE_LL_CONSTANTS .*$/, "#ifdef __LP64__\n/* #undef HAVE_LL_CONSTANTS */\n#else\n#define HAVE_LL_CONSTANTS 1\n#endif")
        s.gsub!(/^.* HAVE_LONG_INT_64 .*$/, "#ifdef __LP64__\n#define HAVE_LONG_INT_64 1\n#else\n/* #undef HAVE_LONG_INT_64 */\n#endif")
        s.gsub!(/^.* HAVE_LONG_LONG_INT_64 .*$/, "#ifdef __LP64__\n/* #undef HAVE_LONG_LONG_INT_64 */\n#else\n#define HAVE_LONG_LONG_INT_64 1\n#endif")
        s.gsub!(/^.* INT64_FORMAT .*$/, "#ifdef __LP64__\n#define INT64_FORMAT \"%ld\"\n#else\n#define INT64_FORMAT \"%lld\"\n#endif")
        s.gsub!(/^.* MAXIMUM_ALIGNOF. *$/, "#ifdef __LP64__\n#define MAXIMUM_ALIGNOF 8\n#else\n#define MAXIMUM_ALIGNOF 4\n#endif")
        s.gsub!(/^.* SIZEOF_LONG .*$/, "#ifdef __LP64__\n#define SIZEOF_LONG 8\n#else\n#define SIZEOF_LONG 4\n#endif")
        s.gsub!(/^.* SIZEOF_SIZE_T .*$/, "#ifdef __LP64__\n#define SIZEOF_SIZE_T 8\n#else\n#define SIZEOF_SIZE_T 4\n#endif")
        s.gsub!(/^.* SIZEOF_VOID_P .*$/, "#ifdef __LP64__\n#define SIZEOF_VOID_P 8\n#else\n#define SIZEOF_VOID_P 4\n#endif")
        s.gsub!(/^.* UINT64_FORMAT .*$/, "#ifdef __LP64__\n#define UINT64_FORMAT \"%lu\"\n#else\n#define UINT64_FORMAT \"%llu\"\n#endif")
        s.gsub!(/^.* USE_FLOAT8_BYVAL .*/, "#ifdef __LP64__\n#define USE_FLOAT8_BYVAL 1\n#else\n/* #undef USE_FLOAT8_BYVAL */\n#endif")
      end
      inreplace "./src/interfaces/ecpg/include/ecpg_config.h" do |s|
        s.gsub!(/^.* HAVE_LONG_INT_64 .*$/, "#ifdef __LP64__\n#define HAVE_LONG_INT_64 1\n#else\n/* #undef HAVE_LONG_INT_64 */\n#endif")
        s.gsub!(/^.* HAVE_LONG_LONG_INT_64 .*$/, "#ifdef __LP64__\n/* #undef HAVE_LONG_LONG_INT_64 */\n#else\n#define HAVE_LONG_LONG_INT_64 1\n#endif")
      end
    end

Basically, it replaces things like this:

/* The normal alignment of `double', in bytes. */
#define ALIGNOF_DOUBLE 8

With this:

/* The normal alignment of `double', in bytes. */
#ifdef __LP64__
#define ALIGNOF_DOUBLE 8
#else
#define ALIGNOF_DOUBLE 4
#endif

How is this very objectionable?

The alternative suggested by upstream is to configure and build twice and then use lipo to combine the libraries. Seems to me, this is much simpler and nicer and not a large patch against upstream, and how MacPorts does it.

[MikeMcQuaid]

How is this very objectionable?

Ask upstream.

The alternative suggested by upstream is to configure and build twice and then use lipo to combine the libraries. Seems to me, this is much simpler and nicer and not a large patch against upstream, and how MacPorts does it.
Do that then, maintain your own tap/fork or use MacPorts.

Please can we let this die? I know it's annoying but, again, go shout at upstream instead of us.