Skip to content
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.

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

path-info: implement --filter-substitutable #7587

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nrdxp
Copy link

@nrdxp nrdxp commented Jan 11, 2023

Fixes #3946

Alternative to #7526 which implements the same logic in the new cli instead of the legacy nix-store

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or bug fix: updated release notes

@nrdxp nrdxp force-pushed the path-info-substitutes branch from b706d6c to dca6010 Compare January 11, 2023 20:55
@nrdxp nrdxp changed the title path-info: inplement --query-substitutes path-info: implement --query-substitutes Jan 11, 2023
@nrdxp nrdxp force-pushed the path-info-substitutes branch 3 times, most recently from 12e0743 to 59c7db4 Compare January 11, 2023 23:00
@nrdxp
Copy link
Author

nrdxp commented Jan 12, 2023

@roberth, it seems this implementation is fast, but it suffers from only every querying the official cache (cache.nixos.org). Trying to nail down why exactly that is has been sort of a PITA. Do you happen to have any insight into that?

Update
Scratch that, I found a fix by manually running queryValidPaths on each substituter

Comment on lines 127 to 130
substitutablePaths.find(store->followLinksToStorePath(storePathS)) ==
substitutablePaths.end()
? v["substitutable"] = false
: v["substitutable"] = true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
substitutablePaths.find(store->followLinksToStorePath(storePathS)) ==
substitutablePaths.end()
? v["substitutable"] = false
: v["substitutable"] = true;
v["substitutable"] = (bool) substitutablePaths.count(store->followLinksToStorePath(storePathS));

Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of a substitutable bool, it should return an array of substituters?

Copy link
Author

Choose a reason for hiding this comment

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

Good idea

Copy link
Author

Choose a reason for hiding this comment

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

So now that I've figured out why Store::querySubstitutablePaths wasn't working: #7587 (comment), I'm not sure how to easily do this. If I stick with defining the logic myself it should be easy though.

Attempts at using querySubstitutablePathInfos result in much slower queries which is why I moved away from that implementation. One possibility would be to add a flag to queryMissing to ignore the local store, but that would break the API.

src/nix/path-info.cc Outdated Show resolved Hide resolved
src/nix/path-info.cc Outdated Show resolved Hide resolved
src/nix/path-info.cc Outdated Show resolved Hide resolved
src/nix/path-info.cc Outdated Show resolved Hide resolved
@nrdxp nrdxp force-pushed the path-info-substitutes branch 3 times, most recently from ab201d1 to a895e82 Compare January 12, 2023 19:00
@nrdxp nrdxp force-pushed the path-info-substitutes branch from 69f6d20 to a895e82 Compare January 21, 2023 00:20
@nrdxp
Copy link
Author

nrdxp commented Jan 23, 2023

I'm starting to think that I am just gonna have to make a new sub-command for this functionality, as the BuiltPathsCommand doesn't seem like the right choice. It completely removes paths that don't exist locally from the input, which is sorta counterproductive to what we want here.

Also have some async refactor work here that I'd like to include, but that might be better off as a separate PR.

edit
So I think I'd like to do something like this in a new subcommand:
divnix/nix-uncached#1

Running all queries asynchronously is faster than waiting to query subsequent caches based on results from previous caches in my benchmarks so far. In addition, returning a json of uncached output dependencies seems like the right move so that they can be easily passed to a build command.

I've also opted to filter out derivations with preferLocalBuild since these will likely never be pulled from a cache anyway. It can save hundreds of queries in some cases (like a NixOS system closure).

@domenkozar
Copy link
Member

That's awesome work @nrdxp

src/nix/path-info.cc Outdated Show resolved Hide resolved
@@ -45,6 +46,14 @@ struct CmdPathInfo : StorePathsCommand, MixJSON
.description = "Show signatures.",
.handler = {&showSigs, true},
});

addFlag({
.longName = "query-substitutes",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be named --filter-substitutable or something like that? Since it doesn't really query what the substitutes are.

Fixes NixOS#3946

Filters out any paths from the output which are already available in
the configured substituters. Useful in CI environments, to avoid builds
that are already cached.
@nrdxp nrdxp force-pushed the path-info-substitutes branch from b68f15f to fb30259 Compare January 31, 2023 19:27
@fricklerhandwerk
Copy link
Contributor

@nrdxp please add a release note.

@nrdxp nrdxp changed the title path-info: implement --query-substitutes path-info: implement --filter-substitutable Feb 11, 2023
@tomberek
Copy link
Contributor

Oops… misclick.

@tomberek tomberek closed this Feb 11, 2023
@tomberek tomberek reopened this Feb 11, 2023
@fricklerhandwerk fricklerhandwerk added feature Feature request or proposal new-cli Relating to the "nix" command labels Mar 3, 2023
@nrdxp nrdxp marked this pull request as draft March 10, 2023 14:25
@edolstra edolstra removed their assignment Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request or proposal new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support building only drvs that lack substitutes (aka nix-build-uncached)
5 participants