Supplement Hydra data with meta.mainProgram - #318
Open
samestep wants to merge 2 commits into
Open
Conversation
Assisted-by: Claude:opus-4.8
Assisted-by: Claude:opus-4.8
14 tasks
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/nix-index-cant-see-wrapper-packages/78059/7 |
13 tasks
This file contains hidden or 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
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.
This PR resolves #317, enabling nix-locate and downstream tools like comma to see packages that Hydra doesn't build:
I would strongly recommend reviewing the two commits of this PR separately.
The first commit rewrites the parser for
nix-env --query --xml. I did this because I found the existing parser extremely unwieldy and difficult to modify. The rewrite uses Serde via quick-xml, is significantly less code, and is roughly 4x faster than before. While the new double usage of.collectinlistings::fetchmay look a bit suspicious, the memory usage is not noticeably different from before.The second commit implements the actual feature: passing the
--metaflag to thatnix-env --querycommand so that we can extractmeta.mainProgramwhen it's available. That is then threaded through along with the store path, and in cases where Hydra doesn't have it, a synthesized/bin/$mainProgramfile listing is created. This whole thing can be turned off vianix-index --no-main-program, which simply doesn't add the--metaflag and so nometa.mainProgramentries are created in the first place.A few notes/questions I have for the reviewer(s):
Is it alright for these two commits to be put together into this one PR, or would the first commit need to be merged as a separate PR first? Or is the parser rewrite simply a no-go and I should change this PR to fit into the existing parser structure?
Should this PR modify
CHANGELOG.md?Is it fine that we say the size of the executable file is zero bytes, and give an empty string for the nar? It seems like the latter is only used for debugging (and thus would never be used in this case anyway), but I wanted to double-check. I believe the former is only displayed by nix-locate (see the example output above); it does look slightly weird, so I wonder if it'd be worth it to change
FileNode::Regular'ssizefield fromu64toOption<u64>.This PR doesn't use quick-xml in such a way that the error messages include line or column numbers. Is that fine? If not, I could make the code slightly uglier and use
Reader::error_positionto report the byte offset of the deserialization error. Or if we really need line and column specifically, I suppose we could keep track of where all the newlines are when we stream the data, and then convert the byte offset using those. I wasn't sure how much this matters, though, so I kept the code simple initially.While the rewritten parser from the first commit is about 4x faster than the previous parser, adding the
--metaflag causes the XML output to be 10x bigger, and in the end, when we feed that bigger XML into the new parser, it's about 3x slower than feeding the smaller XML into the original parser. I'm guessing this is fine since parsing this XML isn't really a bottleneck?I don't love that now
nixpkgs::query_packagesandlistings::fetcheach have two boolean flags passed positionally. But I'm also not sure it's worth it to create astructin this case.