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

Refactor cmake scripts #75

Closed

Conversation

ahmedyarub
Copy link
Contributor

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 :)

Ahmed Yarub Hani Al Nuaimi added 2 commits August 16, 2021 09:37
(cherry picked from commit 84b387e)
(cherry picked from commit 7de3acb)
@k8s-ci-robot
Copy link
Contributor

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

📝 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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 16, 2021
@k8s-ci-robot
Copy link
Contributor

[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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 16, 2021
@ityuhui
Copy link
Member

ityuhui commented Aug 16, 2021

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
Copy link
Contributor Author

Sure I'll do that later today

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 16, 2021
@ityuhui
Copy link
Member

ityuhui commented Aug 16, 2021

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
Copy link
Contributor Author

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 #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
Copy link
Member

ityuhui commented Aug 22, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Added a new PR OpenAPITools/openapi-generator#10249
@ityuhui

@ahmedyarub
Copy link
Contributor Author

Replaced with #78

@ahmedyarub ahmedyarub closed this Aug 30, 2021
@ahmedyarub ahmedyarub deleted the refactor_cmake_scripts branch August 30, 2021 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants