kubernetes-client / c

Official C client library for Kubernetes
Apache License 2.0
144 stars 46 forks source link

Refactor cmake scripts #75

Closed ahmedyarub closed 2 years ago

ahmedyarub commented 3 years ago

1- Use Glob to (greatly) lower CMake's file size 2- Build one of the examples using CMake (later I'll add CMake scripts for building the other examples) 3- Move unit-tests and plugins outside the main source folder to avoid picking them up by GLOB

This has been fully tested. I'm open to any suggestions :)

k8s-ci-robot commented 3 years ago

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
k8s-ci-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ahmedyarub To complete the pull request process, please assign ityuhui after the PR has been reviewed. You can assign the PR to them by writing /assign @ityuhui in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kubernetes-client/c/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
ityuhui commented 3 years ago

Very cool ! Migrating makefile to CMake for examples is very useful !

But c/kubernetes/CMakeLists.txt bases on the template file: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/C-libcurl/CMakeLists.txt.mustache, (actually the .c and .h lists are generated in CMakeLists.txt)

And I don't recommend moving "unit-test" and "plugins" directories outside the main source folder kubernetes because

Could you please add your code to the template file ?

ahmedyarub commented 3 years ago

Sure I'll do that later today

ityuhui commented 3 years ago

libwebsockets, yaml and examples are not needed by https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/C-libcurl/CMakeLists.txt.mustache. They are only used by kubernetes client C.

ahmedyarub commented 3 years ago

libwebsockets, yaml and examples are not needed by https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/C-libcurl/CMakeLists.txt.mustache. They are only used by kubernetes client C.

I went ahead and downloaded all the requirements for regenerating the C client for k8s, trying to understand how the CMake script were customized exactly by the generator, and it looks like additions were made manually in a PR like this https://github.com/kubernetes-client/c/pull/9. In this PR you can see that some .c files were added manually to the SRCS variable since the generator does not know about these files. I was trying to do exactly the same. So the question now is: what is the accepted solution for customizing the template? by adding changes manually to this repo? Or may I suggest including optional CMake scripts in multiple places (ex: in the beginning of the file, before find_package(), at the end of the file, etc...) so that each generated client can add its own scripts without editing the original template?

ityuhui commented 3 years ago

For this PR: 1- Use Glob to (greatly) lower CMake's file size 2- Build one of the examples using CMake (later I'll add CMake scripts for building the other examples) 3- Move unit-tests and plugins outside the main source folder to avoid picking them up by GLOB

I only suggest addressing the # 2 (Build one of the examples using CMake) for now. The # 1 and # 3 are not needed because it will introduce complexity and conflict with openapi-generator/c-libcurl

But I also agree the solution including optional CMake scripts in multiple places is a good solution because this repo will not need update manually each time re-generating the C client. e.g. add below files ( config/kube_config_model.c config/kube_config_yaml.c config/kube_config_util.c config/kube_config.c config/incluster_config.c config/exec_provider.c config/authn_plugin/authn_plugin_util.c config/authn_plugin/authn_plugin.c watch/watch_util.c websocket/wsclient.c websocket/kube_exec.c)

So it's up to you: if you want to merge this PR quickly, then only need address # 2; if you want to re-organize the CMakefiles in upstream repo (openapi-generator/c-libcurl), please try it. Thank you.

ahmedyarub commented 3 years ago

For this PR: 1- Use Glob to (greatly) lower CMake's file size 2- Build one of the examples using CMake (later I'll add CMake scripts for building the other examples) 3- Move unit-tests and plugins outside the main source folder to avoid picking them up by GLOB

I only suggest addressing the # 2 (Build one of the examples using CMake) for now. The # 1 and # 3 are not needed because it will introduce complexity and conflict with openapi-generator/c-libcurl

But I also agree the solution including optional CMake scripts in multiple places is a good solution because this repo will not need update manually each time re-generating the C client. e.g. add below files ( config/kube_config_model.c config/kube_config_yaml.c config/kube_config_util.c config/kube_config.c config/incluster_config.c config/exec_provider.c config/authn_plugin/authn_plugin_util.c config/authn_plugin/authn_plugin.c watch/watch_util.c websocket/wsclient.c websocket/kube_exec.c)

So it's up to you: if you want to merge this PR quickly, then only need address # 2; if you want to re-organize the CMakefiles in upstream repo (openapi-generator/c-libcurl), please try it. Thank you.

I will reorganize the CMake files and create new PR's within the next days. Thank you.

ahmedyarub commented 3 years ago

Added a new PR https://github.com/OpenAPITools/openapi-generator/pull/10249 @ityuhui

ahmedyarub commented 2 years ago

Replaced with https://github.com/kubernetes-client/c/pull/78