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

fix: nix copy ssh-ng:// not respecting --substitute-on-destination #9600

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

SharzyL
Copy link
Contributor

@SharzyL SharzyL commented Dec 13, 2023

Motivation

When using nix copy with a ssh-ng destination, the flag substitute-on-destination is ignored, instead the builder-use-substitute setting is used.

Refer to #9591 for related source code and reproduction steps.

Context

Closes: #9591, #8744

For legacy ssh destination, --substitute-on-destination is used and builder-use-substitute setting is ignored. It seems reasonable to keep consistency with this strategy since builder-use-substitute looks unrelated to nix copy.

Priorities

Add 👍 to pull requests you find important.

@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Dec 13, 2023
@Ericson2314
Copy link
Member

Yup it does seem to be a mistake going back to 3a63fc6. Whereas in the same commit: https://github.com/NixOS/nix/blob/3a63fc6cd515fb009b17d864fede23a356832a5e/src/libstore/legacy-ssh-store.cc

Can you add a test for this, ensuring both protocols do the right thing?

@thufschmitt
Copy link
Member

Can you add a test for this, ensuring both protocols do the right thing?

I also wanted to ask for that, but then I realised that I couldn't think of any way of testing it that's not badly contrived. Do you have an idea of how we could do it?

@Ericson2314
Copy link
Member

What happens when one tries to copy over a path that they don't have, but the substituter does?

@thufschmitt
Copy link
Member

What happens when one tries to copy over a path that they don't have, but the substituter does?

I think it will first try to realise it locally, and fail if it can't

@Ericson2314
Copy link
Member

Ah true. Perhaps we are entering contrived now, but: we could corrupt it locally and see which version got copied over. (Copying the corrupted version would fail.)

@Ericson2314
Copy link
Member

I'll just give it a shot myself. Since this sounds much harder than the actual fix.

@Ericson2314
Copy link
Member

Ok #9604 adds the rest, skipping ssh-ng. @thufschmitt if you approve that then I can make this one no longer skip in the ssh-ng case, and then we are good!

@thufschmitt
Copy link
Member

Ok #9604 adds the rest

Yay, thanks

Let's merge it then, thanks a lot for the fix @SharzyL !

@thufschmitt thufschmitt merged commit b1c633c into NixOS:master Dec 13, 2023
8 checks passed
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Dec 13, 2023
It works with both `ssh://` and `ssh-ng://` now since NixOS#9600 (and
`ssh-ng:// didn't work before that).

Also, by making the two tests share code, we nudge ourselves towards
making sure there is feature parity.
tebowy pushed a commit to tebowy/nix that referenced this pull request Jul 11, 2024
fix: nix copy ssh-ng:// not respecting --substitute-on-destination
(cherry picked from commit b1c633c)
Change-Id: I77356d14a9011d6dc4cf64776995f7590d918874
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
store Issues and pull requests concerning the Nix store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nix copy --substitute-on-destination has no effect
3 participants