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

feat: support cmake compile #124

Merged
merged 7 commits into from
Sep 20, 2023
Merged

feat: support cmake compile #124

merged 7 commits into from
Sep 20, 2023

Conversation

JayInnn
Copy link
Contributor

@JayInnn JayInnn commented Sep 6, 2023

New Feature:

  1. add CMakeLists.txt and related files to support cmake compile;
  2. add README.md to explain how to use and note the limitation about this PR.
  3. test cmake on Centos and Ubuntu OS.

@JayInnn
Copy link
Contributor Author

JayInnn commented Sep 6, 2023

Testing:
image
image

@wu-sheng
Copy link
Member

wu-sheng commented Sep 6, 2023

@wbpcode Does this fit envoy side requirements?

@wbpcode
Copy link
Contributor

wbpcode commented Sep 7, 2023

Hmmm, additional build system support for the cpp2sky won't effect envoy. So from envoy's point, I think it's OK.

But from the cpp2sky self's point, is these submodule necesssary? If there better way to manager these deps?

@JayInnn
Copy link
Contributor Author

JayInnn commented Sep 7, 2023

But from the cpp2sky self's point, is these submodule necesssary? If there better way to manager these deps?

I think I need to explain the reason that this project have added the submodules.
Reason one: this feat support several ways to compile the project, by using cmake. FetchContent(default way) function will download deps from network, so the submodules has no effects. But add_subdirectory & find_package function need the codes of the deps, which was compiled and installed.

Reason two: when I did this task, due to the grpc archive, I could not figure out the tag of grpc. Therefore I added the submodule and checkouted to the specified branch and noted the version in README.md. As for other deps, perhaps, these should be marked.

@wbpcode
Copy link
Contributor

wbpcode commented Sep 13, 2023

Got it, thanks for you explanation. But basically, IMO, I can only accept the FetchContent one because I want to keep the repo clean.

@JayInnn
Copy link
Contributor Author

JayInnn commented Sep 13, 2023

Got it, thanks for you explanation. But basically, IMO, I can only accept the FetchContent one because I want to keep the repo clean.

I understand your considerations. So, in the latest commits, I hava deleted the submodules and updated README.md. BTW, different ways for cmake compile hava also kept in the codes :)

@JayInnn JayInnn closed this Sep 13, 2023
@JayInnn JayInnn reopened this Sep 13, 2023
@wbpcode
Copy link
Contributor

wbpcode commented Sep 19, 2023

Thanks for you update. Now we will have a clean tree.
Would you mind add the build to the ci job then we can ensure this cmake building basically ok? Thanks.

(And I am so sorry for the delay, I check your new messaage on my phone and forget to respond.

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #124 (1b43a76) into main (90da327) will increase coverage by 67.96%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           main     #124       +/-   ##
=========================================
+ Coverage      0   67.96%   +67.96%     
=========================================
  Files         0       26       +26     
  Lines         0     1030     +1030     
=========================================
+ Hits          0      700      +700     
- Misses        0      330      +330     

see 26 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@wu-sheng wu-sheng added this to the 0.6.0 milestone Sep 19, 2023
@@ -31,7 +31,29 @@ cc_binary(
],
)
```
#### Cmake
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#### Cmake
#### CMake

@JayInnn
Copy link
Contributor Author

JayInnn commented Sep 19, 2023

Would you mind add the build to the ci job then we can ensure this cmake building basically ok? Thanks.

My pleasure!
I have added cmake-building ci in the workflows. The new feature will work well, i think :)

@JayInnn
Copy link
Contributor Author

JayInnn commented Sep 20, 2023

image
Some errors ocurred, but the cmake-buiding ci was successful. Maybe, the maintainer should check the error.

@wu-sheng
Copy link
Member

@wbpcode It seems the coverage fails.

@wbpcode
Copy link
Contributor

wbpcode commented Sep 20, 2023

Weird, it just success yesterday. Let's try it again.

Copy link
Contributor

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM. friendly ping @wu-sheng for merge or second review.

@wu-sheng wu-sheng merged commit c2bb8b4 into SkyAPM:main Sep 20, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants