-
Notifications
You must be signed in to change notification settings - Fork 411
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
CURL support for Bazel #402
Conversation
- Pull a specific version of CURL as a dep. - Add this version of CURL as a dep to the test of the CURL client. - CURL should not be a dependency in any other way, but may be built if you glob-build.
Codecov Report
@@ Coverage Diff @@
## master #402 +/- ##
=======================================
Coverage 94.74% 94.74%
=======================================
Files 175 175
Lines 7590 7591 +1
=======================================
+ Hits 7191 7192 +1
Misses 399 399
|
Remove BUILD file causing issues in cmake
- Need to define CURL_STATICLIB for imports - Add missing windows static libraries to link (advapi32, crypt32 and Normaliz)
@lalitb I got the build to work in bazel. It looks like there's some kind of runtime error in the CURL test for "POST" for windows/osx. I'm getting a non-zero exit code running the tests after I build locally. I'm going to take a deeper look when I can, but if you have any insight, it'd be appreciated. |
@jsuereth - I would instead suggest to remove the curl build/test for Windows, as we would like to have WinInet as the default http client for Windows. Either me/Max would work on this client. I will look into the osx failure, as it should work fine there. |
Disabling the unit test on windows is out-of-scope for this PR (I'm still working on that entire mechanism, trying to make sure we don't go down a path of madness). @lalitb I did get the CURL test working on windows CI, just not locally. I'll disable it in the future. For the Darwin build, it may be a similar issue to windows where the CURL |
Have fixed the MacOS test now - it was something to do with how loopback address subnet is configured in MacOS. |
Sorry for not being clear enough in my earlier comment. The fix is to change the non-existent IP address value from "127.0.0.X" to some random address "222.222.X.X". The reason was that loopback address subnet range is different in Linux and MacOS and so timeout test works in one, and fails in another. The new IP address won't be reachable in both the platforms, and so test will pass. Also reduced the timeout value to 2sec, to ensure that test won't take long time, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once the format issue is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This wires CURL into the BAZEL build, and enables the CURL http tests to also be run in the BAZEL build.
Fixes #390
Note: This does NOT provide CURL build in a consumable way for release, that will be done as part of a larger PR for enabling OT CPP releases to be consumed by downstream bazel users.