Add module builder #48
Conversation
| ''; | ||
| }; | ||
|
|
||
| subtarget = lib.mkOption { |
There was a problem hiding this comment.
I called it subtarget instead of variant, because it seems like OpenWRT uses this name.
| # TODO: move arch to profiles | ||
| detectedArch = hashedTarget.packagesArch; |
There was a problem hiding this comment.
arch is already a property in profiles.json. I feel like it's more fitting to get this info from there, than from the feeds, since it's a property of a specific board/target
|
I'd like to review but I haven't been using the imagebuilder (or openwrt). |
|
I like this. Please feel encouraged to follow through. |
pedorich-n
force-pushed
the
add-module
branch
from
7955ae5 to
177716b
Compare
|
Thanks for your encouragement, @astro ! I've updated the docs module, exposing both markdown and json. At a certain iteration, I realized it looked almost like https://github.com/astro/microvm.nix/blob/2ae08de8e8068b00193b9cfbc0acc9dfdda03181/pkgs/doc.nix, so I just copied most of the stuff from there 😅 I also added support for "separate Kmods" and tested the build with both "old" (23.05.5) and "new" (24.10.0-rc4) versions. Builds fine. |
|
Happy New Year everybody! I've added a new package to generate/update module docs and a GitHub flow to do it from GHA. I think this PR is ready for a proper review now. |
Co-authored-by: Sandro <[email protected]>
|
Is there any chance I could get a review @SuperSandro2000 @astro ? 👀 |
| --replace "SHELL:=/usr/bin/env bash" "SHELL:=${pkgs.runtimeShell}" \ | ||
| --replace "/usr/bin/env true" "${pkgs.coreutils}/bin/true" \ | ||
| --replace "/usr/bin/env false" "${pkgs.coreutils}/bin/false" |
There was a problem hiding this comment.
Should we use --replace-fail here?
| release = lib.mkOption { | ||
| type = lib.types.nonEmptyStr; | ||
| default = latestRelease; | ||
| defaultText = lib.literalMD "Value of `latest-release.nix`"; |
There was a problem hiding this comment.
| defaultText = lib.literalMD "Value of `latest-release.nix`"; | |
| defaultText = "Value of `latest-release.nix`"; |
literalMD doesn't exist anymore in unstable
There was a problem hiding this comment.
I would not commit this file and generate it on the fly for github pages or use https://github.com/nuschtos/search
There was a problem hiding this comment.
I would be happy to have the action build and push to GH pages like it is already happening over here: https://github.com/astro/microvm.nix/blob/main/.github/workflows/doc.yml
|
I would really need to try this to know fore sure if it works perfectly but I am not keen on bricking my router right now in case of any issues 😓 |
| ]; | ||
| }) options; | ||
|
|
||
| transformOptions = opt: opt // { |
There was a problem hiding this comment.
| transformOptions = opt: opt // { | |
| # Fix source links in the generated options docs | |
| transformOptions = opt: opt // { |
| if matches != [ ] | ||
| then builtins.getAttr (builtins.elemAt matches 0) variantFiles | ||
| else | ||
| builtins.throw '' |
There was a problem hiding this comment.
| builtins.throw '' | |
| throw '' |
| ''; | ||
| }; | ||
|
|
||
| rootFsPartSize = lib.mkOption { |
There was a problem hiding this comment.
Nit: elsewhere you mention rootfs so I wonder if this shouldn't be rootfsPartSize?
| # TODO: rename to subtarget | ||
| , variant ? "generic" |
There was a problem hiding this comment.
| # TODO: rename to subtarget | |
| , variant ? "generic" | |
| # DEPRECATED: use `subtarget` | |
| , variant ? "generic" | |
| , subtarget ? variant |
Then alias the attrset with }@args and add lib.warnIf (args ? variant) "Using deprecated argument variant, use subtarget instead" before the result of this file (after the top-level let ... in).
| @@ -24,6 +24,7 @@ in rec { | |||
| ) | |||
| ) hashes.targets | |||
| else | |||
| # TODO: check if file exists first | |||
There was a problem hiding this comment.
Why do we have to?
There's builtins.readDir and builtins.pathExists.
| let | ||
| image = pkgs.callPackage ./examples/example-module.nix { inherit (self.lib) build-module; }; | ||
| in | ||
| # Wrap `image` once to avoid `nix flake show` breaking on IFD |
There was a problem hiding this comment.
I would be happy to have the action build and push to GH pages like it is already happening over here: https://github.com/astro/microvm.nix/blob/main/.github/workflows/doc.yml
| mkCopyStatement = file: '' | ||
| ( | ||
| target_rel=${lib.escapeShellArg file.target} | ||
| target="files/''${target_rel#/}" | ||
| ${lib.optionalString (lib.pathIsRegularFile file.source) '' | ||
| mkdir -p "$(dirname "''${target}")" | ||
| ''} | ||
| cp -r --no-preserve=all "${file.source}" "''${target}" | ||
| ) | ||
| ''; | ||
|
|
||
| copyFiles = pkgs.writeScript "copy-openwrt-files" ( | ||
| lib.concatLines (builtins.map mkCopyStatement config.files) | ||
| ); |
There was a problem hiding this comment.
I actually prefer having once-used definitions inline for readability. The indentation isn't very deep here so I think it would be fine:
| mkCopyStatement = file: '' | |
| ( | |
| target_rel=${lib.escapeShellArg file.target} | |
| target="files/''${target_rel#/}" | |
| ${lib.optionalString (lib.pathIsRegularFile file.source) '' | |
| mkdir -p "$(dirname "''${target}")" | |
| ''} | |
| cp -r --no-preserve=all "${file.source}" "''${target}" | |
| ) | |
| ''; | |
| copyFiles = pkgs.writeScript "copy-openwrt-files" ( | |
| lib.concatLines (builtins.map mkCopyStatement config.files) | |
| ); | |
| copyFiles = pkgs.writeScript "copy-openwrt-files" ( | |
| lib.concatMapStrings (file: '' | |
| ( | |
| target_rel=${lib.escapeShellArg file.target} | |
| target="files/''${target_rel#/}" | |
| ${lib.optionalString (lib.pathIsRegularFile file.source) '' | |
| mkdir -p "$(dirname "''${target}")" | |
| ''} | |
| cp -r --no-preserve=all "${file.source}" "''${target}" | |
| ) | |
| '') config.files | |
| ); |
|
Me and @MarcelCoding have some interest in using this for future infrastructure. Hopefully we find some time bringing this forward together. |
|
I'm sorry I haven't been active. I really want to see this working, but I struggled to find the willpower to get back to it. I'd still love to help if we can find a way to contribute incrementally. |
|
I've successfully deployed my testing AP using this PR in https://gitea.c3d2.de/zentralwerk/network/src/branch/openwrt-module. (I've used my fork to get up-to-date hashes) |
This PR adds a module interface to the builder derivation. This brings benefits such as strict options definition, better type-checks, and flexibility for future improvements or modifications.
I left the old builder in place for backward compatibility. In the future, it could be removed, and an "adapter" could be installed to convert old inputs into the new module system and maintain backward compatibility.
The module is inspired by @tomeon's nix-openwrt-imagebuilder-module project, but it's structured differently and the module itself is simpler. Please let me know if you feel it isn't fair to your work.
Opening this as a draft to gather opinions.
If everything is fine I will add module documentation and update README to tell users to use
build-modulein a follow-up PR to not pollute this one.I also moved examples into a dedicated folder, since we have 4 of them now.
Closes #22