Skip to content

pkgs/top-level/release.nix: enable pkgsLLVM #330567

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.

Sign up for GitHub

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

Open
RossComputerGuy wants to merge 4 commits into
base: master
Choose a base branch
from
Open

pkgs/top-level/release.nix: enable pkgsLLVM #330567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d3a26a4
pkgs/top-level/release.nix: enable pkgsLLVM
RossComputerGuy Nov 27, 2024
8a0da10
pkgs/top-level/stage.nix: guard pkgsLLVM on Darwin
RossComputerGuy Nov 27, 2024
18ca929
pkgs/top-level/release-attrpaths-superset.nix: enable pkgsLLVM
RossComputerGuy Nov 27, 2024
f30794e
pkgs/top-level: add subpackage exclusion from hydra
RossComputerGuy Dec 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ci/eval/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ let
chunkSize,
checkMeta ? true,
includeBroken ? true,
includeSubpackages ? evalSystem != "aarch64-darwin" && evalSystem != "x86_64-darwin",
Copy link
Contributor

@wolfgangwalther wolfgangwalther Dec 22, 2024

Choose a reason for hiding this comment

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

Those checks are related to pkgsLLVM specifically, so I think it's the wrong place to put this knowledge.

Assume we are going to add e.g. pkgsStatic eventually, too - we might need different conditions here.

Is this even needed with the "if darwin then self" check you currently have in stage.nix?

Copy link
Member Author

@RossComputerGuy RossComputerGuy Dec 22, 2024

Choose a reason for hiding this comment

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

The problem is I need to disable eval for Darwin but checking if it's for Darwin isn't as simple as it looks.

Copy link
Contributor

@wolfgangwalther wolfgangwalther Dec 22, 2024

Choose a reason for hiding this comment

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

The problem is I need to disable eval for Darwin

That's what I was questioning. With this line:

pkgsLLVM = if stdenv.hostPlatform.isDarwin then self else

eval should succeed on darwin, right?

In fact, it should just refer to the same attributes as on the top-level, so eval on darwin should be a no-op? What am I missing?

Copy link
Member Author

@RossComputerGuy RossComputerGuy Dec 22, 2024

Choose a reason for hiding this comment

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

No, it complains about missing attributes and fails to eval Darwin.

# Whether to just evaluate a single chunk for quick testing
quickTest ? false,
}:
Expand Down Expand Up @@ -100,6 +101,7 @@ let
--arg systems "[ \"$system\" ]" \
--arg checkMeta ${lib.boolToString checkMeta} \
--arg includeBroken ${lib.boolToString includeBroken} \
--arg includeSubpackages ${lib.boolToString includeSubpackages} \
-I ${nixpkgs} \
-I ${attrpathFile} \
> "$outputDir/result/$myChunk"
Expand Down
1 change: 1 addition & 0 deletions pkgs/stdenv/generic/check-meta.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,7 @@ let
broken = isMarkedBroken attrs;
unsupported = hasUnsupportedPlatform attrs;
insecure = isMarkedInsecure attrs;
hydraPlatforms = if config.allowHydra then attrs.hydraPlatforms or [] else [];

available = validity.valid != "no"
&& (if config.checkMetaRecursively or false
Expand Down
18 changes: 18 additions & 0 deletions pkgs/top-level/config.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,24 @@ let
'';
};

allowHydra = mkOption {
type = types.bool;
default = true;
defaultText = literalExpression ''true'';
description = ''
Whether to enable Hydra building of all packages.
'';
};

allowHydraSubpackages = mkOption {
type = types.bool;
default = false;
defaultText = literalExpression ''false'';
description = ''
Whether to enable Hydra building of all sub-nixpkgs instances.
'';
};

allowUnfree = mkOption {
type = types.bool;
default = false;
Expand Down
8 changes: 7 additions & 1 deletion pkgs/top-level/release-attrpaths-parallel.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
myChunk,
checkMeta,
includeBroken,
includeSubpackages,
systems,
}:

Expand All @@ -18,7 +19,12 @@ let

unfiltered = import ./release-outpaths.nix {
inherit path;
inherit checkMeta includeBroken systems;
inherit
checkMeta
includeBroken
includeSubpackages
systems
;
Comment on lines 21 to 27
Copy link
Contributor

@wolfgangwalther wolfgangwalther Dec 22, 2024

Choose a reason for hiding this comment

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

I assume that release-attrpaths-parallel.nix is only run by nix/eval, while release-outpaths.nix is also run by hydra?

If so, I think we should add includeSubpackages here like includeSubpackages = true; and don't take an argument for it in this file.

Copy link
Member Author

@RossComputerGuy RossComputerGuy Dec 22, 2024

Choose a reason for hiding this comment

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

Yes but I still think it isn't a bad idea to have this so if someone runs this in their own setup, they could disable it.

};

# Turns the unfiltered recursive attribute set into one that is limited to myAttrpaths
Expand Down
5 changes: 0 additions & 5 deletions pkgs/top-level/release-attrpaths-superset.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ let
targetPackages = true;

# cross packagesets
pkgsLLVM = true;
pkgsMusl = true;
pkgsStatic = true;
pkgsCross = true;
Expand Down Expand Up @@ -186,10 +185,6 @@ let
# I am not entirely sure why these three packages end up in
# the Hydra jobset. But they do, and they don't meet the
# criteria above, so at the moment they are special-cased.
[
"pkgsLLVM"
"stdenv"
]
[
"pkgsStatic"
"stdenv"
Expand Down
4 changes: 3 additions & 1 deletion pkgs/top-level/release-outpaths.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
{
checkMeta,
includeBroken ? true, # set this to false to exclude meta.broken packages from the output
includeSubpackages ? true, # set this to false to exclude "pkgs*"
path ? ./../..,

# used by pkgs/top-level/release-attrnames-superset.nix
Expand All @@ -28,6 +29,7 @@ let
allowAliases = false;
allowBroken = includeBroken;
allowUnfree = false;
allowHydraSubpackages = includeSubpackages;
allowInsecurePredicate = x: true;
checkMeta = checkMeta;

Expand Down Expand Up @@ -84,7 +86,7 @@ let
"stdenvBootstrapTools"
"moduleSystem"
"lib-tests" # these just confuse the output
];
] ++ lib.optional (!includeSubpackages) "pkgsLLVM";

in
tweak (builtins.removeAttrs hydraJobs blacklist)
5 changes: 4 additions & 1 deletion pkgs/top-level/release.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ let
id
isDerivation
optionals
subtractLists
;

inherit (release-lib.lib.attrsets) unionOfDisjoint;
Expand Down Expand Up @@ -320,6 +321,8 @@ let
jobs = let
packagePlatforms = release-lib.recursiveMapPackages
(if attrNamesOnly then id else release-lib.getPlatforms);
packagePlatformsExclude = systems: release-lib.recursiveMapPackages
(if attrNamesOnly then id else (drv: subtractLists (release-lib.getPlatforms drv) systems));
packageJobs = packagePlatforms pkgs // {
haskell.compiler = packagePlatforms pkgs.haskell.compiler;
haskellPackages = packagePlatforms pkgs.haskellPackages;
Expand All @@ -340,7 +343,7 @@ let
idrisPackages = packagePlatforms pkgs.idrisPackages;
agdaPackages = packagePlatforms pkgs.agdaPackages;

pkgsLLVM.stdenv = [ "x86_64-linux" "aarch64-linux" ];
pkgsLLVM = packagePlatformsExclude [ "aarch64-darwin" "x86_64-darwin" ] pkgs.pkgsLLVM;
Copy link
Member

@infinisil infinisil Nov 27, 2024

Choose a reason for hiding this comment

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

Wouldn't this be super expensive on Hydra because it builds all packages for LLVM?

Copy link
Member Author

@RossComputerGuy RossComputerGuy Nov 27, 2024

Choose a reason for hiding this comment

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

It can be, my hope was LLVM to be cached on Hydra at some point. Maybe we at least do CI eval and disable building on Hydra. I'm just not sure how to do that.

Copy link
Member

@Mic92 Mic92 Dec 15, 2024

Choose a reason for hiding this comment

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

We have to first scale our build infra to before we can accept new architecture. Also evaluation in hydra is too slow, also I don't know about the latest developments with the new hydra machine.

Copy link
Member Author

@RossComputerGuy RossComputerGuy Dec 15, 2024

Choose a reason for hiding this comment

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

Yeah, that's why I'm thinking of just disabling it for Hydra. Do we have an existing way to do so?

Copy link
Contributor

@wolfgangwalther wolfgangwalther Dec 21, 2024

Choose a reason for hiding this comment

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

Maybe we at least do CI eval and disable building on Hydra. I'm just not sure how to do that.

I wish the same and also have no idea how to do it in #361529.

Copy link
Member Author

@RossComputerGuy RossComputerGuy Dec 21, 2024

Choose a reason for hiding this comment

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

Yeah, I agree. There should be like an environment attribute which describes how nixpkgs is being used. It could define whether it's in the normal setup, Hydra, or like an eval only setup.

Copy link
Member

@vcunat vcunat Dec 21, 2024

Choose a reason for hiding this comment

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

Feels like our allowUnfree and friends.

Copy link
Member Author

@RossComputerGuy RossComputerGuy Dec 21, 2024

Choose a reason for hiding this comment

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

Oh lol, I could do allowBuildingSubpackages and add that in. If it's off then it masks all pkgs* to have Hydra platforms to be empty.

Copy link
Member Author

@RossComputerGuy RossComputerGuy Dec 22, 2024

Choose a reason for hiding this comment

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

I think the behavior I just added in should do the job.

Copy link
Contributor

@wolfgangwalther wolfgangwalther Dec 22, 2024

Choose a reason for hiding this comment

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

We can see the effect here:

building '/nix/store/69swchsclvgngpjyspki23sg4wip55np-nixpkgs-eval-x86_64-linux.drv'...
System: x86_64-linux
Cores: 4
Attribute count: 180583
Chunk size: 10000
Chunk count: 19

The linux eval jobs now evaluate 180k instead of 90k packages.

I was thinking that we could evaluate those package sets as different CI jobs - but evaluation time seems to be only slightly higher than before, so maybe that's not needed and in fact more efficient.

pkgsArocc.stdenv = [ "x86_64-linux" "aarch64-linux" ];
pkgsZig.stdenv = [ "x86_64-linux" "aarch64-linux" ];
pkgsMusl.stdenv = [ "x86_64-linux" "aarch64-linux" ];
Expand Down
24 changes: 14 additions & 10 deletions pkgs/top-level/stage.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,10 @@ let
lib.optionalAttrs allowCustomOverrides
((config.packageOverrides or (super: {})) super);

nixpkgsFunSub = attrs: nixpkgsFun (attrs // {
allowHydra = config.allowHydraSubpackages;
});

# Convenience attributes for instantitating package sets. Each of
# these will instantiate a new version of allPackages. Currently the
# following package sets are provided:
Expand All @@ -192,7 +196,7 @@ let
nixpkgsFun { inherit crossSystem; })
lib.systems.examples;

pkgsLLVM = nixpkgsFun {
pkgsLLVM = if stdenv.hostPlatform.isDarwin then self else nixpkgsFunSub {
Copy link
Contributor

@wolfgangwalther wolfgangwalther Dec 22, 2024

Choose a reason for hiding this comment

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

I think it would be great, if we ultimately had an interface like this:

Suggested change
pkgsLLVM = if stdenv.hostPlatform.isDarwin then self else nixpkgsFunSub {
pkgsLLVM = nixpkgsFun {
config = { eval = true; hydra = false; };
...
}

Thus we could define those properties in exactly one location - right where we define the package set.

And then everything else should just work off of that.

Copy link
Member Author

@RossComputerGuy RossComputerGuy Dec 22, 2024

Choose a reason for hiding this comment

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

The problem is some attributes in CUDA disappears in Darwin which causes Darwin evals to fail. From a UX perspective, this looks better but practically this is a harder problem to solve.

overlays = [
(self': super': {
pkgsLLVM = super';
Expand All @@ -207,7 +211,7 @@ let
};
};

pkgsArocc = nixpkgsFun {
pkgsArocc = nixpkgsFunSub {
overlays = [
(self': super': {
pkgsArocc = super';
Expand All @@ -222,7 +226,7 @@ let
};
};

pkgsZig = nixpkgsFun {
pkgsZig = nixpkgsFunSub {
overlays = [
(self': super': {
pkgsZig = super';
Expand All @@ -240,7 +244,7 @@ let
# All packages built with the Musl libc. This will override the
# default GNU libc on Linux systems. Non-Linux systems are not
# supported. 32-bit is also not supported.
pkgsMusl = if stdenv.hostPlatform.isLinux && stdenv.buildPlatform.is64bit then nixpkgsFun {
pkgsMusl = if stdenv.hostPlatform.isLinux && stdenv.buildPlatform.is64bit then nixpkgsFunSub {
overlays = [ (self': super': {
pkgsMusl = super';
})] ++ overlays;
Expand All @@ -252,7 +256,7 @@ let

# All packages built for i686 Linux.
# Used by wine, firefox with debugging version of Flash, ...
pkgsi686Linux = if stdenv.hostPlatform.isLinux && stdenv.hostPlatform.isx86 then nixpkgsFun {
pkgsi686Linux = if stdenv.hostPlatform.isLinux && stdenv.hostPlatform.isx86 then nixpkgsFunSub {
overlays = [ (self': super': {
pkgsi686Linux = super';
})] ++ overlays;
Expand All @@ -265,7 +269,7 @@ let
} else throw "i686 Linux package set can only be used with the x86 family.";

# x86_64-darwin packages for aarch64-darwin users to use with Rosetta for incompatible packages
pkgsx86_64Darwin = if stdenv.hostPlatform.isDarwin then nixpkgsFun {
pkgsx86_64Darwin = if stdenv.hostPlatform.isDarwin then nixpkgsFunSub {
overlays = [ (self': super': {
pkgsx86_64Darwin = super';
})] ++ overlays;
Expand All @@ -282,7 +286,7 @@ let
pkgsLinux =
if stdenv.hostPlatform.isLinux
then self
else nixpkgsFun {
else nixpkgsFunSub {
localSystem = lib.systems.elaborate "${stdenv.hostPlatform.parsed.cpu.name}-linux";
};

Expand All @@ -292,7 +296,7 @@ let
appendOverlays = extraOverlays:
if extraOverlays == []
then self
else nixpkgsFun { overlays = args.overlays ++ extraOverlays; };
else nixpkgsFunSub { overlays = args.overlays ++ extraOverlays; };

# NOTE: each call to extend causes a full nixpkgs rebuild, adding ~130MB
# of allocations. DO NOT USE THIS IN NIXPKGS.
Expand All @@ -305,7 +309,7 @@ let

# Fully static packages.
# Currently uses Musl on Linux (couldn’t get static glibc to work).
pkgsStatic = nixpkgsFun ({
pkgsStatic = nixpkgsFunSub ({
overlays = [ (self': super': {
pkgsStatic = super';
})] ++ overlays;
Expand All @@ -321,7 +325,7 @@ let
};
});

pkgsExtraHardening = nixpkgsFun {
pkgsExtraHardening = nixpkgsFunSub {
overlays = [
(self': super': {
pkgsExtraHardening = super';
Expand Down