Skip to content

Backport changes from upstream #14189#225

Merged
cole-h merged 1 commit into
mainfrom
backport-14189
Oct 9, 2025
Merged

Backport changes from upstream #14189#225
cole-h merged 1 commit into
mainfrom
backport-14189

Conversation

@edolstra
Copy link
Copy Markdown
Collaborator

@edolstra edolstra commented Oct 9, 2025

Motivation

Context

Summary by CodeRabbit

  • Performance Improvements

    • Reduced overhead when processing structured attributes, improving efficiency in scenarios that build export reference graphs.
  • Tests

    • Updated test suite to require a Nix daemon version 2.33 or newer, aligning with current environments and guidance.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Oct 9, 2025

Walkthrough

Updates DerivationOptions::fromStructuredAttrs to use std::move when inserting into exportReferencesGraph. Adjusts a functional test to require Nix daemon version 2.33 instead of 2.4pre20210712, and updates related comments.

Changes

Cohort / File(s) Summary
Move semantics optimization
src/libstore/derivation-options.cc
Insert map values with std::move during exportReferencesGraph construction to avoid copies.
Test daemon version gate update
tests/functional/structured-attrs.sh
Bumps requireDaemonNewerThan from "2.4pre20210712" to "2.33" and refreshes comments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I nudge a byte, then hop along,
Move it, don’t copy—swift and strong.
Tests now ask for 2.33,
The burrow hums more snappily.
Carrots cached, my tail alight—
Small tweaks make code feel right. 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The current title indicates a backport operation but does not describe the actual changes being applied, making it too generic for readers to understand the substantive updates without inspecting the diff. Please revise the title to clearly summarize the main changes, for example “Backport move semantics in DerivationOptions::fromStructuredAttrs and update structured-attrs test daemon version.”
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch backport-14189

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.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 592e828 and e2a1944.

📒 Files selected for processing (2)
  • src/libstore/derivation-options.cc (1 hunks)
  • tests/functional/structured-attrs.sh (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build_x86_64-linux / build
  • GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (1)
src/libstore/derivation-options.cc (1)

235-235: LGTM! Good use of move semantics.

This optimization correctly uses std::move(ss) to avoid an unnecessary copy when inserting the StringSet into the map. Since ss is a loop-local variable that's not used after this point, moving it is both safe and efficient.

Comment on lines +5 to +6
# https://github.com/NixOS/nix/pull/14189
requireDaemonNewerThan "2.33"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify the daemon version "2.33" is valid.

The version requirement changed from "2.4pre20210712" to "2.33", which appears to be a significant version jump. Please confirm that "2.33" is a valid and released Nix daemon version, as this could cause the test to fail or be skipped incorrectly if the version doesn't exist or isn't properly formatted.

Run the following script to check for the existence and validity of Nix version 2.33:

Additionally, search the web to confirm Nix version 2.33 exists:


🏁 Script executed:

#!/bin/bash
# Check if Nix version 2.33 exists and is valid

# Search for version references in the codebase
echo "=== Searching for version 2.33 references ==="
rg -nC3 '2\.33' --type=sh --type=cc --type=cpp --type=h

# Check release notes or version files
echo -e "\n=== Checking for version documentation ==="
fd -e md -e txt -e rst | xargs rg -l 'version.*2\.33|2\.33.*release' 2>/dev/null || echo "No version documentation found"

# Look for the requireDaemonNewerThan function definition
echo -e "\n=== Finding requireDaemonNewerThan implementation ==="
rg -A10 'requireDaemonNewerThan\s*\(' --type=sh

Length of output: 831


🌐 Web query:

What is Nix version 2.33? Is it a released version of the Nix package manager?

💡 Result:

Short answer: No — Nix 2.33 is not a released version (as of Oct 9, 2025). The latest announced release is Nix 2.32.0 (Oct 7, 2025). See the Nix release announcement and the official release-notes pages. [1][2]

Sources:

  1. Nix 2.32.0 release announcement (Discourse). [1]
  2. Nix release notes / manual (release pages listing recent versions). [2]

Invalid Nix daemon version “2.33”
The test requires a daemon ≥ 2.33, but no such release exists (latest is 2.32.0). Update the version requirement to a valid released version or confirm the upcoming 2.33 release before merging.

🤖 Prompt for AI Agents
In tests/functional/structured-attrs.sh around lines 5 to 6, the test currently
requires a Nix daemon version "2.33" which does not exist; change the
requirement to a valid released version (for example requireDaemonNewerThan
"2.32.0") or, if you intend to gate this on an unreleased 2.33, hold off merging
until that release is published and then update the string to the actual
released version number.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 9, 2025

@github-actions github-actions Bot temporarily deployed to pull request October 9, 2025 09:28 Inactive
@cole-h cole-h added this pull request to the merge queue Oct 9, 2025
Merged via the queue into main with commit e8da68a Oct 9, 2025
35 checks passed
@cole-h cole-h deleted the backport-14189 branch October 9, 2025 13:59
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