-
Notifications
You must be signed in to change notification settings - Fork 4k
[C-API] Avoid C++ includes from arrow.h when including c_api.h #6973
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
Conversation
@microsoft-github-policy-service agree |
jameslamb
left a comment
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.
|
Sorry for clicking |
|
/AzurePipelines run |
|
/AzurePipelines run |
|
/AzurePipelines run |
|
/AzurePipelines run |
borchero
left a comment
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.
I honestly don't know if this is the most ergonomic way to do this since I have too little experience with C(++) codebases 😅 as long as it works, I don't oppose it though
|
An SSL error? I don't understand why this happened🧐 |
|
There are hundreds of network requests made for each run of LightGBM CI. It's rare but not THAT rare for some of them to fail due to temporary network disruptions. I've retriggered the failed jobs, we should expect them to pass. Very very very unlikely those failures were related to your changes here. |
|
Ah I see. Thank you |
jameslamb
left a comment
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.
Thanks very much for the contribution!
I think we should have a smoke test in this repo compiling a small C program linking to lib_lightgbm.so and using its C API. That'd help catch things like this in the future and give us more confidence in solutions like this one.
That's tracked in #4609
But with all other CI passing here, I think this is safe to merge. Thanks very much for the contribution, we'd love to have you come contribute more in the future!
I agree that we should have a smoke test written in pure C and exercises the C API. I’ll work on this when I have some free time. Thanks as well for keeping an eye on this PR! |
I've got the similar problem like #6839, while compling my C code. According to the issue, I found out that Ten0/lightgbm-rs@243322e is a good solution, but not so robust, so I made this fix.