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

Windows adaptations and installation configuration #80

Conversation

ahmedyarub
Copy link
Contributor

Should be merged after OpenAPITools/openapi-generator#10326
Includes fixes for Windows and installation configuration.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 4, 2021
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 4, 2021
Ahmed Yarub Hani Al Nuaimi added 3 commits September 4, 2021 21:12
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 5, 2021
@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 cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 5, 2021
@ahmedyarub
Copy link
Contributor Author

I signed it

Ahmed Yarub Hani Al Nuaimi and others added 2 commits September 7, 2021 04:50
@ahmedalnuaimi
Copy link
Contributor

I signed it

@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 Sep 7, 2021
install(TARGETS ${pkgName} DESTINATION ${CMAKE_INSTALL_PREFIX})
else()
include(GNUInstallDirs)
install(TARGETS ${pkgName}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmake .. will fail in my env:

CMake Error at CMakeLists.txt:1566 (install):
  install TARGETS given no LIBRARY DESTINATION for shared library target
  "kubernetes".


CMake Error at CMakeLists.txt:1584 (export):
  export Export set "kubernetesTargets" not found.

After I changed the line 1655
to

install(TARGETS ${pkgName} DESTINATION ${CMAKE_INSTALL_PREFIX}

It works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can build and install on both Ubuntu 20.04 and Windows without any problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know. But on my env (Ubuntu 18.04 and default cmake), this change is needed. I think "DESTINATION" is required but new version of cmake can ignore it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested changes made and a new PR opened OpenAPITools/openapi-generator#10378

@ityuhui
Copy link
Member

ityuhui commented Sep 10, 2021

The build of libkubernetes.so will fail:

/usr/bin/ld: /usr/local/lib/libyaml.a(api.c.o): relocation R_X86_64_PC32 against symbol `yaml_realloc' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status
CMakeFiles/kubernetes.dir/build.make:19861: recipe for target 'libkubernetes.so' failed
make[2]: *** [libkubernetes.so] Error 1
CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/kubernetes.dir/all' failed
make[1]: *** [CMakeFiles/kubernetes.dir/all] Error 2
Makefile:129: recipe for target 'all' failed
make: *** [all] Error 2

After I rebuilt and installed libyaml to a shared library:

user@machine:~/libyaml/build# cmake .. -DBUILD_SHARED_LIBS=1
sudo make install

The libkubernetes.so can be built successfully.

@ahmedyarub
Copy link
Contributor Author

The build of libkubernetes.so will fail:

/usr/bin/ld: /usr/local/lib/libyaml.a(api.c.o): relocation R_X86_64_PC32 against symbol `yaml_realloc' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status
CMakeFiles/kubernetes.dir/build.make:19861: recipe for target 'libkubernetes.so' failed
make[2]: *** [libkubernetes.so] Error 1
CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/kubernetes.dir/all' failed
make[1]: *** [CMakeFiles/kubernetes.dir/all] Error 2
Makefile:129: recipe for target 'all' failed
make: *** [all] Error 2

After I rebuilt and installed libyaml to a shared library:

user@machine:~/libyaml/build# cmake .. -DBUILD_SHARED_LIBS=1
sudo make install

The libkubernetes.so can be built successfully.

Do you propose making a change here?

@ityuhui
Copy link
Member

ityuhui commented Sep 10, 2021

Yes. I suggest updating the readme and actions/workflow to build libyaml as a shared library.

cmake -DCMAKE_INSTALL_PREFIX=/usr/local -DBUILD_TESTING=OFF -DBUILD_SHARED_LIBS=1 .. 

@ahmedyarub
Copy link
Contributor Author

 -DBUILD_SHARED_LIBS=

Done

@ahmedyarub
Copy link
Contributor Author

Next I'll make some adaptations for Android, iOS, MacOS and possibly WASM!

@ityuhui
Copy link
Member

ityuhui commented Sep 14, 2021

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmedyarub, ityuhui

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

The pull request process is described 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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2021
@k8s-ci-robot k8s-ci-robot merged commit 9c7b114 into kubernetes-client:master Sep 14, 2021
@ityuhui
Copy link
Member

ityuhui commented Sep 14, 2021

@ahmedyarub Thanks to your contribution now the C client library supports Windows !

@ahmedyarub ahmedyarub deleted the ay/windows_installation_adaptaions2 branch September 14, 2021 06:39
@ahmedyarub
Copy link
Contributor Author

@ahmedyarub Thanks to your contribution now the C client library supports Windows !

Next I'll work on adaptations for Android, MacOS, iOS, and Emscripten

@ityuhui
Copy link
Member

ityuhui commented Sep 15, 2021

Hi @ahmedyarub

  • Supporting MacOS is useful !
  • But I am afraid that users rarely use the C client library on Android or iOS...
  • For the Emscripten, as far as I known, it is used to convert C code to asm.js, but what's the purpose because community has a Javascript client library (https://github.com/kubernetes-client/javascript)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

4 participants