This repository has been archived by the owner. It is now read-only.

libmemcached: fix build "tr1/cintypes" error on OS X 10.9 #22259

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@DominikTo
Copy link
Contributor

DominikTo commented Sep 2, 2013

A fix for the "tr1/cintypes" build issue on OS X 10.9.
Fixes #20625.

Regarding #20635:

I was told:

  • The tr1 headers were an experiment by the C++ standards committee. They are not part of any standard.
  • On Mavericks libc++ has become the default used by the compiler clang.
  • The newer libc++ lacks the tr1 experiment. However most of tr1 was standardized with C++11, and now lives in namespace std, and in headers not prefixed with tr1/.
@manphiz

View changes

Library/Formula/libmemcached.rb Outdated
-#ifdef __cplusplus
-# include <tr1/cinttypes>
+#ifdef _LIBCPP_VERSION
+# include <cinttypes>

This comment has been minimized.

@manphiz

manphiz Sep 2, 2013

Contributor

This part is incorrect and will break non-C++11 compilers. Should be something like

#ifdef __cplusplus
#ifdef _LIBCPP_VERSION
#  include <cinttypes>
#else
#  include <tr1/cinttypes>
#endif
... [the rest]

Also, has this issue been reported upstream?

This comment has been minimized.

@DominikTo

DominikTo Sep 2, 2013

Contributor

Good catch. Updated the PR.

This issue has also been reported here:
https://bugs.launchpad.net/libmemcached/+bug/1216521

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Sep 2, 2013

10.9 fixes are generally not accepted for now until formal release. Advice to report upstream.

Also it looks like 10.9 no longer ships libstdc++? Can you confirm?

@DominikTo

This comment has been minimized.

Copy link
Contributor

DominikTo commented Sep 2, 2013

/usr/lib/libstdc++.6.0.9.dylib still exists.

Fingers crossed, that it gets fixed upstream:
https://bugs.launchpad.net/libmemcached/+bug/1216521

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Sep 2, 2013

@manphiz I'm wondering if the default stdlib has changed from libstdc++ to libc++. Do you know how to check which stdlib a binary was built against? The otool -L output will just show libSystem in either case.

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Sep 2, 2013

@mistydemeo That's weird. I tried clang++ and clang++ -stdlib=libc++ on 10.8 and otool -L does show which C++ stdlib it is linking to.

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Sep 2, 2013

@manphiz You're right, my mistake - accidentally typed clang instead of clang++.

@DominikTo Can you provide the otool -L output when running clang++ hello.cc -o hello for a generic hello world example?

@DominikTo

This comment has been minimized.

Copy link
Contributor

DominikTo commented Sep 2, 2013

@mistydemeo Sure.

hello:
    /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 120.0.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1197.1.1)
@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Sep 2, 2013

Interesting, it does look like libc++ is now the default stdlib.

@manphiz It's a good thing we have stdlib tracking now... need to beef it up a bit to know about this, and decide what we're going to do about libstdc++ compatibility.

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Sep 3, 2013

@mistydemeo Well it seems that starting from 10.9 both C++03 and C++11 will link to libc++ by default, which can be good or bad, because I've no idea whether these 2 modes are ABI compatible.

@DominikTo

This comment has been minimized.

Copy link
Contributor

DominikTo commented Sep 3, 2013

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 3, 2013

@manphiz I'd be very surprised if they aren't.
@mistydemeo Perhaps a :fails_with is simply enough?

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Sep 3, 2013

@MikeMcQuaid For instance, http://gcc.gnu.org/wiki/Cxx11AbiCompatibility. There's no corresponding document for libc++ that I can find, but I'd expect something similar. That said, it is probably not a good idea to mix C++98/03 objects with C++11 ones, at least for now.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 3, 2013

@mistydemeo @manphiz Also, just occurred to me: it's still not certain at this stage whether changes to Clang behaviour are 10.9 and/or Xcode 5 specific (before we hardcode e.g. 10.9 specific stuff).

@jacknagel

This comment has been minimized.

Copy link
Contributor

jacknagel commented Sep 3, 2013

Note, macports has been doing a lot to handle cxx stdlib stuff recently, it's probably worth looking into how they are handling it.

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Sep 4, 2013

@jacknagel Looks like this cxx_stdlib stuff is to support clang -stdlib=libc++.

Another way to handle standard library implementation is to just modify $CXX, say CXX="clang -stdlib=libc++".

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Sep 7, 2013

Also if Clang 5.0 uses libc++ by default and still provide libstdc++, I guess it is better to adjust the formula to use libstdc++ instead of patching the source. Maybe something like "clang -stdlib=libstdc++".

@bartTC

This comment has been minimized.

Copy link

bartTC commented Oct 5, 2013

This pull request worked fine for me and made libmemcached install without any errors here. Mavericks GM seed and XCode 5.0.1 (5A2034a).

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 5, 2013

@manphiz That's just postponing the problem. Eventually we're going to have to support libc++.

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Oct 5, 2013

I still suggest passing "-stdlib=libstdc++" instead. Some of the TR1
utilities behave differently compared to their C++11 counterparts.

On Oct 5, 2013 1:45 PM, "Mike McQuaid" notifications@github.com wrote:

@manphiz That's just postponing the problem. Eventually we're going to
have to support libc++.


Reply to this email directly or view it on GitHub.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 6, 2013

@manphiz Eventually libstdc++ is going to go away and if we haven't been patching it's going to be very painful. For applications I can maybe see the argument but for libraries that's going to just open a can of worms.

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Oct 6, 2013

@MikeMcQuaid I agree with you, but if as @manphiz points out the TR1 versions of these functions work differently than their C++ counterparts just changing the header location is likely to just cause new problems. This should probably get sorted out by the libmemcached developers, but they haven't commented on the issue in about a month.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 6, 2013

@mistydemeo Does this not mean with our stdlib tracking that anything compiled against libmemcached will also need to use libstdc++? That seems like it's just pushing users towards other bugs or weird functionality when their applications use libc++ by default.

@manphiz When you say "some of they behave differently" is that any used by libmemcached? You know I'm normally very anti-patches but unless this causes known issues I really think we should just merge it.

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Oct 6, 2013

@MikeMcQuaid Yeah, it would essentially mean that users wouldn't be able to link against libmemcached with anything building in the default configuration... which would be nasty.

The ideal thing to do would be to try to engage the libmemcached people again and ask them to look at this now that 10.9 is imminent.

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Oct 6, 2013

I highly doubt that libstdc++ is going away any time soon, given that most 3rd party applications are linking to that at the moment.

@MikeMcQuaid Also I think in principle the patch is wrong, as it is using C++11 header in C++03 code. Though I'm not sure it will cause any real issue, as I don't have 10.9 to test.

That being said, the real fix is still to migrate to C++11, but the change could be substantial and can only be handled properly by upstream. Trying to migrate TR1 code to C++11 is too much burden for package manager and error-prone. What I'm against is to apply the same treatment to other packages, because of those reasons.

And it looks like this issue has not been forwarded upstream yet, no?

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Oct 6, 2013

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Oct 6, 2013

Also currently there is no reverse dependency of libmemcached in core, I'd say the impact to fallback to libstdc++ should be minimum at the moment. I've also added similar comments to upstream bug tracker.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 6, 2013

@manphiz It depends who you want to put the burden on noticing it behaving weirdly on: us or C++ developers who use the brewed libmemcached. Given that nothing depends on it I'd rather we pull it from core than make it use libstdc++ (although patching would be preferred to either).

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 6, 2013

@manphiz The same thing about libstdc++ was said about gcc and llvm-gcc and years later many upstreams still haven't fixed their software to use clang. If you're not sure it'll cause an issue I think we should err on the side of patching. There's also a Mavericks 10.9 VM you can use for testing on bot.brew.sh.

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Oct 6, 2013

I still consider the patch wrong due to the reason I gave, but I partially
agree with you here, so feel free to override me here.

I would suggest still keep an eye on this issue, and follow upstream
decision once they acknowledge and provide official fix.

On Oct 6, 2013 3:07 AM, "Mike McQuaid" notifications@github.com wrote:

@manphiz The same thing about libstdc++ was said about gcc and llvm-gcc
and years later many upstreams still haven't fixed their software to use
clang. If you're not sure it'll cause an issue I think we should err on the
side of patching. There's also a Mavericks 10.9 VM you can use for testing
on bot.brew.sh.


Reply to this email directly or view it on GitHub.

@mgol

This comment has been minimized.

Copy link

mgol commented Oct 23, 2013

This is probably nothing new but just to put it on the table: OS X 10.9 is out and the installation still fails.

@yoloseem

This comment has been minimized.

Copy link

yoloseem commented Oct 23, 2013

yes.. I, too, am in trouble with installing libmemcahced after upgrading to Mavericks.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 23, 2013

@manphiz @jacknagel @adamv @mistydemeo Anyone mind if I pull this as-is and we can work on a better patch later?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 23, 2013

@mzgol We may or may not pull this patch. I suggest putting pressure on upstream as they will be able to fix this quicker than us.

@@ -13,4 +13,30 @@ def install
system "./configure", "--prefix=#{prefix}"
system "make install"
end

def patches
if MacOS.version >= :mavericks

This comment has been minimized.

@manphiz

manphiz Oct 23, 2013

Contributor

Should also check if the compiler is clang. Shouldn't need patch (and will probably fail) when building using GNU GCC.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 23, 2013

Pulled. Please still nag upstream to make a proper fix.

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Oct 23, 2013

Oops, not fast enough. Will add compiler check in a new commit.

@manphiz

This comment has been minimized.

Copy link
Contributor

manphiz commented Oct 23, 2013

@MikeMcQuaid Oh OK, I saw you had added the check. Will shut up and sleep :)

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 23, 2013

@manphiz Please do :) Enjoy!

ehershey added a commit to ehershey/homebrew that referenced this pull request Apr 4, 2014

libmemcached: fix "tr1/cintypes" error on 10.9.
Closes Homebrew#22259.

Signed-off-by: Mike McQuaid <mike@mikemcquaid.com>

@Homebrew Homebrew locked and limited conversation to collaborators Feb 17, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.