Flake schemas fixes

[edolstra]

• Mark legacyPackages as legacy (see Add isLegacy flake schema output attribute nix-src#381).
• Handle modules that are paths. E.g. Nixpkgs has a nixosModules.readOnlyPkgs like this:

  readOnlyPkgs = ./nixos/modules/misc/nixpkgs/read-only.nix;
  

Summary by CodeRabbit

Release Notes

• New Features

  • Legacy packages are now explicitly marked in the package schema for improved categorization and identification.

• Refactor

  • Enhanced module checking to intelligently handle both direct module imports and path-based references, improving configuration flexibility.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR updates the flake schema by implementing path-aware module checking that conditionally imports modules based on type, and adds an isLegacy flag to legacy package entries within the per-system schema definition.

Changes

Cohort / File(s)	Summary
Module Import Enhancement flake.nix	Updated checkModule function to accept paths and conditionally import them, while maintaining backward compatibility with direct module objects. Added isLegacy = true flag to per-system entries in legacyPackages schema.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• Add schema roles #40: Modifies the same legacyPackagesSchema by adding role and appendSystem attributes alongside the new isLegacy flag
• Change the derivation schema attribute to derivationAttrPath #52: Also extends legacyPackages schema in flake.nix, replacing embedded derivations with derivationAttrPath references

Suggested reviewers

• cole-h
• lucperkins

Poem

🐰 A path-aware mind now checks each module with care,
Legacy packages wear their badges fair,
Conditional imports handle both forms with grace,
Schema updated, in its proper place! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Flake schemas fixes' is vague and generic, using non-descriptive language that doesn't convey the specific changes being made.	Consider a more specific title such as 'Add path-aware module checking and mark legacyPackages as legacy' to better reflect the actual changes.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch legacy-packages

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

flake.nix (1)

111-145: ⚠️ Potential issue | 🟠 Major

Propagate isLegacy to the recursive nodes, not just the system wrapper.

Line 113 marks only the top-level system entry. The package and intermediate nodes created on Lines 123-139 never get that flag, and mkChildren on Line 406 does not inherit parent metadata, so nested legacyPackages entries still look non-legacy to consumers that inspect leaves.

Proposed fix

                     builtins.mapAttrs (
                       attrName: attrs:
                       # Necessary to deal with `AAAAAASomeThingsFailToEvaluate` etc. in Nixpkgs.
                       self.lib.try (
                         if attrs.type or null == "derivation" then
                           {
+                            isLegacy = true;
                             forSystems = [ attrs.system ];
                             shortDescription = attrs.meta.description or "";
                             derivationAttrPath = [ ];
                             what = "package";
                           }
                         else
                         # Recurse at the first and second levels, or if the
                         # recurseForDerivations attribute if set.
                         if attrs.recurseForDerivations or false then
                           {
+                            isLegacy = true;
                             children = recurse (prefix + attrName + ".") attrs;
                           }
                         else
                           {
+                            isLegacy = true;
                             what = "unknown";
                           }
                       ) (throw "failed")
                     ) attrs;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@flake.nix` around lines 111 - 145, The top-level isLegacy flag is only
applied to the system wrapper but not to the recursive nodes produced by
recurse, so update the objects returned inside recurse (the branches that
produce the package node with what = "package", the branch that sets children,
and the fallback branch that sets what = "unknown") to include isLegacy = true
so that intermediate and leaf nodes inherit the legacy marker; modify the return
objects inside the self.lib.try(...) block in the recurse function (used with
builtins.mapAttrs on packagesForSystem and referenced by recurse and mkChildren)
to add isLegacy = true to each created attrs object.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@flake.nix`:
- Around line 9-14: The current checkModule function does an import of module_
when builtins.isPath module_ is true, which can throw if the file is unreadable;
change the import to use builtins.tryEval so import-time errors are caught and
you then validate the resulting value's type (e.g., check the tryEval result for
success and then use builtins.isAttrs or builtins.isFunction on the evaluated
value). Update checkModule (and the local symbol module/module_) to use tryEval
when module_ is a path and treat failed tryEval as returning false rather than
letting the error propagate.

---

Outside diff comments:
In `@flake.nix`:
- Around line 111-145: The top-level isLegacy flag is only applied to the system
wrapper but not to the recursive nodes produced by recurse, so update the
objects returned inside recurse (the branches that produce the package node with
what = "package", the branch that sets children, and the fallback branch that
sets what = "unknown") to include isLegacy = true so that intermediate and leaf
nodes inherit the legacy marker; modify the return objects inside the
self.lib.try(...) block in the recurse function (used with builtins.mapAttrs on
packagesForSystem and referenced by recurse and mkChildren) to add isLegacy =
true to each created attrs object.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bbcfc8b9-724f-41ac-89ce-1c4e62700e4e

📥 Commits

Reviewing files that changed from the base of the PR and between 834aaad and c5f9335.

📒 Files selected for processing (1)

• flake.nix

[gustavderdrache]

Do we have tests for this? If not, can we add some?

[edolstra]

Yeah we have a test (tests/legacyPackages).