Skip to content

Conversation

@xuxin930
Copy link
Contributor

@xuxin930 xuxin930 commented Sep 14, 2024

Summary

maintains the same semantics as Kernel’s module build and compatible with Kconfiglib implementation.

The problem of kconfiglib not being able to use tri-states is this:
linux is here torvalds/linux@6dd85ff
The module option is extracted from one of the triate three states into a MODULE keyword.
Kconfiglib supports the default implementation of module to be MODULE for compatibility with the linux kernel, and triate degenerates into bool two states
Refer to this code: https://github.com/ulfalizer/Kconfiglib/blob/061e71f7d78cb057762d88de088055361863deff/kconfiglib.py#L4274-L4284

Impact

kconfiglib can support

Testing

relate apps patch: apache/nuttx-apps#2577

maintains the same semantics as Kernel’s module build
and compatible with Kconfiglib implementation.

The problem of kconfiglib not being able to use tri-states is this:
linux is here torvalds/linux@6dd85ff
The module option is extracted from one of the triate three states into a MODULE keyword.
Kconfiglib supports the default implementation of module to be MODULE for compatibility with the linux kernel, and triate degenerates into bool two states
Refer to this code: https://github.com/ulfalizer/Kconfiglib/blob/061e71f7d78cb057762d88de088055361863deff/kconfiglib.py#L4274-L4284

Signed-off-by: xuxin19 <xuxin19@xiaomi.com>
@xuxin930 xuxin930 marked this pull request as draft September 14, 2024 09:10
@xuxin930 xuxin930 marked this pull request as ready for review September 14, 2024 12:27
@xuxin930
Copy link
Contributor Author

the reason why CI failed is because it has a dependency with the following PR.apache/nuttx-apps#2577
they need to be run together. 😣
the local test was successful.

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

This seems breaking change, lets discuss in depth first.

@cederom
Copy link
Contributor

cederom commented Sep 16, 2024

As @patacongo replied on apache/nuttx-apps#2577, lets discuss here :-)

I am not so much involved these days but in the past, configuration names began with CONFIG_ (of course) then what was being modified ( for example, NET for anyting affecting the network, and then the remainder describing what is being configured.

So by those old rules, CONFIG_MODULES would be wrong. Since it affects the build system, it should be CONFIG_BUILD_MODULES.

What is configured? BUILD system
What is controlled in the build system? MODULE support.

But since no one has been monitoring this I am sure that that naming system has gone to hell.

This is not documented to my knowledge, but there are other documented naming conventions here: https://cwiki.apache.org/confluence/display/NUTTX/Wiki

I can see several problems:

  1. Build configuration variables nomenclature change (breaks code backward/forward compatibility).
  2. Change of Kconfig files syntax / functionality.
  3. Change of Kconfig tools compatibility.

Regarding 2 and 3:

  • Remember that we use pretty old kconfig-frontends external package as dependency and most of us use that version.
  • How is this old Kconfig package going to support option vs module change introduced in 2021 [1]?
  • How this Kconfig files syntax change going to work on new / old Kconfig tools?
  • What is the benefit of intruducing that change? Is it really necessary? Can the problem be solved in a different way?

[1] torvalds/linux@6dd85ff

@patacongo
Copy link
Contributor

patacongo commented Sep 16, 2024

  1. Build configuration variables nomenclature change (breaks code backward/forward compatibility).

Most configuration names conform to the the naming convention. That naming was enforced until I gave the software to Apache. But I still expect most things will conform to the unenforced, undocumented standard. The original name CONFIG_BUILD_LOADABLE, for example, is compliant. The modified name is not.

Look in the defconfig files. Most things are grouped together by the second word in the configuration name. That not only tells you want is being configured, but in most cases, the exact Kconfig file where the configuration is selected.

@xiaoxiang781216 xiaoxiang781216 added breaking change This change requires a mitigation entry in the release notes. and removed breaking change This change requires a mitigation entry in the release notes. labels Sep 16, 2024
@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Sep 16, 2024

@patacongo @cederom

First, it isn't a breaking change since CONFIG_BUILD_LOADABLE/CONFIG_MODULES hasn't prompt string and then user can't select it from defconfig.

Second, CONFIG_MODULES is a built-in option since both kconfig-frontend and kconfiglib contain the code to handle this option specially as @xuxin930 mention here:
torvalds/linux@6dd85ff
https://github.com/ulfalizer/Kconfiglib/blob/061e71f7d78cb057762d88de088055361863deff/kconfiglib.py#L4274-L4284
so, we can't give this option a new name (e.g. CONFIG_BUILD_LOADABLE), but still keep the special handing.

Both tools aren't maintained by NuttX community, so we have to follow their usage, otherwise module feature can't work with kconfiglib now and will break with the new version kconfig-frontend.

This patch makes module work with old kconfig-frontend, new kconfig-frontend and kconfiglib, so it isn't a break change.

@xiaoxiang781216
Copy link
Contributor

  1. Build configuration variables nomenclature change (breaks code backward/forward compatibility).

Most configuration names conform to the the naming convention. That naming was enforced until I gave the software to Apache. But I still expect most things will conform to the unenforced, undocumented standard. The original name CONFIG_BUILD_LOADABLE, for example, is compliant. The modified name is not.

Look in the defconfig files. Most things are grouped together by the second word in the configuration name. That not only tells you want is being configured, but in most cases, the exact Kconfig file where the configuration is selected.

@patacongo it since CONFIG_BUILD_LOADABLE/CONFIG_MODULES hasn't a prompt string and then user can't select it from defconfig, that's why this patch just change Kconfig/Makefile, but not defconfig.

@cederom
Copy link
Contributor

cederom commented Sep 16, 2024

  • Okay, I get the point right now, thanks @xiaoxiang781216 @xuxin930 :-)
  • We need better descriptions in PR for better processing and understanding from someone that jumps into the different tasks with no deep insight like the reporter has. I have provided a PR template update suggestion (Update GitHub Pull Request Template. #13494).
  • This problem is here since NuttX ~7.26.
  • It will not break compatibility with old/new tools but fix the long existing compatibility problem.
  • The config variable is only used in Kconfig, Make, and CMake. This does not impact code API compatibility.
  • Documentation updates are provided.
  • The only problem is (internal) variable name change, but this is somehow enforced by Kconfig, and in align with other OS like Linux.
  • Is this acceptable @patacongo ? :-)

@xiaoxiang781216
Copy link
Contributor

@patacongo do you have any suggestion?

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

After discussion and more details I think this solution is very good, fixes loong lasting problem, adheres to Linux (where Kconfig comes from), and does not break the code, thank you @xuxin930 @xiaoxiang781216 !! :-)

I give go but the final decision is up to @patacongo :-)

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.

4 participants