allow inclusion in C programs #4608
Conversation
drewmiller
requested review from
btrotta,
chivee,
guolinke and
shiyu1994
as code owners
|
Thanks for this @drewmiller ! Is it possible to share a link to the C project you're working on that needs to link against LightGBM? No worries if not, we'll understand if it's proprietary. Also would you be willing to write up a feature request at https://github.com/microsoft/LightGBM/issues explaining why it would be valuable for C projects to be able to link against LightGBM directly, with specific examples of such projects if you have them? If this is a feature that people care about, we'll want to add a continuous integration job that tests that linking such a project works, to ensure that compatibility issues aren't accidentally introduced in the future. |
Unfortunately, it's proprietary.
Sure. |
|
Thanks for the great product, MS team. I've added a simple feature request, and I'm happy to help write a toy example for inclusion in a unit testing pipeline if helpful. I'm not super familiar with the code base yet, so I recognize my "trivial" commit may have cascading effects. However, this software would be more useful to me with this feature, so I thought I'd open the door with some proposed changes, and I'm happy to help bring them into existence however I can. That includes bulking out the feature request as necessary. Warm regards. |
|
Thanks very much for opening #4609. I think it's something we should consider supporting and should having tests for, but that work doesn't need to be done on this PR. Could you please merge the latest |
Done; although the CI seems to be hanging at |
This is a GitHub protection from miners: #4266 (comment). We have to click |
include/LightGBM/c_api.h
Outdated
| @@ -1321,7 +1328,9 @@ LIGHTGBM_C_EXPORT int LGBM_NetworkInitWithFunctions(int num_machines, | |||
| void* reduce_scatter_ext_fun, | |||
| void* allgather_ext_fun); | |||
|
|
|||
| #if defined(_MSC_VER) | |||
| #ifndef __cplusplus | |||
| #define THREAD_LOCAL /*!< \brief Thread local specifier no-op in C builds. */ | |||
There was a problem hiding this comment.
It seems C11 also has thread_local, refer to https://en.wikipedia.org/wiki/C11_(C_standard_revision)
There was a problem hiding this comment.
The follow-up changes take advantage of _Thread_local from C11. They also make the C API header compatible with ANSI C. If the changes to the comments disrupt doc generation, they could be rolled back in favor of abandoning support for C versions prior to C99. The attached test.c.gz can be gunzipped in c_api_test/ and used to test both code generation and compilation with the below:
Code generation:
gcc test.c -ansi -I../../include/ -E - should exclude inline and _Thread_local
gcc test.c -std=c99 -I../../include/ -E - should include inline and exclude _Thread_local
gcc test.c -std=c11 -I../../include/ -E - should include inline and _Thread_local
g++ test.c -I../../include/ -E - should include inline and thread_local
Remove the -E switch on the above to confirm compilation for each setting.
|
|
|
@shiyu1994 can you take a look on this? |
|
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: success ✔️. |
|
/gha run r-solaris Workflow Solaris CRAN check has been triggered! 🚀 solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-56fa0cb3567643248793465ecb7b2a5f |
There was a problem hiding this comment.
@drewmiller Thanks for your contribution. The changes LGTM.
|
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
Including "c_api.h" from programs compiled with C compilers errors due to required inclusion of C++ headers. These edits allow c_api.h to be included directly in C programs.