-
Notifications
You must be signed in to change notification settings - Fork 48
autogenerate lunatik config from makefile flags #400
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
Makefile
Outdated
| "CONFIG_LUNATIK=m CONFIG_LUNATIK_RUN=m CONFIG_LUNATIK_RUNTIME=y CONFIG_LUNATIK_DEVICE=m \ | ||
| CONFIG_LUNATIK_LINUX=m CONFIG_LUNATIK_NOTIFIER=m CONFIG_LUNATIK_SOCKET=m \ | ||
| CONFIG_LUNATIK_RCU=m CONFIG_LUNATIK_THREAD=m CONFIG_LUNATIK_FIB=m \ | ||
| CONFIG_LUNATIK_DATA=m CONFIG_LUNATIK_PROBE=m CONFIG_LUNATIK_SYSCALL=m \ | ||
| CONFIG_LUNATIK_XDP=m CONFIG_LUNATIK_FIFO=m CONFIG_LUNATIK_XTABLE=m \ | ||
| CONFIG_LUNATIK_NETFILTER=m CONFIG_LUNATIK_COMPLETION=m \ | ||
| CONFIG_LUNATIK_CRYPTO_SHASH=m CONFIG_LUNATIK_CRYPTO_SKCIPHER=m \ | ||
| CONFIG_LUNATIK_CRYPTO_AEAD=m CONFIG_LUNATIK_CRYPTO_RNG=m \ | ||
| CONFIG_LUNATIK_CRYPTO_COMP=m CONFIG_LUNATIK_CPU=m CONFIG_LUNATIK_HID=m \ | ||
| CONFIG_LUNATIK_SIGNAL=m CONFIG_LUNATIK_BYTEORDER=m" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you shouldn't redefine them here, but access this flags from your lua script.. perhaps we could use CPP for this..
genconfig.lua
Outdated
| file:write(string.format('\t"%s",\n', mod)) | ||
| end | ||
| file:write("}\n\n") | ||
| file:write("return config\n") No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please.. mind \n
genconfig.lua
Outdated
| @@ -0,0 +1,61 @@ | |||
| -- SPDX-FileCopyrightText: (c) 2023-2026 Ring Zero Desenvolvimento de Software LTDA | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be your copywrite.. if you are borrowing code from the other script, I recommend you properly modularize it, moving common code to another file and just require it..
|
Thanks for the feedback @lneto !
Since we can't move these flags to a Makefile variable (to avoid Kconfig breakage), and the Preprocessor can't read the Makefile directly ..... like both ways not possible ... I'm unsure how to use CPP here to avoid the duplication. so.... would you prefer I stick to the current approach (explicitly passing the flags) as a temporary solution until the Kconfig PR merges (which will give us a .config file to parse)? or do you have a specific way in mind to pipe these Makefile vars to CPP? |
|
The problem here is not only to avoid duplication, but redefinition of the flags values.. think about the case you disable some module on |
|
hey @abhijeetw035 I took a look at this.. I think we should have two separate approaches.. one for the off-tree build, that is more or less what you already doing.. but assigning the value of the Makefile variable (instead of redefining it).. e.g.,
Another think is the Kconfig, for this, we should assume it will be build on-tree and leverage |
|
That sounds good ! will handle the off-tree logic now, and we can look at the Kconfig / autoconf side in a follow-up once there’s some support for that. I will refactor the Makefile to use variable references as u suggested so it correctly respects user overrides Pushing those changes shortly! |
c6f286f to
edb2722
Compare
1f27a66 to
7e4df5f
Compare
|
hey @abhijeetw035 can you test this along with our OpenWRT feed? My main concern now is that these autogen script don't break it. Thanks! Let me know if you need help. |
most specifically, you should call your autogen script here https://github.com/luainkernel/openwrt_feed/blob/openwrt-23.05/kernel/lunatik/Makefile#L127. |
see this discussion, for instance, #387 |
Suree will go through it and update you. |
Well, I will need to work on this for #395. Perhaps you want to put this issue on hold until we merge the other and work on another issue for a while. It might be better usage of both of our time. What do you think? |
yes suree ....it makes sense to synchronize the OpenWRT work since both PRs touch the same integration points. |
genconfig.lua
Outdated
| local KERNEL_RELEASE = arg[1] | ||
| local OUTPUT_DIR = arg[2] | ||
| local CONFIGS = arg[3] | ||
|
|
||
| if not KERNEL_RELEASE or not OUTPUT_DIR or not CONFIGS then | ||
| exit("usage: lua genconfig.lua <KERNEL_RELEASE> <OUTPUT_DIR> <CONFIGS>") | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other PR is merged.. let's avoid replicate code and sharing the common code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mabe a better approach is just to insert your logic on gendefines.lua, se we will have a single entry point on the Makefile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abhijeetw035 it's weird, it looks like you are re-adding the changes that are already in the base.. did you rebase your branch on master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abhijeetw035 it's weird, it looks like you are re-adding the changes that are already in the base.. did you rebase your branch on master?
that was an artifact from squashing the merge commit. I have rebased onto origin/master to filter out the upstream changes. The diff should be clean now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, see this #395 (comment).
We don't use merge commits..
--ff-only
@abhijeetw035 it's weird, it looks like you are re-adding the changes that are already in the base.. did you rebase your branch on master?
fixing it ...
7c23a62 to
6c1535f
Compare
gendefines.lua
Outdated
| @@ -1,5 +1,6 @@ | |||
| -- | |||
| -- SPDX-FileCopyrightText: (c) 2026 Ashwani Kumar Kamal <ashwanikamal.im421@gmail.com> | |||
| -- SPDX-FileCopyrightText: (c) 2026 Abhijeet Waghmare <abhijeetw035@gmail.com> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually don't do this, we keep the original copyright unless there's very significant change in the original, such as major refactor. Which is not the case of adding new features. If you want to modularize it and move the config portion to a sublib, it's alright to add your copyright to the new file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have autogen/{init,defines,config}.lua for instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rebased onto the latest master and squashed everything into a single commit to keep the history linear.
Changes:
copyright: removed my copyright header from gendefines.lua .
runtime: updated bin/lunatik to load the generated config dynamically (added /lib/modules/lua to package path).
Regarding the modularization (autogen/{defines,config}.lua): I agree that is a cleaner architecture. however, to keep this PR focused and safe, i think we should merge this functional version first. I can then submit a follow-up PR dedicated to splitting the scripts and organizing the autogen/ structure properly. what do you think?
I don't think it's the problem as master (and other branches are alright with CI build).. |
Makefile
Outdated
|
|
||
| defines: | ||
| @mkdir -p autogen/lunatik autogen/linux | ||
| CC='${CC}' lua gendefines.lua "$(KERNEL_RELEASE)" "$(INCLUDE_PATH)" "autogen/linux" "$(LUNATIK_CONFIG_FLAGS)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be lua5.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes i was checking if the lua works .like before ci build was failing due to that
/bin/sh: 1: lua5.4: not found
gendefines.lua
Outdated
| exit("cannot open " .. filepath .. ": " .. err) | ||
| end | ||
|
|
||
| file:write("-- auto-generated, do not edit\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should avoid repeat this, instead we should proper modularize the code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes.... I will submit a follow-up PR immediately after this merges to split gendefines.lua into proper modules. I kept it inline here to keep this PR focused on the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my point is that we should at least avoid to repeat strings that can be added to a local var or very simple patterns that can be added to a local function..
538ac02 to
320d4e0
Compare
I checked the build.yaml file and I see the issue. The workflow installs Since my PR requires running a Lua script during the build, the runner fails to find the binary. |
Makefile
Outdated
|
|
||
| defines: | ||
| ${shell CC='$(CC)' lua5.4 gendefines.lua $(KERNEL_RELEASE) $(INCLUDE_PATH) autogen/linux} | ||
| @mkdir -p autogen/lunatik autogen/linux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't do this.. it's handled by the .gitignore
Makefile
Outdated
| defines: | ||
| ${shell CC='$(CC)' lua5.4 gendefines.lua $(KERNEL_RELEASE) $(INCLUDE_PATH) autogen/linux} | ||
| @mkdir -p autogen/lunatik autogen/linux | ||
| CC='${CC}' lua5.4 gendefines.lua "$(KERNEL_RELEASE)" "$(INCLUDE_PATH)" "autogen/linux" "$(LUNATIK_CONFIG_FLAGS)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you remove ${shell ...}? this seems to be CI issue to me.
gendefines.lua
Outdated
| if key == "CONFIG_LUNATIK" then | ||
| name = "lunatik" | ||
| elseif key == "CONFIG_LUNATIK_RUN" then | ||
| name = "lunatik_run" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use the same approach of ignore_list here.. having an overlay_list (couldn't figure a better name)..
gendefines.lua
Outdated
| local modules = {} | ||
|
|
||
| for token in configs:gmatch("%S+") do | ||
| local key = token:match("(CONFIG_[%w_]+)=[my]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's avoid use key as a variable name and have more meaningful names instead..
the defines
master is already running gendefines.lua on build.. I think the problem is to suppress |
320d4e0 to
9365856
Compare
gendefines.lua
Outdated
| end | ||
| end | ||
|
|
||
| local filepath = "autogen/lunatik/config.lua" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should come from the args, like on defines. You coul pass autogen dir and concat subdir on both cases.
gendefines.lua
Outdated
| local CONFIG_FLAGS = arg[4] | ||
|
|
||
| if not KERNEL or not INCLUDE_PATH or not OUTPUT_DIR then | ||
| exit("usage: lua gendefines.lua <KERNEL> <INCLUDE_PATH> <OUTPUT_DIR>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be updated with CONFIG_FLAGS
gendefines.lua
Outdated
| write_module(spec, constants, macro_names) | ||
| end | ||
|
|
||
| if CONFIG_FLAGS then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be checked along with the other args.. it should be a requirement..
9365856 to
8ef6d34
Compare
|
@sneaky-potato can you take a look at this as well? thanks! |
gendefines.lua
Outdated
| file:write("return config\n") | ||
| end | ||
|
|
||
| local linux_dir = OUTPUT_DIR .. "/linux" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| local linux_dir = OUTPUT_DIR .. "/linux" |
gendefines.lua
Outdated
| local cpp_output = preprocess(spec.header) | ||
| local constants, macro_names = collect_constants(cpp_output, spec.prefix) | ||
| write_module(spec, constants, macro_names) | ||
| write_module(spec, constants, macro_names, linux_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| write_module(spec, constants, macro_names, linux_dir) | |
| write_module(spec, constants, macro_names, OUTPUT_DIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep the script uniform, let's pass the same base directory to both write_module and generate_build_config
gendefines.lua
Outdated
| local function write_module(spec, constants, macro_names, dir) | ||
| local filepath = string.format("%s/%s.lua", dir, spec.module_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| local function write_module(spec, constants, macro_names, dir) | |
| local filepath = string.format("%s/%s.lua", dir, spec.module_name) | |
| local function write_module(spec, constants, macro_names, output_dir) | |
| local filepath = string.format("%s/linux/%s.lua", output_dir, spec.module_name) |
8ef6d34 to
98dce54
Compare
|
@sneaky-potato, it seems @abhijeetw035 has committed your suggestions. Please, let me know if we can merge it. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we can merge this




Closes #396
This PR removes the hardcoded module list in
bin/lunatikand replaces it with an auto-generated configuration file created during the build process.Changes
genconfig.lua): ParsesCONFIG_LUNATIK_*flags to generateautogen/lunatik/config.luacontaining themodulestable and kernel release version.configtarget to execute the generator. The configuration flags are passed explicitly to the script to avoid modifying thealltarget variablesbin/lunatiktorequire("lunatik.config")instead of using a static list.autogen/lunatik/.gitignoreto ensure the directory structure exists on fresh clones while ignoring generated artifacts.I included an ignore list in
genconfig.luato filter out non-module flags (e.g.,INSTALL_EXAMPLES,INSTALL_TESTS) introduced in pending PR #387.