Skip to content
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

Build with nixpkgs scope #10963

Merged
merged 11 commits into from
Jun 26, 2024
Merged

Conversation

roberth
Copy link
Member

@roberth roberth commented Jun 26, 2024

Motivation

Use a Nixpkgs scope instead of top level "pkgs" attributes.
This is a pattern that could be applied in Nixpkgs proper, so it brings us closer to being able to share code with Nixpkgs, which also has to implement the meson migration. (Just closer; more work needs to be done)

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@roberth roberth requested a review from edolstra as a code owner June 26, 2024 00:48
@github-actions github-actions bot added fetching Networking with the outside (non-Nix) world, input locking c api Nix as a C library with a stable interface labels Jun 26, 2024
Comment on lines +33 to 36
# Technically we could just return `pkgs.nixComponents`, but for Hydra it's
# convention to transpose it, and to transpose it efficiently, we need to
# enumerate them manually, so that we don't evaluate unnecessary package sets.
forAllPackages = lib.genAttrs [
Copy link
Member

Choose a reason for hiding this comment

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

Future work: Let's us builtins.attrNames for this list

Copy link
Member

Choose a reason for hiding this comment

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

I think we should move this file to packaging actually. We have a lit of nix files scattered around willy-nilly right now as no directory felt right.

This avoids polluting nixComponents with things that aren't our
components.
Fixes the extraction of passthru tests, which failed for boehmgc
which had many irrelevant ones anyway.
flake.nix Outdated
Comment on lines 196 to 207
nix-internal-api-docs = final.callPackage ./src/internal-api-docs/package.nix {
inherit
fileset
stdenv
versionSuffix
;
};

nix-external-api-docs = final.callPackage ./src/external-api-docs/package.nix {
inherit
fileset
stdenv
versionSuffix
;
Copy link
Member

Choose a reason for hiding this comment

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

I would put these in nixComponents too. They only weren't in the list that became them because (as @edolstra reminded us) we didn't want to cross compile them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we're not using attrNames nixComponents anyway, so it's actually fine.
Early on in the commit series it would have been suitable for that, but makeScope adds some stuff that'd need to be filtered out, which I think is the right trade-off.

flake.nix Outdated
Comment on lines 185 to 230
# in Nixpkgs top level `pkgs` or `nixComponents`.
nixDependencies = lib.makeScope final.newScope (scope: {
inherit stdenv versionSuffix;
libseccomp = final.libseccomp_nix;
boehmgc = final.boehmgc_nix;
libgit2 = final.libgit2_nix;
busybox-sandbox-shell = final.busybox-sandbox-shell or final.default-busybox-sandbox-shell;
};
Copy link
Member

Choose a reason for hiding this comment

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

I would put out the these deps in packaging/<some-file>.nix. too. Instead of overriding prev/super, they would be overriding the regular scope. (Actually the package set function is the same, up to alpha equivalence, but the caller is different.)

The _nix suffix is just poor mans scoping anyways.

Agreed with using self.callPackage of course

Copy link
Member Author

Choose a reason for hiding this comment

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

Done:

  • Inline the _nix definitions (ie they're not in the overlay anymore),
  • Move the scope function into a separate file, passing the free variables by attrset
  • Rename final to pkgs.

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Looks fantastic! Really like the evolution from "ugh our fake is so ugly" to "actually we're demonstrating somebes practices people should emulate"

@Ericson2314 Ericson2314 merged commit 5e4e334 into NixOS:master Jun 26, 2024
10 checks passed
@roberth
Copy link
Member Author

roberth commented Jun 26, 2024

I'm more a fan of transposing the "system" attribute so that it "comes first in the attribute paths you actually think with", or more concretely you only bring it into scope like once, instead of a .${system} on every other line, but other than that, I guess it might be ok for a flake without a framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c api Nix as a C library with a stable interface fetching Networking with the outside (non-Nix) world, input locking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants