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

runtime api enforcement #204

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open

runtime api enforcement #204

wants to merge 31 commits into from

Conversation

Xavrax
Copy link
Contributor

@Xavrax Xavrax commented Jan 23, 2025

feat: optional runtime api selection

Add PUBNUB_NTF_RUNTIME_SELECTION flag that allows to select API type during the runtime

Copy link
Contributor

@parfeon parfeon left a comment

Choose a reason for hiding this comment

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

Do we need to change something according to the review?

CMakeLists.txt Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to compare with make/common/source_files.mk, make/posix_source_files.mk, and windows_source_files.mk? I've noticed that some source files are missing and some placed for wrong environment.

Here is some:
${CMAKE_CURRENT_LIST_DIR}/lib/pubnub_dns_codec.c and ${CMAKE_CURRENT_LIST_DIR}/lib/sockets/pbpal_adns_sockets.c - this one used only in Callback API (at least according to the code)
${CMAKE_CURRENT_LIST_DIR}/core/pubnub_alloc_std.c - present in both if and else branches. Should we just move it to the core files list declaration?
${CMAKE_CURRENT_LIST_DIR}/core/pubnub_memory_block.c - for some reason specified only for Callback API but it is general API (at least according to the code)
${CMAKE_CURRENT_LIST_DIR}/posix/pubnub_version_posix.c - can't be added as is to OS_SOURCEFILES because there exists another version file for WITH_CPP: ${CMAKE_CURRENT_LIST_DIR}/cpp/pubnub_version_posix.cpp. Similar situation with version file for Windows which also depends from WITH_CPP

${CMAKE_CURRENT_LIST_DIR}/lib/sockets/pbpal_sockets.c - I've noticed that we don't have this in cmake settings for non-openssl build (at least it was in original make files)
${CMAKE_CURRENT_LIST_DIR}/posix/pbpal_posix_blocking_io.c - is missing for non-openssl POSIX build

${CMAKE_CURRENT_LIST_DIR}/core/pubnub_ssl.c, ${CMAKE_CURRENT_LIST_DIR}/openssl/pbpal_connect_openssl.c, ${CMAKE_CURRENT_LIST_DIR}/openssl/pbpal_openssl.c, ${CMAKE_CURRENT_LIST_DIR}/openssl/pbpal_openssl_blocking_io.c - looks like this one also missing for openssl build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ues sure.
I found that there are few differences between makefiles and cmake - probably I will need some help with that.

#if defined(PUBNUB_NTF_RUNTIME_SELECTION)
if (PNA_SYNC == pb->api_policy) {
add_heartbeat_in_progress(pb->thumperIndex);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have early exit from the function if pb->api_policy is PNA_CALLBACK and #if defined(PUBNUB_NTF_RUNTIME_SELECTION) by moving check to the function start? It will let us have only one check and remove similar code from the end of this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm.. I didn't think about it.
There were so many places that I changes automatically so probably your solution seems to be better.

pubnub_subloop_t* pubnub_subloop_define(pubnub_t* p,
#else
pubnub_subloop_t* pubnub_callback_subloop_define(pubnub_t* p,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really care about separate name if enforcement enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because C mangling doesn't allow me to have the same name of function twice - however I might be wrong because of the lack of knowladge.

@Xavrax
Copy link
Contributor Author

Xavrax commented Mar 6, 2025

I will add those changes in the meanwhile.

@Xavrax
Copy link
Contributor Author

Xavrax commented Mar 28, 2025

I'm going to remove USE_SYNC_API because it breaks the compatibility.
It will be better to just add the api reinforcement as the additional flag that just requires USE_CALLBACK_API (same for makefiles)

@Xavrax
Copy link
Contributor Author

Xavrax commented Mar 28, 2025

${CMAKE_CURRENT_LIST_DIR}/lib/sockets/pbpal_sockets.c - I've noticed that we don't have this in cmake settings for non-openssl build (at least it was in original make files)

It is added when there is no SSL and no MbedTSL

${CMAKE_CURRENT_LIST_DIR}/posix/pbpal_posix_blocking_io.c - is missing for non-openssl POSIX build

As above

${CMAKE_CURRENT_LIST_DIR}/core/pubnub_ssl.c, ${CMAKE_CURRENT_LIST_DIR}/openssl/pbpal_connect_openssl.c, ${CMAKE_CURRENT_LIST_DIR}/openssl/pbpal_openssl.c, ${CMAKE_CURRENT_LIST_DIR}/openssl/pbpal_openssl_blocking_io.c - looks like this one also missing for openssl build

Only pubnub_ssl was missing and I've added it.

@parfeon parfeon self-requested a review March 28, 2025 12:07
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