rhendric
commented
3 months ago Did you born yesterday?
@AndersonTorres, this is rude. OP may be misguided but that isn't license to take this tone.
Open lolbinarycat opened 3 months ago
rhendric
commented
3 months ago Did you born yesterday?
@AndersonTorres, this is rude. OP may be misguided but that isn't license to take this tone.
ghost
commented
3 months ago `repository` would be the first to pull directly from a complex attribute not already part of `meta`, which is a threshold worth pausing at, because of precisely the sort of problem we've just witnessed.
@rhendric what is the problem we just witnessed? we made a mistake fixed it and have a better implementation because of it that is immune to the previous failure. requiring a perfect first implementation is not a good faith argument.
lolbinarycat
commented
3 months ago More like no longer needs to be tested via CI, because for most packages there'll be nothing to test.
most packages. the entire point of testing is to catch edge cases.
As soon as ‘everyone’ is more than one nice-to-have link in the search app, I'll entertain this objection.
well guess what, i have immediate plans for other stuff i would like to do with it so... yeah it's more that nixos-search
As already noted, addressing eval errors outside of Nix is easier than inside, and consumers can always filter the packages down to a set they're interested in.
as already noted, doing so results in massive performance penalties.
You're proposing changing what src attributes are required to evaluate under what conditions just so that you can put logic intended to populate a search result field in Nixpkgs instead of in the search app—this is clearly a case of the tail wagging the dog.
this is simply misrepresenting my intent. i already said there are other usecases. i would really appreciate if people would stop telling me what i want.
rhendric
commented
3 months ago @a-n-n-a-l-e-e, I'm not requiring a perfect first implementation.
The issue is that the scope increased from ‘add a field to meta’ to ‘add a field to meta and also create a new guarantee for src to evaluate successfully in more contexts’, and that second bit is harder and more error-prone. If we agree on the value of undertaking both parts, then making a mistake in the first attempt of the second part isn't a big deal—fix it and move on, as you say. But the mistake is evidence that the second part is harder, and that even if it looks like we get it right now, problems may arise again in the future (and that comment in the PR is evidence that we're expecting those problems—and that resolution mechanism (find the comment based on stack trace breadcrumbs) is not great!). And given that the second part is harder, I assert that it isn't worth it to attempt and we should take the scope of the work back to just the first part.
@lolbinarycat
most packages. the entire point of testing is to catch edge cases.
But you don't have to test what you don't implement! I'm really confused by our disagreement here. You want to change src and then test that the change is good; I want to refrain from changing src and then there's nothing to do or test.
well guess what, i have immediate plans for other stuff i would like to do with it so... yeah it's more that nixos-search
How about discussing those plans then? That might convince me that implementing the feature the way you want is worth doing even if it involves changing the guarantees around src.
as already noted, doing so results in massive performance penalties.
And that's much less of a big deal if it doesn't happen in the heart of Nixpkgs for everyone's evaluation.
this is simply misrepresenting my intent. i already said there are other usecases. i would really appreciate if people would stop telling me what i want.
Sorry you're frustrated, but I'm going off of what you've said in this thread so far, and I think the above comment is the first time you're mentioning any other use cases in a non-hypothetical way.
lolbinarycat
commented
3 months ago But you don't have to test what you don't implement! I'm really confused by our disagreement here. You want to change src and then test that the change is good; I want to refrain from changing src and then there's nothing to do or test.
i thought you wanted to not implement it in nixpkgs.
guess what, if you move the code somewhere else, it still exists!
lolbinarycat
commented
3 months ago github sucks on mobile
rhendric
commented
3 months ago i thought you wanted to not implement it in nixpkgs.
guess what, if you move the code somewhere else, it still exists!
This isn't the gotcha that you think it is, so the attempt at a dunk isn't exactly going to be convincing.
What you would be testing if this logic were in Nixpkgs is not the correctness of the logic itself. The logic is pretty dang simple—use meta.repository if it exists, and use src.meta.repository if it doesn't, and adjust for lists appropriately. That's a function that could be unit tested very easily, but it's not what we're discussing, I think. It seems to me that when you say you want the correctness tested in Nixpkgs, what you want to ensure is that every derivation in Nixpkgs has a src that can be accessed in this way, across all platforms and under any other weird conditions. That's hard to test and hard to guarantee going forward.
I'm proposing not making that change to the expectations around src. Implement the trivial function elsewhere. If you plan on using it in multiple places, stick it in a library. Maybe it even deserves to live in lib. So yes, that code still exists! And you can test it if you want! What won't be tested, because it won't change, is that src can be accessed in all the contexts that you might want meta. If consumers try to access src in those contexts, they can fail and recover from the failure in whatever way is appropriate for them, because that's not Nixpkgs's concern.
AndersonTorres
commented
3 months ago Did you born yesterday?
@AndersonTorres, this is rude. OP may be misguided but that isn't license to take this tone.
Not my intention, just an expression of surprise.
For someone throwing arguments like "a lot of this was made before" without backing them up, it is hilarious that the most obvious example is now requiring evidence!
AndersonTorres
commented
3 months ago requiring a perfect first implementation is not a good faith argument
the first one broke - twice, if you can't remember, Ghostie...
lolbinarycat
commented
3 months ago That's hard to test
it's really not, it's basically just nesting two for-each loops.
in my experience it isn't difficult to make src eval either.
hard to guarantee going forward.
rhendric
commented
3 months ago That's hard to test
it's really not, it's basically just nesting two for-each loops.
That'll test that it works in the specific configuration that it's tested in. But a given src could depend on any configuration value, not just system, and you aren't going to test every possible permutation in CI.
hard to guarantee going forward.
- nixpkgs doesn't guarantee stability outside of stable releases
- i've only ever seen one pattern that breaks this expectation: indexing src based on the value of the current system
Yeah, so this really comes down to a risk appetite thing. If you're a move-fast-break-things person, you might see the risk as worth it. You're looking at the things you see and saying they don't look so bad. I'm looking at the spaces where there can be things I don't see and saying why risk it? I don't think there have been enough eyeballs on this for me to feel comfortable with work that has a good chance of slipping time bombs into Hydra, which take precious time and effort to resolve. Particularly when consumers who want this logic can shoulder the burden of evaluating it themselves. Why not start there and propose moving things into Nixpkgs after you've proved the value of the work?
ghost
commented
3 months ago @a-n-n-a-l-e-e, I'm not requiring a perfect first implementation.
The issue is that the scope increased from ‘add a field to
meta’ to ‘add a field tometaand also create a new guarantee forsrcto evaluate successfully in more contexts’, and that second bit is harder and more error-prone.
so we are not trying to guarantee that src evaluates correctly. the new implementation doesn't require src to evaluate meta attributes.
lolbinarycat
commented
3 months ago That'll test that it works in the specific configuration that it's tested in. But a given src could depend on any configuration value, not just system, and you aren't going to test every possible permutation in CI.
theoretically, but i don't think they're likely to in practice.
anyways, by that argument, some combination of config options could cause nixpkgs as a whole to stop evaluating, since we're not testing every permutation.
rhendric
commented
3 months ago @a-n-n-a-l-e-e
so we are not trying to guarantee that src evaluates correctly. the new implementation doesn't require src to evaluate meta attributes.
The new implementation being #300313? Which contains this code?
# 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.
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 [];That's the code that doesn't require src? The code that fails ‘if src fails to eval’? Can you clarify?
@lolbinarycat
anyways, by that argument, some combination of config options could cause nixpkgs as a whole to stop evaluating, since we're not testing every permutation.
Yes, some combination of config options could do that. I don't know what point you're trying to make with that statement.
ghost
commented
3 months ago @a-n-n-a-l-e-e
so we are not trying to guarantee that src evaluates correctly. the new implementation doesn't require src to evaluate meta attributes.
The new implementation being #300313? Which contains this code?
# 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. 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 [];That's the code that doesn't require
src? The code that fails ‘if src fails to eval’? Can you clarify?
right. src not evaluating will not prevent access to other fields in meta, which was the issue before. trying to evaluate repository will still be problematic but i'm not sure it's worth adding additional checks to repository to prevent errors related to src. it's possible, checking that sourceProvence isSource, but perhaps not necessary. my point is that adding the repository field will not break current usage.
AndersonTorres
commented
3 months ago it's possible, checking that sourceProvence isSource
this is another assumption that can be broken easily.
sourceProvenance == fromSource does not imply it "evaluates" in all platforms. What about assembly?
sourceProvenance == binaryNativeCode does not imply it does not "evaluate" in all platforms. What about object files for Windows that are used in forensics and pentesting?
Indeed we can argue font glyphs are exactly like that.
(currently I am more inclined to an "outside nix" approach (scripting, manual fixup, treewide, small steps), in case someone asks me.)
lolbinarycat
commented
3 months ago What about assembly?
unless you are using import-from-derivation (which is banned in nixpkgs), assembly will never be interpreted at eval time, only at build time.
Indeed we can argue font glyphs are exactly like that.
font glyphs are binary data, not binary code. it could be argued that truetype fonts contain vm bytecode, but they certainly do not contain native code.
ghost
commented
3 months ago so i wanted to see how many aborts are generated from trying pkg.meta.repository over all packages with the hydra systems x64/aarch64 linux & darwin. so hacked the script below which runs the check meta eval. Running the script i found 19 files that needed to be modified to throw. (probably better way to do this but here is a hacked 1st attempt)
[edit] opened PRs to change aborts to throw (and fix a typo)
AndersonTorres
commented
3 months ago What about assembly?
unless you are using import-from-derivation (which is banned in nixpkgs), assembly will never be interpreted at eval time, only at build time.
Irrelevant. There is nothing hindering someone to write code like "throw when host is not x86" when facing a fromSource.
font glyphs are binary data, not binary code.
Data-code distinction lose most of its explanatory power since before the first worm, passing through undergrounds like Forth, Lisp and Phrack, and nowadays with the dreadful xzdoor.
Further it misses the point I tried to convey: a non-fromSource can evaluate despite not being fromSource.
Nonetheless my catastrophic example does not affect the argument: neither fromSource implies src will eval, nor non-fromSource implies src will break.
lolbinarycat
commented
3 months ago There is nothing hindering someone to write code like "throw when host is not x86"
a throw error would be fine, actually. it would be caught by a tryEval in release-attrpaths-superset
calling abort would not be fine, and would probably cause CI to fail.
the only exceptions i know are calling abort within src on an unsupported platform, or in a transient derivation which is not exposed to hydra.
so... don't do that?
rhendric
commented
2 months ago right.
srcnot evaluating will not prevent access to other fields in meta, which was the issue before. trying to evaluaterepositorywill still be problematic but i'm not sure it's worth adding additional checks to repository to prevent errors related to src. it's possible, checking that sourceProvence isSource, but perhaps not necessary. my point is that adding the repository field will not break current usage.
You don't know that. Current usage can include toJSONing the meta attributes of every package in the list. This currently works—I just confirmed it—and would break with your proposed change.
I'm fine with #300286 but I think any PR that makes meta depend on src, even in the way you're currently endorsing, should be reviewed by more of the community before it is merged.
nixos-discourse
commented
2 months ago This issue 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
lolbinarycat
commented
2 months ago Current usage can include toJSONing the meta attributes of every package in the list
if that is a usage we want to support then it should have a CI test.
Problem
most packages use
meta.homepageto link to the source code repository, but for projects with an actual homepage (such as luajit), finding the source code often requires clicking around various links to try to find the official repo.for packages that use
fetchFromGitHubor similar, you can usually piece the url back together with some effort, but for projects that use a source tarball (again, like luajit), you're out of luck.Proposal
a
meta.repositoryfield that points to an http-browsable source tree.for packages without a separate homepage, you could just set the
meta.repositoryfield instead.Checklist
Add a :+1: reaction to issues you find important.