stdenv: add meta.repository (fixed) #300313
Conversation
|
@AndersonTorres @a-n-n-a-l-e-e because of their role in the previous PR. |
|
the fix to serapi seems to be getting deleted in the merge, unsure how to fix. |
lolbinarycat
force-pushed
the
meta.repository
branch
from
24e63c8 to
c28b2f2
Compare
ofborg
bot
added
the
2.status: merge conflict
|
i think this should work |
ofborg
bot
removed
the
2.status: merge conflict
pkgs/stdenv/generic/check-meta.nix
Outdated
| list; | ||
| in | ||
| unlist (getHomePages (getSrcs attrs)); | ||
| } // { |
There was a problem hiding this comment.
Since the user is somewhat free to set meta.repository, the use of // can bring unexpected outcomes.
https://nix.dev/guides/best-practices.html#updating-nested-attribute-sets
It's better to run this logic inside the block (below name = . . .; below), somewhat like
repository =
if (complicatedConditions) then
complicatedFunction
else
[ ];There was a problem hiding this comment.
Since the user is somewhat free to set meta.repository, the use of // can bring unexpected outcomes.
what unexpected outcomes?
manually setting repository will override the autodetected value, but that is intended behavior.
that link specifically warns of the fact that // performs a shallow update, but a shallow update is what is desired here.
It's better to run this logic inside the block (below name = . . .; below), somewhat like
if you want the field to default to unset (to match the behavior of the other fields), that is impossible, since nix has no equivalent to javascript's undefined.
There was a problem hiding this comment.
if you want the field to default to unset
no. This is only strange.
But the surrounding code is no less strange.
There was a problem hiding this comment.
if you have a problem with defaulting to unset, take it up with the original authors of check-meta.nix or open a github issue, it wasn't my choice.
There was a problem hiding this comment.
I never said a thing about "default to unset".
There was a problem hiding this comment.
also a downside of not having implicit null
There was a problem hiding this comment.
eliminating the optionalAttrs also eliminates the eval performance hit. #300177 (comment)
There was a problem hiding this comment.
hmm... i guess we could just make it default to the empty list and be a bit different. i believe nixos search treats unset as an empty list anyways.
There was a problem hiding this comment.
additionally, not using // means the conditions are only evaluated if meta.repository is not already set, which means any packages that have a src that doesn't eval can just set repository manually.
|
something is throwing an error and i don't know what. if anyone can tell me what package is failing to eval, or why ofborg-eval dosn't show any error logs, that would be greatly appreciated. |
|
if this doesn't work, then i'll try fixing eval errors at the package end instead of the check-meta end. this could actually be done by manually adding |
ofborg
bot
added
10.rebuild-darwin: 0
|
results from various permutations of the change. diffs below.
using 127c068 prlist.diffdiff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix
index 1cd1ae6dd72e..c092063a03e8 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
@@ -303,6 +305,10 @@ let
(listOf str)
str
];
+ repository = union [
+ (listOf str)
+ str
+ ];
downloadPage = str;
changelog = union [
(listOf str)
@@ -440,6 +446,22 @@ let
# -----
else { valid = "yes"; });
+ # get the default value for the meta.repository field.
+ # the fetchFrom* fetchers set src.meta.homepage
+ getRepository = let
+ getSrcs = attrs:
+ filter (src: src ? meta.homepage)
+ (if attrs ? src
+ then [ attrs.src ]
+ else optionals (attrs ? srcs && isList attrs.srcs) attrs.srcs);
+ getHomePages = map (src: src.meta.homepage);
+ unlist = list:
+ # the following line should show up in the stack trace is src doesn't eval
+ # HINT: try adding meta.repository to your package if it fails to eval
+ if length list == 1
+ then head list
+ else list;
+ in attrs: unlist (getHomePages (getSrcs attrs));
# The meta attribute is passed in the resulting attribute set,
# but it's not part of the actual derivation, i.e., it's not
@@ -452,8 +474,9 @@ let
let
outputs = attrs.outputs or [ "out" ];
hasOutput = out: builtins.elem out outputs;
- in
- {
+ in {
+ repository = getRepository attrs;
+
# `name` derivation attribute includes cross-compilation cruft,
# is under assert, and is sanitized.
# Let's have a clean always accessible version here.prlist2.diffdiff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix
index 1cd1ae6dd72e..a28f0c342ecd 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
@@ -303,6 +305,10 @@ let
(listOf str)
str
];
+ repository = union [
+ (listOf str)
+ str
+ ];
downloadPage = str;
changelog = union [
(listOf str)
@@ -440,6 +446,20 @@ let
# -----
else { valid = "yes"; });
+ # get the default value for the meta.repository field.
+ # the fetchFrom* fetchers set src.meta.homepage
+ getRepository = let
+ getSrcs = attrs:
+ if attrs ? src
+ then
+ [ attrs.src ]
+ else
+ filter (src: src ? meta.homepage) attrs.srcs;
+ getHomePages = map (src: src.meta.homepage);
+ unlist = list:
+ if length list == 1 then head list
+ else list;
+ in attrs: unlist (getHomePages (getSrcs attrs));
# The meta attribute is passed in the resulting attribute set,
# but it's not part of the actual derivation, i.e., it's not
@@ -452,8 +472,9 @@ let
let
outputs = attrs.outputs or [ "out" ];
hasOutput = out: builtins.elem out outputs;
- in
- {
+ in {
+ repository = optionals (attrs ? src.meta.homepage || attrs ? srcs && isList attrs.srcs && any (src: src ? meta.homepage) attrs.srcs) (getRepository attrs);
+
# `name` derivation attribute includes cross-compilation cruft,
# is under assert, and is sanitized.
# Let's have a clean always accessible version here.prlist3.diffdiff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix
index 1cd1ae6dd72e..4c1f6ce8e81f 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
@@ -303,6 +305,10 @@ let
(listOf str)
str
];
+ repository = union [
+ (listOf str)
+ str
+ ];
downloadPage = str;
changelog = union [
(listOf str)
@@ -452,8 +457,17 @@ let
let
outputs = attrs.outputs or [ "out" ];
hasOutput = out: builtins.elem out outputs;
- in
- {
+ in {
+ repository = let
+ srcs =
+ if attrs ? src then [ attrs.src ]
+ else filter (src: src ? meta.homepage) attrs.srcs;
+ unlist = list:
+ if length list == 1 then head list
+ else list;
+ hasRep = attrs ? src.meta.homepage || attrs ? srcs && isList attrs.srcs && any (src: src ? meta.homepage) attrs.srcs;
+ in optionals hasRep (unlist (map (src: src.meta.homepage) srcs));
+
# `name` derivation attribute includes cross-compilation cruft,
# is under assert, and is sanitized.
# Let's have a clean always accessible version here.prlist4.diffdiff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix
index 1cd1ae6dd72e..7895b1b2f5b9 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
@@ -303,6 +305,10 @@ let
(listOf str)
str
];
+ repository = union [
+ (listOf str)
+ str
+ ];
downloadPage = str;
changelog = union [
(listOf str)
@@ -452,8 +457,16 @@ let
let
outputs = attrs.outputs or [ "out" ];
hasOutput = out: builtins.elem out outputs;
- in
- {
+ in {
+ repository = let
+ srcs =
+ if attrs ? src then [ attrs.src ]
+ else filter (src: src ? meta.homepage) attrs.srcs;
+ unlist = list:
+ if length list == 1 then head list
+ else list;
+ in optionals (attrs ? src.meta.homepage || attrs ? srcs && isList attrs.srcs && any (src: src ? meta.homepage) attrs.srcs) (unlist (map (src: src.meta.homepage) srcs));
+
# `name` derivation attribute includes cross-compilation cruft,
# is under assert, and is sanitized.
# Let's have a clean always accessible version here.prlist6.diffdiff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix
index 1cd1ae6dd72e..2f55bb36e67b 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
@@ -303,6 +305,10 @@ let
(listOf str)
str
];
+ repository = union [
+ (listOf str)
+ str
+ ];
downloadPage = str;
changelog = union [
(listOf str)
@@ -452,8 +458,20 @@ let
let
outputs = attrs.outputs or [ "out" ];
hasOutput = out: builtins.elem out outputs;
- in
- {
+ in {
+ repository = let
+ srcs =
+ if attrs ? src.meta.homepage
+ then [ attrs.src ]
+ else if attrs ? srcs && isList attrs.srcs
+ then filter (src: src ? meta.homepage) attrs.srcs
+ else [];
+ unlist = list:
+ if length list == 1
+ then head list
+ else list;
+ in unlist (map (src: src.meta.homepage) srcs);
+
# `name` derivation attribute includes cross-compilation cruft,
# is under assert, and is sanitized.
# Let's have a clean always accessible version here.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.table generated using duckdb 0.10.1 gentable.sqlwith
data as (
select
-- json files named as {name}-stats[0-9]+.json in current directory
-- and name will be used as the column name and median value chosen
regexp_replace(filename, '-stats[0-9]+.json$', '') as run,
cpuTime,
nrAvoided,
nrFunctionCalls,
nrLookups,
nrOpUpdateValuesCopied,
nrOpUpdates,
nrPrimOpCalls,
nrThunks,
envs.bytes as "envs.bytes",
envs.elements as "envs.elements",
envs.number as "envs.number",
gc.heapSize as "gc.heapSize",
gc.totalBytes as "gc.totalBytes",
list.bytes as "list.bytes",
list.concats as "list.concats",
list.elements as "list.elements",
sets.bytes as "sets.bytes",
sets.elements as "sets.elements",
sets.number as "sets.number",
sizes.Attr as "sizes.Attr",
sizes.Bindings as "sizes.Bindings",
sizes.Env as "sizes.Env",
sizes.Value as "sizes.Value",
symbols.bytes as "symbols.bytes",
symbols.number as "symbols.number",
values.bytes as "values.bytes",
values.number as "values.number",
from
read_json('*.json', filename=true)
),
med as (
select
run,
median(columns(* exclude run)) as "\0"
from
data
group by
run
),
res as (
select
run,
name,
value
from (unpivot med on (* exclude run))
)
select
name,
round(baseline, 2) as baseline,
round(100 * (columns(* exclude (name, baseline))-baseline)/baseline,2) as "\0 %"
-- only one value, so last doesn't matter but pivot requires an aggregate
from (pivot res on run using last(value) order by name); |
ghost
linked an issue
|
This needs to be squashed a bit btw. |
|
i will squash once i'm sure everything is working properly. |
|
@a-n-n-a-l-e-e updated |
|
can you remove the 2 files from the PR -- now the files just have comments which seem outdated. this PR should have 1 file, check-meta.nix, and then squash the commits so we can move forward. thx. |
|
what do you mean by "remove the files from the pr"? will squashing do that, or do i need to do something else? |
ofborg
bot
added
2.status: merge conflict
lolbinarycat
force-pushed
the
meta.repository
branch
from
1aed3c4 to
de77733
Compare
ofborg
bot
added
2.status: merge conflict
lolbinarycat
force-pushed
the
meta.repository
branch
from
de37003 to
57534a2
Compare
ofborg
bot
added
the
2.status: merge conflict
There was a problem hiding this comment.
Please let's get some reviewers on this who didn't work on the previous iteration before merging, to make sure this is a consensus-driven change.
I've been expressing skepticism about this approach in the parent issue and while I don't mind being overruled by consensus, right now it seems like the same people who cowboyed through the first PR in a very short time are pushing forward again without changing much about their approach. I'm not trying to criticize them for their enthusiasm but I'd feel better if some more experienced voices were involved.
21 days of active discussion.
it has been completely rewritten and the semantics have been changed.
if anyone with relevant experience with stdenv is available in a timely manner, then sure, but i'm not sure that's the case. |
<sigh> @AndersonTorres proposed splitting the PR into two parts along the basis of whether
The approach of deriving this field from
This isn't how consensus-driven projects should work. Cooperate and build consensus; don't just rush through what you can get away with because you have more free time than the older hands. |
the current PR is an amalgamation of code written by at least three different people, using feedback provided by several others. if that's not cooperation, i don't know what is. additionally, i'm not sure how to cooperate with someone when their only feedback is "don't do this". |
the commit still needs to get squashed. if you're struggling with git one thing i do is create a test branch to experiment on so if i run the wrong git commands i can just toss the branch and start over. eg: |
|
@a-n-n-a-l-e-e whenever i would squash it would just create a merge conflict, which i would fix on github, but that would just create another commit. i have 3 backup presquash branches by now my local nixpkgs clone is extremely messy due to using things like shallow pulls in order to cope with my extremely slow internet. |
ghost
github-actions
bot
added
the
8.has: documentation
ok. i fixed it then. you'll need to pull before doing any changes. |
ofborg
bot
removed
the
2.status: merge conflict
pkgs/stdenv/generic/check-meta.nix
Outdated
| @@ -452,8 +462,19 @@ let | |||
| let | |||
| outputs = attrs.outputs or [ "out" ]; | |||
| hasOutput = out: builtins.elem out outputs; | |||
| in | |||
| { | |||
| in { | |||
There was a problem hiding this comment.
Not sure, but afaicr format rules RFC requires in in a separate line.
doc/stdenv/meta.chapter.md
Outdated
| @@ -47,6 +47,10 @@ Release branch. Used to specify that a package is not going to receive updates t | |||
|
|
|||
| The package’s homepage. Example: `https://www.gnu.org/software/hello/manual/` | |||
|
|
|||
| ### `repository` {#var-meta-repository} | |||
|
|
|||
| A webpage where the package's source code can be viewed. `https` links are preferred if available. Automatically set to a default value if the package uses a `fetchFrom*` fetcher for its `src`. Example: `https://github.com/forthy42/gforth` | |||
There was a problem hiding this comment.
| A webpage where the package's source code can be viewed. `https` links are preferred if available. Automatically set to a default value if the package uses a `fetchFrom*` fetcher for its `src`. Example: `https://github.com/forthy42/gforth` | |
| A webpage (or a list of webpages) where the package's source code can be viewed. `https` links are preferred if available. Automatically set to a default value if the package uses a `fetchFrom*` fetcher for its `src`. Example: `https://github.com/forthy42/gforth` |
P.S.: Again, this is not the official upstream source for Gforth.
And, since the xzdoor incident (that happened on its github mirror), we should not promote mirrors.
|
@rhendric you have requested changes but have not commented on any part of the PR. This is a low stakes change that has minimal impact on eval performance, #300313 (comment), and does not impact the building of nix packages. additionally, backing out the change, if required, does not require a staging cycle or any rebuilds, so your behavior seems out of place given impact of this change. Please reconsider to help move forward and deescalate this situation or consider stepping down from your role as a committer if you are unable to do so. |
lolbinarycat
force-pushed
the
meta.repository
branch
from
cb6da39 to
0c01466
Compare
|
so, turns out shallow fetches kinda break everything. will manually reconstruct the broken branches on top of current master and create a new PR. |
|
@a-n-n-a-l-e-e 1. any committer can dismiss «changes requested» status with a comment, so you might be overstating the level of blocking, and 2. if a change has broken eval and was reverted, its updated version is not low-stakes. |
Description of changes
packages assume
srcwon't be evaluated on unsupported platforms, so we match that assumption.hopefully this doesn't break anything else.
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.