search

conda-forge / icu-feedstock

A conda-smithy repository for icu.
BSD 3-Clause "New" or "Revised" License
1 stars 21 forks source link

added toolchain support #8

Closed gillins closed 8 years ago

gillins commented 8 years ago

This adds support for the toolchain which fixes incorrect linking to the C++ runtime libs on OSX.

ping @jakirkham

conda-forge-linter commented 8 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

msarahan commented 8 years ago

Cool, thanks @gillins

jakirkham commented 8 years ago

Thanks @gillins. I really appreciate you taking the lead on this issue. Are there any more cases that we know of that need to switch?

jakirkham commented 8 years ago

Also LGTM. 😄

gillins commented 8 years ago

libtiff (has a C++ extension), gdal, qt (and maybe pyqt4 - I haven't checked) are the ones I know about. But I'm sure there will be others....

jakirkham commented 8 years ago

Looks like Windows Python 2.7 64-bit is failing. Not sure that why this change would cause that. Is this a known issue? Do we need to open one?

gillins commented 8 years ago

Same thing in kealib (https://github.com/conda-forge/kealib-feedstock/pull/4). Sounds like @msarahan has it under control :smile:

jakirkham commented 8 years ago

Thanks for the list. Thanks for the cross-ref too.

Yeah, Qt uses C++ so anything using Qt directly (PyQt, PySide, etc.) will also be in C++.

Though it is important to note that the issue is when C++ objects cross the boundary between a libstdc++-based library and a libc++-based library (Windows anyone?). If we have libraries linking to Qt that are linking to libraries using libc++, then we definitely have a problem. If not, we don't have any issue.

gillins commented 8 years ago

I checked - Qt links against libstdc++ only. So my guess is that it won't work well with these other libs now that we have moved them to libc++.

jakirkham commented 8 years ago

So, passing C objects across boundaries is ok though. This is what would be happening between Python and Qt. I expect we should be ok, but more investigation is certainly welcome.

ccordoba12 commented 8 years ago

Qt4 doesn't use ICU, but Qt5 will. Since Qt5 links against libc++, so this change is welcomed :-)

However, Boost uses ICU. But I think it's linking againts libc++ now, or is that not the case?

jakirkham commented 8 years ago

Glad to have you in the discussion, @ccordoba12. Was thinking of pinging you, but realized you are already watching the repo. 😄

Qt4 doesn't use ICU, but Qt5 will.

That was my recollection. Sorry, my comments above were a bit vague.

Since Qt5 links against libc++, so this change is welcomed :-)

Ah, ok. That's great news.

Just for my own curiosity, is there a reason libc++ is preferred?

However, Boost uses ICU. But I think it's linking againts libc++ now, or is that not the case?

That is correct. 😄

ccordoba12 commented 8 years ago

Just for my own curiosity, is there a reason libc++ is preferred?

Qt 5.6 needs C++11 support to compile one of its modules (QWebEngine), and the very ancient gcc that comes with XCode (4.2.1, I think) doesn't support it. So we had to use clang to compile it and so Qt 5 is now linked against libc++ :-)

jakirkham commented 8 years ago

Yep, that makes perfect sense. Thank for the details.

jakirkham commented 8 years ago

Sounds like we are happy with this change, correct? Should we wait to fix AppVeyor somehow or should we go ahead with this as is and rerun AppVeyor later?

Also, looks like this affects libxml2 and cairo directly.

jakirkham commented 8 years ago

Thanks @ocefpaf for restarting.

LGTM. Any objections on merging?

jakirkham commented 8 years ago

Guessing there are no complaints. Thanks @gillins.

jakirkham commented 8 years ago

Oops, forgot to ask that when unpin conda-build, which is causing this issue ( https://github.com/conda-forge/icu-feedstock/issues/9 ). Sorry about that. See this PR ( https://github.com/conda-forge/icu-feedstock/pull/10 ).

jakirkham commented 8 years ago

I have changed the OS X package build number 3's label on the icu package from "main" to "broken". It does not download by default any more, but is still available for inspection.