pkg/lua: model in kconfig #18017
Merged
pkg/lua: model in kconfig #18017
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| select MODULE_PRINTF_FLOAT | ||
| select MODULE_LUA-CONTRIB | ||
|
|
||
| select MODULE_LIBC_GETTIMEOFDAY if !CPU_NATIVE |
There was a problem hiding this comment.
This is actually needed for native, according to Makefile.dep, also should be brought when newlib_syscalls_default is in
Suggested change
| select MODULE_LIBC_GETTIMEOFDAY if !CPU_NATIVE | |
| select MODULE_LIBC_GETTIMEOFDAY if CPU_NATIVE || MODULE_NEWLIB_SYSCALLS_DEFAULT |
There was a problem hiding this comment.
Interesting. This was my first attempt as well. But the Kconfig resolution failed with:
TEST_KCONFIG=1 make -C examples/lua_basic/
make: Entering directory '/work/riot/RIOT/examples/lua_basic'
=== [ATTENTION] Testing Kconfig dependency modelling ===
Traceback (most recent call last):
File "/work/riot/RIOT/dist/tools/kconfiglib/genconfig.py", line 446, in <module>
main()
File "/work/riot/RIOT/dist/tools/kconfiglib/genconfig.py", line 393, in main
kconf = RiotKconfig(args.kconfig_filename, warn_to_stderr=False)
File "/work/riot/RIOT/dist/tools/kconfiglib/kconfiglib.py", line 947, in __init__
self._init(filename, warn, warn_to_stderr, encoding)
File "/work/riot/RIOT/dist/tools/kconfiglib/kconfiglib.py", line 1120, in _init
check_dep_loop_sym(sym, False)
File "/work/riot/RIOT/dist/tools/kconfiglib/kconfiglib.py", line 6606, in _check_dep_loop_sym
else _check_dep_loop_sym(dep, False)
File "/work/riot/RIOT/dist/tools/kconfiglib/kconfiglib.py", line 6610, in _check_dep_loop_sym
return _found_dep_loop(loop, sym)
File "/work/riot/RIOT/dist/tools/kconfiglib/kconfiglib.py", line 6714, in _found_dep_loop
raise KconfigError(msg)
kconfiglib.KconfigError:
Dependency loop
===============
MODULE_LIBC_GETTIMEOFDAY (defined at /work/riot/RIOT/sys/Kconfig:55, /work/riot/RIOT/sys/Kconfig.newlib:24), with definition...
config MODULE_LIBC_GETTIMEOFDAY
bool "Support for gettimeofday()"
select MODULE_XTIMER
select MODULE_ZTIMER64_XTIMER_COMPAT if MODULE_ZTIMER_XTIMER_COMPAT
config MODULE_LIBC_GETTIMEOFDAY
bool
select MODULE_NEWLIB_SYSCALLS_DEFAULT if MODULE_NEWLIB
select MODULE_XTIMER
select MODULE_ZTIMER64_XTIMER_COMPAT if MODULE_ZTIMER_XTIMER_COMPAT
depends on MODULE_NEWLIB
help
Support for gettimeofday()
(select-related dependencies: PACKAGE_LUA && MODULE_NEWLIB_SYSCALLS_DEFAULT && CPU_NATIVE && TEST_KCONFIG && HAS_ARCH_32BIT)
...depends on MODULE_NEWLIB_SYSCALLS_DEFAULT (defined at /work/riot/RIOT/sys/Kconfig.newlib:16), with definition...
config MODULE_NEWLIB_SYSCALLS_DEFAULT
bool
default y
select MODULE_DIV
depends on !HAVE_CUSTOM_NEWLIB_SYSCALLS && MODULE_NEWLIB
help
Default implementation of newlib system calls.
(select-related dependencies: MODULE_LIBC_GETTIMEOFDAY && MODULE_NEWLIB && MODULE_NEWLIB)
...depends again on MODULE_LIBC_GETTIMEOFDAY (defined at /work/riot/RIOT/sys/Kconfig:55, /work/riot/RIOT/sys/Kconfig.newlib:24)
I had the same kind of failures with nucleo-f746zg. The current version of this PR works for both and the resolved dependencies are the same between Make and Kconfig.
There was a problem hiding this comment.
Hmm I think there is something fishy with the modelling of MODULE_LIBC_GETTIMEOFDAY, let me take a look
There was a problem hiding this comment.
For what I see, we should be able to get rid of the MODULE_LIBC_GETTIMEOFDAY definition in Kconfig.newlib, which is introducing unnecessary dependencies. MODULE_NEWLIB_SYSCALLS_DEFAULT defaults to y, so the extra select is redundant. @fjmolinas is this correct?
I'd propose:
diff --git a/sys/Kconfig.newlib b/sys/Kconfig.newlib
index 2f846e80e3..b1ba0e6a34 100644
--- a/sys/Kconfig.newlib
+++ b/sys/Kconfig.newlib
@@ -21,14 +21,6 @@ config MODULE_NEWLIB_SYSCALLS_DEFAULT
help
Default implementation of newlib system calls.
-config MODULE_LIBC_GETTIMEOFDAY
- bool
- select MODULE_NEWLIB_SYSCALLS_DEFAULT if MODULE_NEWLIB
- select MODULE_XTIMER
- select MODULE_ZTIMER64_XTIMER_COMPAT if MODULE_ZTIMER_XTIMER_COMPAT
- help
- Support for gettimeofday()
-
endif # MODULE_NEWLIB
config HAVE_CUSTOM_NEWLIB_SYSCALLS
| config PACKAGE_LUA | ||
| bool "LUA language package" | ||
| depends on TEST_KCONFIG | ||
| depends on HAS_ARCH_32BIT |
There was a problem hiding this comment.
There are some blacklisted architectures as well
Suggested change
| depends on HAS_ARCH_32BIT | |
| depends on HAS_ARCH_32BIT | |
| depends on !HAS_ARCH_MIPS32R2 | |
| depends on !HAS_ARCH_RISCV |
| bool "LUA language package" | ||
| depends on TEST_KCONFIG | ||
| depends on HAS_ARCH_32BIT | ||
|
|
There was a problem hiding this comment.
In the makefile there seems to be an extra condition in Makefile.dep blacklisting the picolibc feature, but I think they meant something like:
depends on !MODULE_PICOLIBC
Not 100% sure though
4691423 to
0b90204
Compare
0b90204 to
cf0b3de
Compare
MrKevinWeiss
approved these changes
May 3, 2022
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
As the title says.
Testing procedure
Green Murdock
Issues/PRs references
None