Added print callback/redirect support to the C library to route C prints to glog in C++. #63

adamshapiro0 · 2024-08-20T21:58:14Z

New Features

Added optional user print callback to C library

Changes

Route all debug and error prints from the C library through glog in C++, rather than printing directly to stderr

Fixed

Fixed deprecated upload/download artifact actions in CI script

c/src/point_one/polaris/polaris.c

…fined.

…e app.

anathan · 2024-10-19T23:49:37Z

c/src/point_one/polaris/polaris.c

@@ -236,11 +282,18 @@ void Polaris_Free(PolarisContext_t* context) { CloseSocket(context, 1); }

 /******************************************************************************/
 void Polaris_SetLogLevel(int log_level) {
-#if !defined(POLARIS_NO_PRINT)
+#if !POLARIS_NO_PRINT


are you sure we dont want ifndef....this just invest the value of POLARIS_NO_PRINT (which i guess by default is 0?)

This is a little more flexible in practice. !defined(POLARIS_NO_PRINT) would disable prints if the POLARIS_NO_PRINT macro is defined, no matter what its value is.

If you don't use defined() and just use the macro in an expression like !POLARIS_NO_PRINT and the macro is not defined, the compiler will treat it as 0. So in this case you get:

Don't define POLARIS_NO_PRINT at all -- Prints enabled

#define POLARIS_NO_PRINT 0 -- Prints enabled

#define POLARIS_NO_PRINT 1 -- Prints disabled

This makes it a little easier to turn prints on/off in a CMakeLists.txt file for example by having it change the value it uses in a -DPOLARIS_NO_PRINT=${foo} statement instead of having to remove the line entirely.

c/src/point_one/polaris/polaris.c

anathan · 2024-10-19T23:51:26Z

c/src/point_one/polaris/polaris.c

+  if (str_length != 0) {
+    str[str_length++] = '\0';
+    if (__print_callback) {
+      __print_callback("polaris.c", __LINE__, POLARIS_LOG_LEVEL_TRACE, str);


if using line, we should use FILE

the above should be the c style thing with two underscore in front and behind, github is formatting it instead lol

In theory, yes. We did this just to simplify/shorten the print since __FILE__ always includes the full path to the file (src/...) and this is the only source file in the repo that prints anything. Unfortunately there's no macro to get just the filename, and you can't do string manipulation in the pre-processor.

I can switch it to __FILE__ if you think that's better?

anathan · 2024-10-19T23:52:03Z

c/src/point_one/polaris/polaris.c

 }
 #endif

 /******************************************************************************/
-#if !defined(P1_NO_PRINT) && defined(POLARIS_USE_TLS)
+#if !P1_NO_PRINT && POLARIS_USE_TLS


See my explanation above. This makes it a lot easier to set these macros in CMake or Bazel by changing the value of a -D statement instead of having to actually conditionally remove the -D altogether.

anathan · 2024-10-19T23:54:08Z

examples/simple_polaris_client.cc

+DEFINE_string(
+    polaris_api_hostname, "",
+    "Specify an alternate hostname to use when connecting to the Polaris "
+    "authentication API server. If blank, use the default hostname.");


we should add to this comment that changes this is almost never necessary (for the public)

@anathan how's this?

DEFINE_string( polaris_api_hostname, "", "Specify an alternate hostname to use when connecting to the Polaris " "authentication API server. Specifying a custom API hostname is not " "common, and is intended primarily for development purposes. If blank, use " "the default hostname.");

anathan · 2024-10-19T23:54:26Z

examples/simple_polaris_client.cc

+DEFINE_string(
+    polaris_hostname, "",
+    "Specify an alternate hostname to use when connecting to the Polaris "
+    "corrections network. If blank, use the default hostname.");


hint that is useful for regional endpoints in the EU and APAC.

@anathan Is this clear, or would you suggest anything else?

DEFINE_string( polaris_hostname, "", "Specify an alternate hostname to use when connecting to the Polaris " "corrections network. This may be useful when connecting to a regional " "endpoint in the EU or APAC. If blank, use the default hostname.");

anathan

my only nit is that it makes it so we have to print debug prints as lines, which introduces overhead in some embedded cases where you may be printing out as your loop goes. This is overall though a better structure.

adamshapiro0 · 2024-10-23T15:17:24Z

my only nit is that it makes it so we have to print debug prints as lines, which introduces overhead in some embedded cases where you may be printing out as your loop goes. This is overall though a better structure.

@anathan Not totally sure I follow the concern here? You can still print from within a loop if you want. Do you have an example?

v1 of these actions was marked deprecated in June 2024, and can no longer be used as of Oct 2024. https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/

adamshapiro0 added 2 commits August 20, 2024 09:29

Formatting cleanup.

dfae49e

Organized print macros to allow control at any verbosity level.

de79280

adamshapiro0 requested a review from anathan August 20, 2024 21:58

adamshapiro0 self-assigned this Aug 20, 2024

anathan reviewed Aug 20, 2024

View reviewed changes

c/src/point_one/polaris/polaris.c Show resolved Hide resolved

adamshapiro0 added 8 commits August 21, 2024 09:06

Added print callback support.

be9c328

Include filename and line in the callback.

bd68bd7

Redirect prints from the C library to glog in C++.

6fc1b7c

Fixed extra newline in data trace printout.

139a3fe

Fixed VLOG_IS_ON definition when glog is disabled.

c96df20

Explicitly separate P1_NO_PRINT in polaris_client.cc.

80214d7

Treat macro definition P1_x=0 as false instead of just checking if de…

8a7aceb

…fined.

Fixed clang-format preprocessor indenting.

fee4425

adamshapiro0 force-pushed the print-hook branch from add2df5 to fee4425 Compare August 21, 2024 13:06

Added hostname command line arguments to simple_polaris_client exampl…

4d44ff1

…e app.

anathan reviewed Oct 19, 2024

View reviewed changes

c/src/point_one/polaris/polaris.c Show resolved Hide resolved

anathan reviewed Oct 19, 2024

View reviewed changes

anathan approved these changes Oct 19, 2024

View reviewed changes

adamshapiro0 added 2 commits October 23, 2024 11:22

Clarified --polaris-hostname and --polaris-api-hostname usage.

cc022af

Updated upload-artifact and download-artifact actions.

bd16cbc

v1 of these actions was marked deprecated in June 2024, and can no longer be used as of Oct 2024. https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/

adamshapiro0 mentioned this pull request Oct 23, 2024

Creates codeql.yml #66

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added print callback/redirect support to the C library to route C prints to glog in C++. #63

Added print callback/redirect support to the C library to route C prints to glog in C++. #63

adamshapiro0 commented Aug 20, 2024 •

edited

Loading

anathan Oct 19, 2024

adamshapiro0 Oct 23, 2024

anathan Oct 19, 2024

anathan Oct 19, 2024

adamshapiro0 Oct 23, 2024

anathan Oct 19, 2024

adamshapiro0 Oct 23, 2024

anathan Oct 19, 2024

adamshapiro0 Oct 23, 2024

anathan Oct 19, 2024

adamshapiro0 Oct 23, 2024

anathan left a comment

adamshapiro0 commented Oct 23, 2024

Added print callback/redirect support to the C library to route C prints to glog in C++. #63

Are you sure you want to change the base?

Added print callback/redirect support to the C library to route C prints to glog in C++. #63

Conversation

adamshapiro0 commented Aug 20, 2024 • edited Loading

New Features

Changes

Fixed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anathan left a comment

Choose a reason for hiding this comment

adamshapiro0 commented Oct 23, 2024

adamshapiro0 commented Aug 20, 2024 •

edited

Loading