stdenv: re-add meta.repository (resurrected) #301947
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
8.has: documentation
ofborg
bot
added
10.rebuild-darwin: 0
ghost
approved these changes
Apr 6, 2024
AndersonTorres
approved these changes
Apr 6, 2024
rhendric
previously requested changes
Apr 6, 2024
There was a problem hiding this comment.
As with its predecessor, I think the design of this (not any particular line of code in this PR, but the dependency of meta on src) needs to be discussed more before this PR is merged. I'll withdraw my objection if it is discussed and I'm outvoted or convinced otherwise, but there's no need for haste here. We should take the time to hash it out in #293838.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/when-should-meta-attributes-be-computed/42833/1 |
wegank
added
the
12.approvals: 2
pkgs/stdenv/generic/check-meta.nix
Outdated
| @@ -454,6 +464,15 @@ let | |||
| hasOutput = out: builtins.elem out outputs; | |||
| in | |||
| { | |||
| # get the default value for the meta.repository field. | |||
| # the fetchFrom* fetchers set src.meta.homepage | |||
| # NOTE: this will fail if src fails to eval, in that case either set meta.repository manually to prevent this default from being evaluated, or just make sure src doesn't fail to eval. | |||
There was a problem hiding this comment.
Have you tried builtins.tryEval to catch the src evaluation errors and just put nothing in it?
There was a problem hiding this comment.
builtins.tryEval only catches throw errors. this note was orignally made when packages enconering abort-class errors in src was fairly common (at the time this was causing eval to fail), although both of these issues have been fixed since then.
There was a problem hiding this comment.
abort-class errors in evaluations are abnormal. Throwing in derivations is the only thing allowed to make evaluation fail.
Doing src = { "x86_64-linux" = ...; }.${builtins.currentSystem} is a mistake in nixpkgs, it requires at least:
src = { ... }.${builtins.currentSystem} or (throw "Source not available on {...}")
Thus, tryEval is the only thing you really really need in checkMeta for infaillible evaluations, all the other errors are bugs in nixpkgs and must be fixed as such because Hydra won't catch them neither.
wegank
added
12.approvals: 1
wrapping in median of 11 runs (only really matters for cpu time, everything else is stable i think). using change 253c0db and x86_64-linux check meta tests baseline is change 253c0db, prlist11 is with this PR, prlist15 and prlist16 use tryEval
then running with NIX_SHOW_STATS=1 NIX_SHOW_STATS_PATH=results/baseline-stats$i.json nix eval -f - --show-trace <<EOF
with import ./.{}; ((import ./out-paths.nix) pkgs)
EOF
prlist11.diffdiff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix
index 1cd1ae6dd72e..f1f486b115a8 100644
--- a/pkgs/stdenv/generic/check-meta.nix
+++ b/pkgs/stdenv/generic/check-meta.nix
@@ -10,7 +10,9 @@ let
concatMapStrings
concatMapStringsSep
concatStrings
+ filter
findFirst
+ head
isDerivation
length
concatMap
@@ -39,6 +41,11 @@ let
toPretty
;
+ unlist = list:
+ if length list == 1
+ then head list
+ else list;
+
# If we're in hydra, we can dispense with the more verbose error
# messages and make problems easier to spot.
inHydra = config.inHydra or false;
@@ -303,6 +310,10 @@ let
(listOf str)
str
];
+ repository = union [
+ (listOf str)
+ str
+ ];
downloadPage = str;
changelog = union [
(listOf str)
@@ -452,8 +463,14 @@ let
let
outputs = attrs.outputs or [ "out" ];
hasOutput = out: builtins.elem out outputs;
- in
- {
+ in {
+ repository =
+ if attrs ? src.meta.homepage
+ then attrs.src.meta.homepage
+ else if attrs ? srcs && isList attrs.srcs
+ then unlist (map (src: src.meta.homepage) (filter (src: src ? meta.homepage) attrs.srcs))
+ else [];
+
# `name` derivation attribute includes cross-compilation cruft,
# is under assert, and is sanitized.
# Let's have a clean always accessible version here.prlist15.diffdiff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix
index bcb2ca249ddf..18764e52a319 100644
--- a/pkgs/stdenv/generic/check-meta.nix
+++ b/pkgs/stdenv/generic/check-meta.nix
@@ -10,7 +10,9 @@ let
concatMapStrings
concatMapStringsSep
concatStrings
+ filter
findFirst
+ head
isDerivation
length
concatMap
@@ -39,6 +41,11 @@ let
toPretty
;
+ unlist = list:
+ if length list == 1
+ then head list
+ else list;
+
# If we're in hydra, we can dispense with the more verbose error
# messages and make problems easier to spot.
inHydra = config.inHydra or false;
@@ -303,6 +310,10 @@ let
(listOf str)
str
];
+ repository = union [
+ (listOf str)
+ str
+ ];
downloadPage = str;
changelog = union [
(listOf str)
@@ -454,8 +465,15 @@ let
let
outputs = attrs.outputs or [ "out" ];
hasOutput = out: builtins.elem out outputs;
- in
- {
+ in {
+ repository = let res = builtins.tryEval (
+ if attrs ? src.meta.homepage
+ then attrs.src.meta.homepage
+ else if attrs ? srcs && isList attrs.srcs
+ then unlist (map (src: src.meta.homepage) (filter (src: src ? meta.homepage) attrs.srcs))
+ else []);
+ in if res.success then res.value else [];
+
# `name` derivation attribute includes cross-compilation cruft,
# is under assert, and is sanitized.
# Let's have a clean always accessible version here.prlist16.diffdiff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix
index bcb2ca249ddf..a31c02ec225c 100644
--- a/pkgs/stdenv/generic/check-meta.nix
+++ b/pkgs/stdenv/generic/check-meta.nix
@@ -10,7 +10,9 @@ let
concatMapStrings
concatMapStringsSep
concatStrings
+ filter
findFirst
+ head
isDerivation
length
concatMap
@@ -39,6 +41,16 @@ let
toPretty
;
+ unlist = list:
+ if length list == 1
+ then head list
+ else list;
+
+ valOrEmpty = res:
+ if res.success
+ then res.value
+ else [];
+
# If we're in hydra, we can dispense with the more verbose error
# messages and make problems easier to spot.
inHydra = config.inHydra or false;
@@ -303,6 +315,10 @@ let
(listOf str)
str
];
+ repository = union [
+ (listOf str)
+ str
+ ];
downloadPage = str;
changelog = union [
(listOf str)
@@ -454,8 +470,14 @@ let
let
outputs = attrs.outputs or [ "out" ];
hasOutput = out: builtins.elem out outputs;
- in
- {
+ in {
+ repository = valOrEmpty (builtins.tryEval (
+ if attrs ? src.meta.homepage
+ then attrs.src.meta.homepage
+ else if attrs ? srcs && isList attrs.srcs
+ then unlist (map (src: src.meta.homepage) (filter (src: src ? meta.homepage) attrs.srcs))
+ else []));
+
# `name` derivation attribute includes cross-compilation cruft,
# is under assert, and is sanitized.
# Let's have a clean always accessible version here. |
lolbinarycat
force-pushed
the
meta.repository-v2
branch
from
a95b4fb to
16139a4
Compare
|
added |
wegank
removed
the
12.approvals: 1
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
rhendric
dismissed
their stale review
I still don't endorse this design but Raito's approval is sufficient for me to get out of the way.
ghost
approved these changes
Apr 10, 2024
wegank
added
12.approvals: 2
13 tasks
wegank
removed
the
12.approvals: 1
wegank
added
the
12.approvals: 2
FliegendeWurst
approved these changes
Dec 13, 2024
wegank
added
12.approvals: 3+
3 tasks
wegank
added
the
2.status: stale
stale
bot
removed
the
2.status: stale
nix-owners
bot
requested review from
philiptaron,
emilazy,
reckenrode,
Artturin and
RossComputerGuy
github-actions
bot
added
10.rebuild-darwin: 1-10
10.rebuild-linux: 1-10
and removed
10.rebuild-darwin: 0
wegank
removed
the
12.approvals: 3+
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
previous PR (#300313) got messed up due to me trying to do too many fancy git things at once (mainly using shallow pulls and amending commits)
i've updated the documentation to match the new behavior, and have added a different example
@a-n-n-a-l-e-e @AndersonTorres @nyabinary for participating in the previous PR
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.