Skip to content

nix develop: Respect legacyPackages#413

Merged
edolstra merged 1 commit intomainfrom
eelcodolstra/nix-369
Apr 6, 2026
Merged

nix develop: Respect legacyPackages#413
edolstra merged 1 commit intomainfrom
eelcodolstra/nix-369

Conversation

@edolstra
Copy link
Copy Markdown
Collaborator

@edolstra edolstra commented Apr 5, 2026

Motivation

Context

Summary by CodeRabbit

  • New Features

    • Extended support in the nix develop command to work with legacyPackages flake outputs, enabling developers to reference legacy package definitions.
  • Tests

    • Added functional test coverage validating correct attribute resolution for nix develop when using legacyPackages in flake definitions.

@edolstra edolstra enabled auto-merge April 5, 2026 09:32
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 5, 2026

📝 Walkthrough

Walkthrough

This PR adds a new role declaration (roles.nix-develop = { };) to the legacyPackagesSchema in the Nix flake schema definitions, accompanied by a functional test that verifies nix develop behavior when only legacyPackages is exposed without explicit attribute selection.

Changes

Cohort / File(s) Summary
Schema Role Declaration
src/libcmd/builtin-flake-schemas.nix
Added roles.nix-develop = { }; entry to legacyPackagesSchema, extending the role set for legacy packages output schema.
Flake Develop Regression Test
tests/functional/flakes/develop.sh
Added test case verifying that nix develop . fails without explicit attribute when only legacyPackages.$system.default is available, while nix develop .#default succeeds with correct output.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested labels

flake-regression-test

Suggested reviewers

  • grahamc
  • cole-h

Poem

🐰 A role for the develop, declared with care,
In legacyPackages, it now dwells there.
With test in hand, we verify its way—
No implicit magic, just explicit play! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'nix develop: Respect legacyPackages' directly and specifically describes the main change: adding support for legacyPackages in the nix develop command.
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 docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch eelcodolstra/nix-369

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

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/functional/flakes/develop.sh (1)

210-210: Make the negative assertion verify the failure reason.

(! nix develop ...) passes on any non-zero exit, including unrelated breakage. Prefer asserting stderr to lock this test to the schema-resolution failure mode.

Proposed test hardening
-(! nix develop . -L --command sh -c "echo \$x")
+expectStderr 1 nix develop . -L --command sh -c "echo \$x" |
+  grepQuiet "does not provide attribute"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/functional/flakes/develop.sh` at line 210, Replace the loose negative
assertion that uses (! nix develop . -L --command sh -c "echo \$x") with an
assertion that captures stderr and verifies the failure is the schema-resolution
error: run nix develop . -L --command sh -c "echo \$x" (without the leading !)
capture its exit status and stderr, assert the exit is non-zero, and grep/assert
stderr contains the schema-resolution failure text (e.g. a phrase like "could
not resolve" or "schema" / "flake input" that matches the flake
schema-resolution error) so the test fails only for unrelated breakage and
passes only when the expected schema-resolution error occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/functional/flakes/develop.sh`:
- Line 210: Replace the loose negative assertion that uses (! nix develop . -L
--command sh -c "echo \$x") with an assertion that captures stderr and verifies
the failure is the schema-resolution error: run nix develop . -L --command sh -c
"echo \$x" (without the leading !) capture its exit status and stderr, assert
the exit is non-zero, and grep/assert stderr contains the schema-resolution
failure text (e.g. a phrase like "could not resolve" or "schema" / "flake input"
that matches the flake schema-resolution error) so the test fails only for
unrelated breakage and passes only when the expected schema-resolution error
occurs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 55d74342-009e-4b2c-bece-9561b5bbbaf2

📥 Commits

Reviewing files that changed from the base of the PR and between 53f56d2 and 8a1b2d9.

📒 Files selected for processing (2)
  • src/libcmd/builtin-flake-schemas.nix
  • tests/functional/flakes/develop.sh

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 5, 2026

@github-actions github-actions bot temporarily deployed to pull request April 5, 2026 09:37 Inactive
@edolstra edolstra added this pull request to the merge queue Apr 5, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 5, 2026
@edolstra edolstra added this pull request to the merge queue Apr 6, 2026
Merged via the queue into main with commit 9ef16ad Apr 6, 2026
28 checks passed
@edolstra edolstra deleted the eelcodolstra/nix-369 branch April 6, 2026 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants