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

CMake External Project Fixes #173

Merged
merged 3 commits into from
Jan 4, 2024

Conversation

tomadamatkinson
Copy link
Contributor

I tested the new CMake branch. The build works great but I ran into some issues when integrating PubNub into an external CMake project

Demo project is set up as shown

image

CMake build fails on missing symbols and missing headers

PR includes fixes for symbols and uses the preferred target based headers target_include_directories. To do this i moved the source file list creation above the target add_library and moved the header includes below the target creation.

@Xavrax
Copy link
Contributor

Xavrax commented Jan 3, 2024

Thank you so much! I will review this and merge tomorrow when I'll start my work.

@tomadamatkinson tomadamatkinson mentioned this pull request Jan 3, 2024
Copy link
Contributor

@Xavrax Xavrax left a comment

Choose a reason for hiding this comment

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

LGTM!

@Xavrax
Copy link
Contributor

Xavrax commented Jan 4, 2024

Quick question to you @tomadamatkinson:
Out contribution CI is bad - or even not exists.
My question is - do you prefer to merge that into the base branch (and be co-author of the whole branch) or do you want to push this directly into the master (which needs a little bit more things to do)?

@tomadamatkinson
Copy link
Contributor Author

tomadamatkinson commented Jan 4, 2024

Thanks for reviewing!

My question is - do you prefer to merge that into the base branch (and be co-author of the whole branch) or do you want to push this directly into the master (which needs a little bit more things to do)?

@Xavrax completely up to you. My goal is to help improve the CMake integration so that I can use it in an upcoming C++ project. I don't mind in what order contributions merge or even if my proposed solutions aren't excepted. As long as the integration works as a dependency in a parent build system then I am happy :)

As the CMake integration is fairly new and works in its current state we could merge #172 first and then re-target #173 and #174 to main? I'm open to any suggestions

I guess a better question is: How does the release cycle work on this project. Do others depend on the head of main or can we merge in development features? No one should be using the CMake integration yet so it is fairly safe to merge

@Xavrax
Copy link
Contributor

Xavrax commented Jan 4, 2024

@tomadamatkinson Easier for me is just to merge this and the cpp branch into the cmake branch and mention you as the co-author (I don't have to play with the branches and forces you play with the PRs :/ ).

About the release cycle - We release when it is possible. Since we wanted to provide very first step of the CMake support, it will be released soon.
At the moment, I'm wondering if I'll be able to finish the CPP changes as soon as possible, and then just release everything to unlock CMake compilations and wait for feedback.

TBH with you - I love that you came here and became the first user of our CMake build system before it has been even released. Your feedback and works are very helpful! ;d

@tomadamatkinson
Copy link
Contributor Author

@Xavrax No problem. Glad i could help!

I'll follow your lead. Feel free to merge :)

@Xavrax Xavrax merged commit aa6149b into pubnub:feat/cmake-build-system Jan 4, 2024
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.

None yet

2 participants