Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use conda-forge dependencies #8

Merged
merged 4 commits into from
Jul 16, 2016
Merged

Use conda-forge dependencies #8

merged 4 commits into from
Jul 16, 2016

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Jun 20, 2016

Fixes #7

Tries to use conda-forge dependencies as much as possible instead of trying to use CMake internally built dependencies. This should speed up the build, improve portability, and fix various subtle issues like not having certs for curl.

This adds some dependencies, but some of them like bzip2 are not here at conda-forge yet. There are more dependencies we should add like expat and xz, which don't exist on Windows yet. There are some other dependencies like jsoncpp and libarchive, which are simply not packaged yet. (EDIT: everything is packaged on UNIX and added as a dependency) Finally, we still need to address pinnings here and probably add a test for using SSL. Other questions include whether we should build this with Qt support as that is an option and could be desirable in some cases.

Feedback welcome on the progress thus far and any of these points that still need to be addressed.

cc @patricksnape @msarahan

@conda-forge-linter
Copy link

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.

@jakirkham jakirkham changed the title WIP: Use conda-forge dependencies Use conda-forge dependencies Jun 28, 2016
@jakirkham
Copy link
Member Author

Interesting, appears this uses ncurses too.

@jakirkham
Copy link
Member Author

So, this is a pretty subtle point, but cmake looks for ncurses. However, it fails to find the cbreak function defined. Would suspect this is because it is not searching tinfo too. Need to figure out why and how to fix it.

@jakirkham
Copy link
Member Author

After investigating the tinfo situation more closely, I think this is a false positive. Basically, cmake use to state it was searching tinfo for cbreak, but they decided to drop that with commit ( Kitware/CMake@b4005a3 ). As a result, it doesn't say it is searching for tinfo, but still tries to find and link against it for cbreak if needed. This still find cbreak, but simply doesn't announce it. The behavior can be verified by noting ccmake's linkage which includes tinfo.

@jakirkham
Copy link
Member Author

This is ready to go on my end.

@patricksnape
Copy link

Did we confirm this fixes the SSL issues?

@jakirkham
Copy link
Member Author

Not yet. Though curl is tested for downloading via SSL. That being said, I think we should be able to come up with a simple test and add it here.

--system-libarchive \
--system-liblzma \
--system-zlib \
--no-qt-gui \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider adding the following:
--parallel=${CPU_COUNT}
--
-DCMAKE_BUILD_TYPE:STRING=Release \

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CPU_COUNT value tends to be inaccurate on the CIs. We have explored setting this through MAKEFLAGS in PR ( conda-forge/conda-forge-build-setup-feedstock#1 ). However, that turns out to be more trouble than it is worth and removal is thus being proposed in PR ( conda-forge/conda-forge-build-setup-feedstock#18 ). Ideally we will override CPU_COUNT so that it can be used responsibly, but we will need to get some other players on-board first.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this issue ( conda/conda-build#1117 ) for overriding CPU_COUNT.

@blowekamp
Copy link
Contributor

Anything I could to to help with this patch?

@jakirkham
Copy link
Member Author

If you can come up with a test, for https that would be greatly appreciated, @blowekamp.

@blowekamp
Copy link
Contributor

I took a stab at it here: blowekamp@c41ba77

HTH

@jakirkham
Copy link
Member Author

Thanks @blowekamp. I made some tweaks to it, but have left you as author on that commit. It seems to work ok for me locally. Let's see what the CIs think.

@jakirkham
Copy link
Member Author

Seems it passes that test.

@pelson
Copy link
Member

pelson commented Jul 16, 2016

This looks good to me. @blowekamp - are you interested in being added as a maintainer here?

@blowekamp
Copy link
Contributor

Yes, I'll continue to help out with this recipe.

@jakirkham
Copy link
Member Author

Thanks @blowekamp. Was thinking of asking the same thing. Have added you on. An email should be sent to you later that will ask you to join conda-forge (after this is merged).

@jakirkham
Copy link
Member Author

@pelson, I was going to ask @msarahan to review, but if he is too busy I think this is ready to go. Nothing to surprising happened here and it has been tested.

@pelson pelson merged commit 71a7d7f into conda-forge:master Jul 16, 2016
@jakirkham jakirkham deleted the use_conda_deps branch July 16, 2016 17:36
@jakirkham
Copy link
Member Author

Alright, @blowekamp @patricksnape the Linux and OS X builds of this are out in the wild. Windows builds are pending though there was no actionable change for them with this addition. You should be good to go. Please let me know if you run into any issues. Thanks for raising this and helping me resolve it. 😄

@patricksnape
Copy link

patricksnape commented Jul 18, 2016

@jakirkham Still seeing this error for the OpenCV3 recipe.

Edit: Seems I'm not getting this build actually - any idea why I'm pulling build 3 rather than build 4?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMake needs OpenSSL
5 participants