From 1774239986d36f4894e7f59efd953f408cbbf80a Mon Sep 17 00:00:00 2001 From: Ulf Magnusson Date: Sun, 1 Oct 2017 20:01:08 +0200 Subject: Make 'imply' consider direct dependencies Bad oversight. Weak reverse dependencies (from imply) are not considered if the direct dependencies of a symbol are not met (the 'if'/'depends on' dependencies from the symbol and its parents, taking location into account if the symbol is defined in multiple places). Caused a wrong value for the symbol FS_FAT in the U-Boot Kconfigs, where 'imply' is more heavily used compared to the kernel. Add a new variable _direct_deps that corresponds to dir_dep from the C implementation. Before 'imply', dir_dep was only used for a 'select'-related warning in the C implementation. Add a bunch of tests to cover 'imply' semantics. Should be solid now. --- kconfiglib.py | 43 ++++++++++++----- tests/Kdep | 111 +++++++++++++++++++++++--------------------- tests/Kimply | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ testsuite.py | 105 +++++++++++++++++++++++++++++++++++++++++- 4 files changed, 338 insertions(+), 66 deletions(-) create mode 100644 tests/Kimply diff --git a/kconfiglib.py b/kconfiglib.py index c111c68..795472f 100644 --- a/kconfiglib.py +++ b/kconfiglib.py @@ -1094,6 +1094,9 @@ class Config(object): else: # Symbol or Choice + if isinstance(stmt, Symbol): + stmt._direct_deps = _make_or(stmt._direct_deps, stmt._menu_dep) + # Propagate dependencies to prompts if new_prompt is not None: prompt, cond_expr = new_prompt @@ -1608,14 +1611,20 @@ class Config(object): for expr_sym in res: expr_sym._dep.add(sym) - # The directly dependent symbols of a symbol are: + # The directly dependent symbols of a symbol S are: + # # - Any symbols whose prompts, default values, _rev_dep (select - # condition), _weak_rev_dep (imply condition) or ranges depend on - # the symbol - # - Any symbols that belong to the same choice statement as the symbol - # (these won't be included in _dep as that makes the dependency - # graph unwieldy, but Symbol._get_dependent() will include them) - # - Any symbols in a choice statement that depends on the symbol + # condition), _weak_rev_dep (imply condition) or ranges depend on S + # + # - Any symbol that has S as a direct dependency (has S in + # _direct_deps). This is needed to get invalidation right for + # 'imply'. + # + # - Any symbols that belong to the same choice statement as S + # (these won't be included in S._dep as that makes the dependency + # graph unwieldy, but S._get_dependent() will include them) + # + # - Any symbols in a choice statement that depends on S # Only calculate _dep for defined symbols. Undefined symbols could # theoretically be selected/implied, but it wouldn't change their value @@ -1637,6 +1646,8 @@ class Config(object): add_expr_deps(u, sym) add_expr_deps(e, sym) + add_expr_deps(sym._direct_deps, sym) + if sym._is_choice_sym: choice = sym._parent for _, e in choice._prompts: @@ -2098,11 +2109,14 @@ class Symbol(Item): val = self._config._eval_min(def_expr, cond_val) break - weak_rev_dep_val = \ - self._config._eval_expr(self._weak_rev_dep) - if weak_rev_dep_val != "n": - self._write_to_conf = True - val = self._config._eval_max(val, weak_rev_dep_val) + # Weak reverse dependencies are only considered if our + # direct dependencies are met + if self._config._eval_expr(self._direct_deps) != "n": + weak_rev_dep_val = \ + self._config._eval_expr(self._weak_rev_dep) + if weak_rev_dep_val != "n": + self._write_to_conf = True + val = self._config._eval_max(val, weak_rev_dep_val) # Reverse (select-related) dependencies take precedence rev_dep_val = self._config._eval_expr(self._rev_dep) @@ -2529,6 +2543,11 @@ class Symbol(Item): # See comment in _parse_properties() self._menu_dep = None + # The direct dependencies (inherited + 'depends on', with OR if a + # symbol is defined in multiple locations). This is needed for 'imply' + # support. + self._direct_deps = "n" + # See Symbol.get_ref/def_locations(). self._def_locations = [] self._ref_locations = [] diff --git a/tests/Kdep b/tests/Kdep index bde8790..ef1a4dc 100644 --- a/tests/Kdep +++ b/tests/Kdep @@ -1,31 +1,31 @@ config D bool "D" - select D29 - imply D30 + select D31 + imply D32 config DUMMY - select D31 if D - imply D32 if D + select D33 if D + imply D34 if D # The symbols below depend on D in different ways config D1 - def_bool D + def_bool D config D2 - int "D2" if D + int "D2" if D config D3 - int "D3" - depends on D + int "D3" + depends on D config D4 - bool "D4" - default D + bool "D4" + default D config D5 - bool - default y if D + bool + default y if D config D6 int @@ -39,42 +39,47 @@ config D8 int range 0 D -if D +# D9 and D10 depend on D even though they have no prompt, because it's needed +# to get invalidation right for 'imply' -# Has no prompt and hence does not depend on D even though it's within the -# 'if D' block -config NO_DEPEND +if D +config D9 bool +endif -config D9 - bool "D9" +config D10 + bool + depends on D +if D +config D11 + bool "D11" endif menu "m" depends on D -config D10 - bool "D10" +config D12 + bool "D12" menu "nested" -config D11 - bool "D11" +config D13 + bool "D13" endmenu endmenu # Indirect dependency -config D12 - def_tristate D11 +config D14 + def_tristate D13 menu "m" depends on D if D # Depends on D in lots of different ways -config D13 - int "D13" if D +config D15 + int "D15" if D depends on D && D12 default D if D range D D if D @@ -83,56 +88,50 @@ endmenu # Different kinds of expressions -config D14 - bool "D14" if D || n - -config D15 - bool "D15" if n || D - config D16 - bool "D16" if D && y + bool "D16" if D || n config D17 - bool "D17" if y && D + bool "D17" if n || D config D18 - bool "D18" if !D + bool "D18" if D && y config D19 - bool "D19" if !D && y + bool "D19" if y && D config D20 - bool "D20" if !(D && y) + bool "D20" if !D config D21 - bool "D21" if (D) + bool "D21" if !D && y config D22 - bool "D22" if ((D)) + bool "D22" if !(D && y) config D23 - bool "D23" if n || (y && n || (m || D)) + bool "D23" if (D) config D24 - bool "D24" if D = n + bool "D24" if ((D)) config D25 - bool "D25" if n = D + bool "D25" if n || (y && n || (m || D)) config D26 - bool "D26" if n != D + bool "D26" if D = n config D27 - bool "D27" if D != n + bool "D27" if n = D config D28 - bool "D28" if n || ((n != D) || n) + bool "D28" if n != D config D29 - tristate "D29" + bool "D29" if D != n config D30 - tristate "D30" + bool "D30" if n || ((n != D) || n) config D31 tristate "D31" @@ -141,23 +140,29 @@ config D32 tristate "D32" config D33 - int "D33" - default 0 if D < 0 + tristate "D33" config D34 - int "D34" - default 0 if 0 < D + tristate "D34" config D35 int "D35" - default 0 if 0 <= D + default 0 if D < 0 config D36 int "D36" - default 0 if 0 > D + default 0 if 0 < D config D37 int "D37" + default 0 if 0 <= D + +config D38 + int "D38" + default 0 if 0 > D + +config D39 + int "D39" default 0 if 0 >= D # diff --git a/tests/Kimply b/tests/Kimply new file mode 100644 index 0000000..3ce346f --- /dev/null +++ b/tests/Kimply @@ -0,0 +1,145 @@ +config MODULES + def_bool y + option modules + +# +# Implied symbols with unmet and met direct dependencies +# + +config IMPLY_DIRECT_DEPS + def_tristate y + imply UNMET_DIRECT_1 + imply UNMET_DIRECT_2 + imply UNMET_DIRECT_3 + imply MET_DIRECT_1 + imply MET_DIRECT_2 + imply MET_DIRECT_3 + imply MET_DIRECT_4 + +config UNMET_DIRECT_1 + tristate + depends on n + +if n +config UNMET_DIRECT_2 + tristate +endif + +menu "menu" + depends on n + +config UNMET_DIRECT_3 + tristate + +endmenu + +config MET_DIRECT_1 + tristate + +config MET_DIRECT_2 + depends on y + tristate + +if y +config MET_DIRECT_3 + tristate +endif + +menu "menu" + depends on y + +config MET_DIRECT_4 + tristate + +endmenu + +# +# 'imply' with condition +# + +config IMPLY_COND + def_tristate y + tristate + imply IMPLIED_N_COND if n + imply IMPLIED_M_COND if m + imply IMPLIED_Y_COND if y + +config IMPLIED_N_COND + tristate + +config IMPLIED_M_COND + tristate + +config IMPLIED_Y_COND + tristate + +# +# Implying from symbol with value n +# + +# Will default to 'n' +config IMPLY_N_1 + tristate + imply IMPLIED_FROM_N_1 + +# This test also disables the imply, so it's kinda redundant, but why not +if n +config IMPLY_N_2 + tristate + imply IMPLIED_FROM_N_2 +endif + +config IMPLIED_FROM_N_1 + tristate + +config IMPLIED_FROM_N_2 + tristate + +# +# Implying from symbol with value m +# + +config IMPLY_M + def_tristate m + imply IMPLIED_M + # Implying a bool to 'm' makes it default to 'y' + imply IMPLIED_M_BOOL + +config IMPLIED_M + tristate + +config IMPLIED_M_BOOL + bool + +# +# 'imply' which should raise an 'm' default to 'y' +# + +config IMPLY_M_TO_Y + tristate + default y + imply IMPLIED_M_TO_Y + +config IMPLIED_M_TO_Y + tristate + default m + +# +# Used for testing user values +# + +config DIRECT_DEP + tristate "direct dep" + +config IMPLY + tristate "imply" + imply IMPLIED_TRISTATE + imply IMPLIED_BOOL + +config IMPLIED_TRISTATE + tristate "implied tristate" + depends on DIRECT_DEP + +config IMPLIED_BOOL + bool "implied bool" + depends on DIRECT_DEP diff --git a/testsuite.py b/testsuite.py index b3377cd..0943865 100644 --- a/testsuite.py +++ b/testsuite.py @@ -115,6 +115,11 @@ def run_selftests(): "value was '{}'." .format(sym_name, new_val, user_val, sym_new_val, sym_old_val)) + def assign_and_verify(sym_name, user_val): + """Like assign_and_verify_new_value(), with the expected value being + the value just set.""" + assign_and_verify_new_value(sym_name, user_val, user_val) + def assign_and_verify_new_user_value(sym_name, user_val, new_user_val): """Assigns a user value to the symbol and verifies the new user value.""" @@ -1827,6 +1832,104 @@ def run_selftests(): verify(kconfig_filename == "Kconfiglib/tests/Kmisc", "Wrong Kconfig filename - got '{}'".format(kconfig_filename)) + # + # Imply semantics + # + + print("Testing imply semantics...") + + c = kconfiglib.Config("Kconfiglib/tests/Kimply") + + verify_value("IMPLY_DIRECT_DEPS", "y") + verify_value("UNMET_DIRECT_1", "n") + verify_value("UNMET_DIRECT_2", "n") + verify_value("UNMET_DIRECT_3", "n") + verify_value("MET_DIRECT_1", "y") + verify_value("MET_DIRECT_2", "y") + verify_value("MET_DIRECT_3", "y") + verify_value("MET_DIRECT_4", "y") + + verify_value("IMPLY_COND", "y") + verify_value("IMPLIED_N_COND", "n") + verify_value("IMPLIED_M_COND", "m") + verify_value("IMPLIED_Y_COND", "y") + + verify_value("IMPLY_N_1", "n") + verify_value("IMPLY_N_2", "n") + verify_value("IMPLIED_FROM_N_1", "n") + verify_value("IMPLIED_FROM_N_2", "n") + + verify_value("IMPLY_M", "m") + verify_value("IMPLIED_M", "m") + verify_value("IMPLIED_M_BOOL", "y") + + verify_value("IMPLY_M_TO_Y", "y") + verify_value("IMPLIED_M_TO_Y", "y") + + # Test user value semantics + + # Verify that IMPLIED_TRISTATE is invalidated if the direct + # dependencies change + + assign_and_verify("IMPLY", "y") + assign_and_verify("DIRECT_DEP", "y") + verify_value("IMPLIED_TRISTATE", "y") + assign_and_verify("DIRECT_DEP", "n") + verify_value("IMPLIED_TRISTATE", "n") + # Set back for later tests + assign_and_verify("DIRECT_DEP", "y") + + # Verify that IMPLIED_TRISTATE can be set to anything when IMPLY has value + # "n", and that it gets the value "n" by default (for non-imply-related + # reasons) + + assign_and_verify("IMPLY", "n") + assign_and_verify("IMPLIED_TRISTATE", "n") + assign_and_verify("IMPLIED_TRISTATE", "m") + assign_and_verify("IMPLIED_TRISTATE", "y") + c["IMPLIED_TRISTATE"].unset_user_value() + verify_value("IMPLIED_TRISTATE", "n") + + # Same as above for "m". Anything still goes, but "m" by default now. + + assign_and_verify("IMPLY", "m") + assign_and_verify("IMPLIED_TRISTATE", "n") + assign_and_verify("IMPLIED_TRISTATE", "m") + assign_and_verify("IMPLIED_TRISTATE", "y") + c["IMPLIED_TRISTATE"].unset_user_value() + verify_value("IMPLIED_TRISTATE", "m") + + # Same as above for "y". Only "n" and "y" should be accepted. "m" gets + # promoted to "y". Default should be "y". + + assign_and_verify("IMPLY", "y") + assign_and_verify("IMPLIED_TRISTATE", "n") + assign_and_verify_new_value("IMPLIED_TRISTATE", "m", "y") + assign_and_verify("IMPLIED_TRISTATE", "y") + c["IMPLIED_TRISTATE"].unset_user_value() + verify_value("IMPLIED_TRISTATE", "y") + + # Being implied to either "m" or "y" should give a bool the value "y" + + c["IMPLY"].unset_user_value() + verify_value("IMPLIED_BOOL", "n") + assign_and_verify("IMPLY", "n") + verify_value("IMPLIED_BOOL", "n") + assign_and_verify("IMPLY", "m") + verify_value("IMPLIED_BOOL", "y") + assign_and_verify("IMPLY", "y") + verify_value("IMPLIED_BOOL", "y") + + # A bool implied to "m" or "y" can take the values "n" and "y" + + c["IMPLY"].set_user_value("m") + assign_and_verify("IMPLIED_BOOL", "n") + assign_and_verify("IMPLIED_BOOL", "y") + + c["IMPLY"].set_user_value("y") + assign_and_verify("IMPLIED_BOOL", "n") + assign_and_verify("IMPLIED_BOOL", "y") + # # Choice semantics # @@ -2025,7 +2128,7 @@ def run_selftests(): # Test twice to cover dependency caching for i in range(0, 2): - n_deps = 37 + n_deps = 39 # Verify that D1, D2, .., D are dependent on D verify_dependent("D", ["D{}".format(i) for i in range(1, n_deps + 1)]) # Choices -- cgit v1.2.3