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

Allow substituting paths when building remotely using ssh-ng:// #4180

Merged
merged 1 commit into from
Nov 17, 2020

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Oct 22, 2020

Until now, it was not possible to substitute missing paths from e.g.
https://cache.nixos.org on a remote server when building on it using
the new ssh-ng protocol.

This is because every store implementation except legacy ssh://
ignores the substitution flag passed to Store::queryValidPaths while
the legacy-ssh-store substitutes the remote store using
cmdQueryValidPaths when the remote store is opened with nix-store --serve.

This patch slightly modifies the daemon protocol to allow passing an
integer value suggesting whether to substitute missing paths during
wopQueryValidPaths. To implement this on the daemon-side, the
substitution logic from nix-store --serve has been moved into a
protected method named Store::substitutePaths which gets currently
called from LocalStore::queryValidPaths and Store::queryValidPaths
if maybeSubstitute is true.

Fixes #2770

cc @elaforge @edolstra @Ericson2314 @lheckemann

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Excellent change!

@Ma27 Ma27 force-pushed the ssh-ng-substitute branch from f1c0300 to db19618 Compare October 23, 2020 09:56
@Ma27
Copy link
Member Author

Ma27 commented Oct 23, 2020

@edolstra does this look OK to you as well? :)

@edolstra edolstra self-assigned this Oct 28, 2020
@Ma27
Copy link
Member Author

Ma27 commented Nov 3, 2020

ping @edolstra anything missing to get this merged? %)

@edolstra
Copy link
Member

edolstra commented Nov 4, 2020

maybeSubstitute is a pretty ugly hack and it would probably be better to get rid of it somehow rather than add it to other stores. For instance, nix copy could call destStore->substitutePaths() (or destStore->buildPaths()) directly.

@Ma27
Copy link
Member Author

Ma27 commented Nov 4, 2020

@edolstra so you mean that the daemon (and store) workers should invoke destStore->substitutePaths() directly if needed?

Until now, it was not possible to substitute missing paths from e.g.
`https://cache.nixos.org` on a remote server when building on it using
the new `ssh-ng` protocol.

This is because every store implementation except legacy `ssh://`
ignores the substitution flag passed to `Store::queryValidPaths` while
the `legacy-ssh-store` substitutes the remote store using
`cmdQueryValidPaths` when the remote store is opened with `nix-store
--serve`.

This patch slightly modifies the daemon protocol to allow passing an
integer value suggesting whether to substitute missing paths during
`wopQueryValidPaths`. To implement this on the daemon-side, the
substitution logic from `nix-store --serve` has been moved into a
protected method named `Store::substitutePaths` which gets currently
called from `LocalStore::queryValidPaths` and `Store::queryValidPaths`
if `maybeSubstitute` is `true`.

Fixes NixOS#2770
@Ma27 Ma27 force-pushed the ssh-ng-substitute branch from db19618 to 3a63fc6 Compare November 5, 2020 19:31
@Ma27
Copy link
Member Author

Ma27 commented Nov 5, 2020

@edolstra the Store::substitutePaths() method is now called from nix-daemon directly. Anything else to fix to get this merged? :)

@Ma27
Copy link
Member Author

Ma27 commented Nov 8, 2020

ping @edolstra anything else tbd here to get this merged? :)

@Ma27
Copy link
Member Author

Ma27 commented Nov 15, 2020

@edolstra any updates here? :)

@edolstra edolstra merged commit df5c69a into NixOS:master Nov 17, 2020
@edolstra
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

builders-use-substitutes only implemented for LegacySSHStore?
4 participants