Skip to content

kconfig: Add option to fail build on warning #77529

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

Conversation

rettichschnidi
Copy link
Contributor

@rettichschnidi rettichschnidi commented Aug 25, 2024

This allows to ensure that CONFIG_* options do not get ignored due to unmet dependencies.

This would implement the first step of #77530. In a later one, twister should gain a parameter to enable this option on its runs.

This allows to ensure that CONFIG_* options do not get ignored due to
unmet dependencies.

In a later step, twister should gain a parameter to enable this option
on its runs.

Signed-off-by: Reto Schneider <[email protected]>
@rettichschnidi rettichschnidi marked this pull request as ready for review August 26, 2024 19:04
@zephyrbot zephyrbot requested review from nashif and tejlmand August 26, 2024 19:04
Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Kconfig warnings are not an issue, there are various conditions that will emit them which are harmless, I therefore cannot accept this change since you want to make this the default for twister

@rettichschnidi
Copy link
Contributor Author

CMake warnings are not an issue, there are various conditions that will emit them which are harmless,

Are we talking about the same thing? This PR is not about CMake warnings, but Kconfig one.

It allows to turn the following warning into an error:

warning: LOG_BUFFER_SIZE (defined at subsys/logging/Kconfig.processing:122) was assigned the value
'65536' but got the value ''. Check these unsatisfied dependencies: LOG_MODE_DEFERRED (=n). See
http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_LOG_BUFFER_SIZE and/or look up
LOG_BUFFER_SIZE in the menuconfig/guiconfig interface. The Application Development Primer, Setting
Configuration Values, and Kconfig - Tips and Best Practices sections of the manual might be helpful
too.

But it will not do anything to the CMake ones, for example this one:

CMake Warning at /home/reto/code/3rd-party/zephyrproject/zephyr/CMakeLists.txt:2097 (message):
__ASSERT() statements are globally ENABLED

I therefore cannot accept this change since you want to make this the default for twister

Yes, that is my long-term plan. But this PR here just allows devs to opt-in to ensure their apps build without any warnings from Kconfig.

@nordicjm
Copy link
Contributor

Yes, as said they are not issues, the comment should have been Kconfig not cmake

@rettichschnidi
Copy link
Contributor Author

rettichschnidi commented Aug 27, 2024

Kconfig warnings are not an issue, there are various conditions that will emit them which are harmless,

In my experience, those are often, but not always harmless. Too often I enabled a certain option for my app, just to have it disabled by Kconfig due to missing dependencies. Typically this happens downstream, after upgrading to a newer Zephyr version.

I think it would be useful to have an easy way to enforce that this does not happen.

I therefore cannot accept this change since you want to make this the default for twister

Can we please split this discuss up in two parts?

  1. This PR here. No default will be changed, but having this feature in (upstream) Kconfig simplifies certain work downstream (mainly, upgrading to new Zephyr versions). Maybe it even helps for certain upstream work too.
  2. Discuss everything else (i.e. defaults in Twister) in Fail build on Kconfig warning #77530

Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

this should not be controlled through a Kconfig setting.

In fact, Kconfig will fail on certain warnings, or more precisely, CMake will fail if kconfig returns an error code, as can be seen here:

if(NOT "${ret}" STREQUAL "0")
message(FATAL_ERROR "command failed with return code: ${ret}")

For example, a warning of a symbol being selected by another symbol when there are unmet dependencies will call a failure, like this:

warning: FOO (defined at ...) has direct dependencies BAR with value n, but is currently being y-selected by the following symbols:
 - BAZ (defined at ...), with value y, direct dependencies y (value: y)

error: Aborting due to Kconfig warnings

But as this PR describes then there are also warnings which doesn't result in an error, and those are user configured settings, to undefined symbols or symbols with unmet dependencies, and a reason for that can be seen here:

if args.handwritten_input_configs:
# Warn for assignments to undefined symbols, but only for handwritten
# fragments, to avoid warnings-turned-errors when using an old
# configuration file together with updated Kconfig files
kconf.warn_assign_undef = True

# Print warnings for symbols that didn't get the assigned value. Only
# do this for handwritten input too, to avoid likely unhelpful warnings
# when using an old configuration and updating Kconfig files.
check_assigned_sym_values(kconf)
check_assigned_choice_values(kconf)

From the inline comments above, then it looks to me as this behavior was decided on purpose.
Of course behavior can be discussed and changed if there are good reasons, but the proposal of introducing a Kconfig setting to control the behavior looks wrong to me, as it doesn't take into consideration how the rest of the build system is designed regarding this. (kconfig.py / kconfig.cmake)

@rettichschnidi
Copy link
Contributor Author

rettichschnidi commented Sep 4, 2024

@tejlmand Thanks a lot for the detailed response. I think I understand your point and will try to figure out a better approach to address my needs for keeping the builds clean/warning free.

rettichschnidi added a commit to husqvarnagroup/zephyr that referenced this pull request Sep 25, 2024
This allows to ensure that CONFIG_* options do not get ignored due to
unmet dependencies.

In a later step, twister should gain a parameter to enable this option
on its runs.

Upstream: zephyrproject-rtos#77529
Signed-off-by: Reto Schneider <[email protected]>

(cherry picked from commit c4c5a10)
rettichschnidi added a commit to husqvarnagroup/zephyr that referenced this pull request Sep 25, 2024
This reverts commit af41d91.

This approach was shot down upstream. Probably rightfully so.

Upstream: zephyrproject-rtos#77529
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants