-
Notifications
You must be signed in to change notification settings - Fork 833
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
implement ShowExtendedSystemInfo() in version.c #6149
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start. It'd be better if this would be a bit more generic so that other platforms (ARM, x86, etc.) would make use of this function as well. Perhaps adding in a call to this for our binaries used in the examples would help when the support team receives a log.
src/version.c
Outdated
#include "sdkconfig.h" | ||
const char* TAG = "Version"; | ||
|
||
int ShowExtendedVersionInfo() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put this outside of the defined(WOLFSSL_ESPIDF)? That way other platforms could use it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to call the function ShowExtendedSystemInfo
since you're printing out clock speeds and capabilities of the part as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both good suggestions. Function is now ShowExtendedSystemInfo()
and a bit more friendly to other platforms.
src/version.c
Outdated
ESP_LOGI(TAG, "Extended Version and Platform Information"); | ||
|
||
#if defined(WOLFSSL_MULTI_INSTALL_WARNING) | ||
ESP_LOGI(TAG, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use a macro to print the lines. Then have a section above like so:
#if defined(WOLFSSL_ESPIDF)
#define VERSION_PRINTF(...) ESP_LOGI(TAG, ...)
#else
#define VERSION_PRINTF(...)
// or #warn "No printing capability
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't be surprised if we already have a macro like this somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may, but I put in a local VERSION_PRINTF
src/version.c
Outdated
ESP_LOGI(TAG, "LIBWOLFSSL_VERSION_STRING = %s", LIBWOLFSSL_VERSION_STRING); | ||
|
||
#if defined(LIBWOLFSSL_VERSION_GIT_HASH) | ||
ESP_LOGI(TAG, "LIBWOLFSSL_VERSION_GIT_HASH = %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stuff would be great in a general use case, for example if we compile with '--enable-debug' it would be nice to see this printed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting this stuff in a separate function like 'gitVersionInfo' might be better organized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the end user can choose the --enable-debug
option in their code around their call to ShowExtendedSystemInfo()
.
I did however put things into separate functions for readability. (e.g. ShowExtendedSystemInfo_git()
)
src/version.c
Outdated
#endif | ||
|
||
/* some interesting settings are target specific (ESP32, -C3, -S3, etc */ | ||
#if defined(CONFIG_IDF_TARGET_ESP32C3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe then you could put the ESP32 stuff in a separate function that is guarded by WOLFSSL_ESPIDF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup: see ShowExtendedSystemInfo_platform_espressif()
wolfssl/version.h.in
Outdated
@@ -28,8 +28,12 @@ | |||
extern "C" { | |||
#endif | |||
|
|||
#define LIBWOLFSSL_VERSION_STRING "@VERSION@" | |||
#define LIBWOLFSSL_VERSION_HEX @HEX_VERSION@ | |||
#define LIBWOLFSSL_VERSION_STRING "5.5.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the definition of this come from configure.ac
(i.e.: WOLFSSL_LIBRARY_VERSION)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes, good catch.
9b2e1b4
to
fb77459
Compare
@bandi13 all good suggestions. See latest update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some minor changes requested.
src/version.c
Outdated
/* define new platform-specific printf() here */ | ||
#define WOLFSSL_VERSION_PRINTF(...) printf(__VA_ARGS__); | ||
#else | ||
#define WOLFSSL_VERSION_PRINTF(...) printf(__VA_ARGS__); printf("\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs "{}" for instance if we have in code:
if(false) WOLFSSL_VERSION_PRINTF("stuff");
then, that would always print a newline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes, good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been implemented. Let me know if any other changes are needed.
src/version.c
Outdated
#elif defined(CONFIG_IDF_TARGET_ESP32S3) | ||
#error "ESP32WROOM32_CRYPT not yet supported on ESP32-S3" | ||
#else | ||
WOLFSSL_VERSION_PRINTF("ESP32WROOM32_CRYPT is enabled."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems error prone when new parts come out we would have to be aware and add it to the list (not to mention propagate to customers). Instead, you should have an IF block on parts that do support ESP32WROOM32_CRYPT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. changed logic to known supported targets, everything else is an error
WOLFSSL_VERSION_PRINTF("Xthal_have_ccount = %u", | ||
Xthal_have_ccount); | ||
#elif CONFIG_IDF_TARGET_ESP32H2 | ||
/* not supported at this time */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't put in unnecessary conditions. When we add support we can add those targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. changed logic to known supported targets, everything else is an error
src/version.c
Outdated
Xthal_have_ccount); | ||
|
||
#if defined(SINGLE_THREADED) | ||
/* need stack monitor for single thread */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Negate this condition around so there is no empty block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, no... it was actually a TODO. I instead put in a HWM from stack allocation address.
*/ | ||
#include <wolfssl/version.h> | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/version.c
Outdated
|
||
#include <wolfssl/wolfcrypt/settings.h> | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/version.c
Outdated
return 0; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
return 0; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra empty lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/version.c
Outdated
ShowExtendedSystemInfo_platform_espressif(); | ||
#endif | ||
#elif defined(WOLFSSL_OTHER_PLATFORM) | ||
/* add other platforms here */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need this block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. changed logic to known supported targets, everything else is an error
fb77459
to
eb736fd
Compare
This is currently only for CMake projects, such as Espressif. For makefiles, I tried adding this to configure.ac at line 8519:
and this in src/include.am at line 23:
and this at line 29:
and tried this at line 56
But the linker still fails with I tried adding a
but then multiple definition error:
If I then remove the above if I tried these steps from @kaleb-himes
As there's not much interesting for other platforms, perhaps makefile support can be added in a future PR. Here's the updated output for Espressif
|
There has to be a way for this to go in our regular build flow. Especially the 'git' information is super useful for all platforms. As we've discussed before, it would help at the minimum the support team whereby they have the exact version information as part of the logs. Not having this be part of |
We can always add that later in a separate PR. Just because I've not yet been successful does not mean I've given up.
Good suggestion. I've tried that as well. Clearly I'm missing something.
I completely agree. Otherwise, any objection to merging this PR? I'd like to remove all the duplicate code from the Espressif examples soon. |
Jenkins retest this please |
eb736fd
to
3103a88
Compare
d745786
to
e44abef
Compare
I have some proposed changes, not yet applied here as related to #6123 introspection. |
6e3eb61
to
7991c06
Compare
Jenkins retest this please |
1 similar comment
Jenkins retest this please |
Jenkins retest this please |
1297153
to
736e30f
Compare
8d7e91c
to
98bfc9c
Compare
I believe I have all build issue resolved. The last problematic one was needing to add the
I'm still getting a *edit: here's a script to check the previously problematic build: make clean
make distclean
./autogen.sh
./configure --enable-aesgcm=table --enable-all-crypto --enable-cryptonly --enable-crypttests --disable-asm --disable-fastmath && make && ./wolfcrypt/test/testwolfcrypt |
b6ec6d3
to
afb7b8d
Compare
afb7b8d
to
aeebb9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what you are trying to do here, but I don't like the solution. This is very ESP centric and should not be in the library. It should be in the ESP specific code or even the ESP port code. Why not extend the existing HAVE_WC_INTROSPECTION
?
@@ -22,6 +22,7 @@ EXTRA_DIST += src/pk.c | |||
EXTRA_DIST += src/ssl_asn1.c | |||
EXTRA_DIST += src/ssl_bn.c | |||
EXTRA_DIST += src/ssl_misc.c | |||
EXTRA_DIST += src/version.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a new files requires updating all the other build platforms like CMake, Visual Studio projects, etc. Please don't do this action unless a new file is absolutely required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be a reason not to separate out functionality into smaller files. Our files are too big as it is. Instead, maybe update the other build platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very ESP centric
Indeed this is very ESP centric at the moment, as I've been doing mostly Espressif development. My plan is to have an infrastructure in place to allow others to add their own platform-specific details as needed & desired. When doing so, and only as needed, the respective build & make platforms could be updated.
Although Espressif-specific now, I do see there being significant value in having a template of common system information for all platforms and all products. That would seem to be a valuable resource for customers to provide when asking for support.
It should be in the ESP specific code or even the ESP port code
I see your point here on the Espressif-specific details. I envisioned all of the "hardware / configuration overview" details being in a single file. As hardware typically has a version as well, that's another reason I was thinking of the version.c
as a home. for this information.
If instead we had something way down in the ./port/expressif
, what structure would you use? Perhaps a hardware specific ShowHardwareConfiguration()
that is implemented for each platform, then called as appropriate from a single function elsewhere?
don't [add version.c] unless a new file is absolutely required
It was quite a learning curve to tease that file into the builds, so I definitely respect that comment. Still, iIntuitively for me, it seems to make sense to have this information in the version.c
file. That seems to be a universal file & the most global across all wolfSSL libraries.
Why not extend the existing
HAVE_WC_INTROSPECTION
?
This is an excellent question. Although similar in concept, some of the information is mutually exclusive from introspection. @douzzer pointed out that: "HAVE_WC_INTROSPECTION
requires that the configuration and build time artifacts, particularly the date/time of build and git parameters, be excluded from the build". For example: some of the information such as git hash & date may be updated, but the binary build itself should not actually change.
Given this, if you still feel strongly that I should not add the version.c
, I'm happy to implement this functionality elsewhere. Any preference? Perhaps wolfcrypt/logging.c
would be a appropriate? I certainly agree with @bandi13 about the big files. I'm in favor of more specific, smaller files when possible.
#ifdef __cplusplus | ||
extern "C" { | ||
#endif | ||
|
||
#ifdef HAVE_CONFIG_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These config.h and settings.h go in the .c, not the .h.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the config.h
and settings.h
using the model from ssl.h; in particular I needed to include visibility.h
in order to use the WOLFSSL_API macro, without which, the linker could not find ShowExtendedSystemInfo()
. Is there a preferred way of doing this?
|
||
#endif /* NO_VERSION_EXTENDED_INFO */ | ||
|
||
#ifdef __cplusplus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The __cplusplus
logic only goes in the .h. It is already there, just remove in .c.
This PR will be mothballed for the time being. I've moved the The functionality can still be used in main.c Espressif code like this:
There's a new PR coming soon from my WIP ED25519_SHA2_fix branch. See also #6234 Espressif Imporvements summary. |
|
||
if test "$VERSION_EXTENDED_INFO" = "no" | ||
then | ||
AM_CFLAGS="$AM_CFLAGS -DNO_VERSION_EXTENDED_INFO" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably don't need this as I don't think you use NO_VERSION_EXTENDED_INFO
anywhere. Better to have the affirmative form of options, so that we can avoid things like #ifndef NO_VERSION_
to check that the extended info exists.
We decided not to implement the extended version information in There is however some extended information in the esp32_util.c for the Espressif embedded target. Closing this lingering draft PR.... |
Description
Adds a platform-specific
ShowExtendedSystemInfo()
function.This code is otherwise duplicated among many examples, cluttering up
main
in each.The version details are expected to be generated from CMake tasks, such as the benchmark example.
I plan to add additional "interesting" application information as appropriate.
Allows displaying of build-time info like this at runtime:
Fixes zd#
Testing
Tested in Espressif ESP-IDF. Linux apps will compile, but the
version.c
is not yet included in the makefiles and otherwise has no additional information for that platform. Future PRs are expected to add platform and environment-specific details.Edit 3/14/23:
A script to fully test:
Now includes the options
--disable-version-extended-info
or--enable-version-extended-info
for non-embedded builds.Disabled
Commandline version will look like this for
testwolfcrypt
output with--disable-version-extended-info
Enabled
Commandline version will look like this for
testwolfcrypt
output with--enable-version-extended-info
and the usual:
Checklist