ulfalizer / Kconfiglib

A flexible Python 2/3 Kconfig implementation and library
ISC License
450 stars 161 forks source link

Dependency loop issue #54

Closed weety closed 6 years ago

weety commented 6 years ago

I use Kconfiglib to configure my project, but found problems. I didn't find anything wrong with the Kconfig file. I don't know if there is a problem with the dependency loop detection logic of the library.Please take a look, thank you!

Attach log below:

Dependency loop

RT_USING_DFS (defined at ....\components\dfs\Kconfig:3), with definition...

config RT_USING_DFS bool prompt "Using device virtual file system" default "y" select RT_USING_MUTEX help The device file system is a light weight virtual file system.

(select-related dependencies: (RT_USING_SPI_MSD && RT_USING_SPI) || (SAL_USING_POSIX && RT_USING_SAL))

...depends on SAL_USING_POSIX (defined at ....\components\net\Kconfig:28), with definition...

config SAL_USING_POSIX bool prompt "Enable BSD socket operated by file system API" if RT_USING_SAL default "y" if RT_USING_POSIX && RT_USING_SAL default "n" if RT_USING_SAL select RT_USING_DFS if RT_USING_SAL select RT_USING_LIBC if RT_USING_SAL select RT_USING_POSIX if RT_USING_SAL depends on RT_USING_SAL help Let BSD socket operated by file system API, such as read/write and involveed in select/poll POSIX APIs.

...depends on RT_USING_POSIX (defined at ....\components\libc\Kconfig:12), with definition...

config RT_USING_POSIX bool prompt "Enable POSIX layer for poll/select, stdin etc" if RT_USING_LIBC && RT_USING_DFS default "y" if RT_USING_LIBC && RT_USING_DFS select RT_USING_DFS_DEVFS if RT_USING_LIBC && RT_USING_DFS depends on RT_USING_LIBC && RT_USING_DFS

(select-related dependencies: SAL_USING_POSIX && RT_USING_SAL)

...depends again on RT_USING_DFS (defined at ....\components\dfs\Kconfig:3):

weety commented 6 years ago

With the same configuration file, I have no problem using the c version of mconf. @ulfalizer

ulfalizer commented 6 years ago

Looks like a legit dependency loop. It's just not being detected by the C tools. Sure you're running with the same environment variables set, etc.?

With irrelevant stuff removed, it boils down to this:

config SAL_USING_POSIX
    ...
    default y if RT_USING_POSIX
    select RT_USING_DFS

...

if ... && RT_USING_DFS
    config RT_USING_POSIX
        ...

...

config RT_USING_DFS
    ...
  1. SAL_USING_POSIX depends on RT_USING_POSIX (default y if RT_USING_POSIX)
  2. RT_USING_POSIX depends on RT_USING_DFS (it's in a if ... && RT_USING_DFS block)
  3. RT_USING_DFS depends on SAL_USING_POSIX (because SAL_USING_POSIX has a select RT_USING_DFS)

Reference for symbols:

The dependency loop error message could be improved when selects are involved. It's a bit hard to interpret now. selected-related dependencies: ... || (SAL_USING_POSIX && RT_USING_SAL) means that SAL_USING_POSIX has a select RT_USING_DFS if RT_USING_SAL.

weety commented 6 years ago

@ulfalizer All environments and variables are the same and work well with the c tool。

weety commented 6 years ago

I don't think this is a real circular dependency.

ulfalizer commented 6 years ago

@weety It's real in the sense that it matches how the symbol is evaluated. SAL_USING_POSIX evaluates RT_USING_POSIX evaluates RT_USING_DFS, which again evaluates SAL_USING_POSIX.

In practice, you might not run into infinite recursion anyway, because there are internal optimizations that e.g. immediately return n when A evaluates to n in A && B, never looking at B (see the expr_value() code), but the dependency loop detection can't know that that will happen, so it flags it.

The C implementation seems to do the same simple dependency link following that Kconfiglib does to detect dependency loops, so not sure why it isn't detecting that one. Could be a bug in the C tools (there's been others, e.g. related to loop detection for imply), or some slight algorithm tweak that I'm not noticing. There's no fancy analysis happening internally at least. It's just following dependency links (A depends in some way on B, which depends in some way on C, ...)

No dependency loops are detected in the Linux kernel, Zephyr, U-Boot, ACRN, or Espressif at least.

weety commented 6 years ago

@ulfalizer Your analysis is reasonable. Let me see how to eliminate cyclic dependency. I don't know if you have any good suggestions.Thanks!

ulfalizer commented 6 years ago

@weety Bit hard to say without being familiar with the codebase. One thing that often works is to replace select with depends on though (provided that still makes sense). select is often a bit overused.

If you just need a temporary quickfix to get it to run, you could comment out this code in Kconfiglib:

# Check for dependency loops
for sym in self.unique_defined_syms:
    _check_dep_loop_sym(sym, False)

You could theoretically get infinite recursion then (which makes the program crash), but it might just happen to work due to expression short-circuiting.

ulfalizer commented 6 years ago

@weety I tried running the latest version of the C tools on the rt-thread codebase, and they do detect a variant of the recursive dependency as well:

$ RTT_DIR=. ~/devel/linux/scripts/kconfig/mconf Kconfig
...
./components/net/Kconfig:30:error: recursive dependency detected!
./components/net/Kconfig:30:    symbol SAL_USING_POSIX depends on RT_USING_POSIX
./components/libc/Kconfig:12:   symbol RT_USING_POSIX is selected by SAL_USING_POSIX
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"

The difference is that the C tools do not turn it into an error, meaning they can potentially get stuck later.

(You can find different loops depending on the order that you iterate over the symbols during dependency link following, so that's why it isn't exactly the same loop. Just noticed that different loops are detected for identical runs on Python 3.4 with Kconfiglib as well, which is a bit weird. Think it's because of Python set internals. :))

The latest version of the C tools only support the $(RTT_DIR) syntax for referencing environment variables, so I had to replace $RTT_DIR with $(RTT_DIR) first in the Kconfig files. That's the new recommended syntax. The syntax without () is supported for backwards compatibility in Kconfiglib.

The documentation linked from the error message is helpful. Maybe I should add it to Kconfiglib as well. Had forgotten about it.

weety commented 6 years ago

@ulfalizer I tested it with the linux-4.9 version of mconf and found cyclic dependency detection. You are right.

ulfalizer commented 6 years ago

Yep, so doesn't seem to be a case of the C tools doing something fancy that I had missed either. Phew.

Closing for now.