Skip to content

Remove hexdump_test.cc from source list #3094

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

Merged
merged 7 commits into from
Apr 16, 2025

Conversation

shlmregev
Copy link
Member

As a unit test, it belongs on MICROLITE_TEST_SRCS, which gets filtered out of MICROLITE_CC_SRCS. Keeping it on the source list results in it being compiled into the tflm archive, where its main() function conflicts with the application's main.
Also, since hexdump.cc calls DebugVsnprintf(), we need to define it in debug_log.cc, also when TF_LITE_STRIP_ERROR_STRINGS is #defined, else compilation fails. Move the #ifdef TF_LITE_STRIP_ERROR_STRINGS to be inside the function implementation, rather around the whole function.

BUG=384562154

@shlmregev shlmregev requested a review from a team as a code owner April 14, 2025 13:56
@TFLM-bot TFLM-bot removed the ci:run label Apr 14, 2025
@ddavis-2015
Copy link
Member

@shlmregev I am thinking that running the tests in RELEASE builds is not something we do, as there is no point in running tests that can't tell you if they fail/succeed. So it seems all the changes to debug_log.cc are not needed.

@shlmregev shlmregev force-pushed the remove-hexdump branch 4 times, most recently from f5694ef to 3acafc2 Compare April 15, 2025 08:55
As a unit test, it belongs on MICROLITE_TEST_SRCS, which gets filtered
out of MICROLITE_CC_SRCS. Keeping it on the source list results in it
being compiled into the tflm archive, where its main() function
conflicts with the application's main.

BUG=384562154

	modified:   tensorflow/lite/micro/tools/make/Makefile
@shlmregev
Copy link
Member Author

@shlmregev I am thinking that running the tests in RELEASE builds is not something we do, as there is no point in running tests that can't tell you if they fail/succeed. So it seems all the changes to debug_log.cc are not needed.

That's a good point and I reverted the changes to debug_log.cc, but the 'x86 Release' CI check is currently failing because I added hexdump_test to the list of tests and DebugVsnprintf() isn't defined. Even if I define it to return 0, hexdump_test will fail.
@suleshahid @rkuester any suggestions?

@suleshahid
Copy link
Collaborator

I believe that test should only run when Compression is enabled in the config (as we can see Makefile x86 with Compression CI is passing). Is that correct @rkuester ?

@ddavis-2015
Copy link
Member

@shlmregev I am thinking that running the tests in RELEASE builds is not something we do, as there is no point in running tests that can't tell you if they fail/succeed. So it seems all the changes to debug_log.cc are not needed.

That's a good point and I reverted the changes to debug_log.cc, but the 'x86 Release' CI check is currently failing because I added hexdump_test to the list of tests and DebugVsnprintf() isn't defined. Even if I define it to return 0, hexdump_test will fail. @suleshahid @rkuester any suggestions?

@shlmregev I believe I see the issue now. In hexdump.cc, the DebugVsnprintf should be changed to MicroVsnprintf and the include of debug_log.h changed to micro_log.h. The micro_log.h handles builds with TF_LITE_STRIP_ERROR_STRINGS such that they will not fail. Both the bluepill and x86 release builds should then complete without error.

@rkuester
Copy link
Contributor

I believe that test should only run when Compression is enabled in the config (as we can see Makefile x86 with Compression CI is passing). Is that correct @rkuester ?

Compression uses this feature, but the feature and its unit test are independent of compression. So no, I would not conditionally compile out the unit test. It should always run.

@ddavis-2015
Copy link
Member

@shlmregev The bluepill build of hexdump_test is still excluded?

@mergify mergify bot merged commit 80472ee into tensorflow:main Apr 16, 2025
33 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.

5 participants