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.

Sign up for GitHub

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

Jump to bottom

ssh-copy-id: init at 8.4p1 #105896

Merged
merged 3 commits into from Apr 8, 2021
Merged

ssh-copy-id: init at 8.4p1 #105896

merged 3 commits into from Apr 8, 2021

Conversation

midchildan
Copy link
Member

@midchildan midchildan commented Dec 4, 2020 •
edited

Split the ssh-copy-id tool from the openssh package into a separate output. This mainly benefits macOS users because the version of openssh that is preintalled in macOS doesn't provide ssh-copy-id.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested review from aneeshusa and edolstra December 4, 2020 17:45
@SuperSandro2000
Copy link
Member

Please target staging with the mass rebuild.

Also I am not sure if this is worth the benefit. The openssh package is ~6.23 MB and over 1000 other packages want to download it anyway.

@midchildan midchildan changed the base branch from master to staging December 5, 2020 00:03
@midchildan
Copy link
Member Author

The main problem with bundling ssh and ssh-copy-id into a single package is that it would overshadow the preinstalled ssh. This is undesirable on macOS because the preinstalled version is tweaked to integrate with the system keychain. Replacing that with openssh upstream would break existing workflows for many users.

@midchildan
Copy link
Member Author

I've just successfully ran nixos/tests/openssh.nix.

@midchildan
Copy link
Member Author

Package closure size

From 20.09:

❯ nix path-info -S "$(which ssh)"
/nix/store/jln5qqzyy4laawfp5y4f4lqgiwja9n1l-openssh-8.3p1	 171975736

This PR:

❯ nix path-info -S "$(nix-build --no-out-link -A openssh.out)"
/nix/store/lw81h47vxsinc1ayxvpbc1a2vqcgv59j-openssh-8.4p1	 111366840
❯ nix path-info -S "$(nix-build --no-out-link -A openssh.man)"
/nix/store/afvy3yac5jr616jvapa8f1h32zp9nnjj-openssh-8.4p1-man	     90808
❯ nix path-info -S "$(nix-build --no-out-link -A openssh.ssh_copy_id)"
/nix/store/w1wxr8kjfgm40m7dxnjwh1xidf4ybvvr-openssh-8.4p1-ssh_copy_id	  34288168

@SuperSandro2000
Copy link
Member

Replacing that with openssh upstream would break existing workflows for many users.

But why do you need copy-id then? Can't you just use the system one or add an alias for copy-id to the nix copy id one?

@midchildan
Copy link
Member Author

midchildan commented Dec 11, 2020 •
edited

macOS doesn't come preinstalled with ssh-copy-id so people install it through package managers. For what it's worth, it ranks 271 on the list of popular packages in Homebrew so it's a somewhat popular package on macOS 1.

On the other hand, for the alias approach to work, the nix profile path would have to come after /usr/bin which I believe is uncommon as it's not the default. I think it's best if people could install ssh-copy-id directly.

matthewbauer
matthewbauer reviewed Dec 14, 2020
View reviewed changes
pkgs/tools/networking/openssh/default.nix Outdated
mkdir -p $ssh_copy_id/bin $man/share/man/man1
cp contrib/ssh-copy-id $ssh_copy_id/bin/
chmod +x $ssh_copy_id/bin/ssh-copy-id
cp contrib/ssh-copy-id.1 $man/share/man/man1/
Copy link
Member

Choose a reason for hiding this comment

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

To avoid breaking things, can you install a symlink in $out as well? This is so that things like lib.getBin openssh have all of the outputs. This still allows for users to install ssh_copy_id output.

Something like:

$ ln -s $ssh_copy_id/bin/ssh-copy-id $out/bin/ssh-copy-id

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

matthewbauer
matthewbauer reviewed Dec 14, 2020
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
@@ -6251,6 +6251,8 @@ in
pam = if stdenv.isLinux then pam else null;
};

ssh-copy-id = openssh.ssh_copy_id;
Copy link
Member

Choose a reason for hiding this comment

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

Note, that how nix-env works will ignore which output selected since. I think nix profile install may not have this same issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh my, I wasn't aware of this. Running nix-env -f . -A ssh-copy-id does indeed link the whole ssh package into my user profile.

I think I'll change this PR to split out the ssh-copy-id package instead of using multiple outputs, since nix-env would likely be here to stay for years to come.

@midchildan
Copy link
Member Author

I've split ssh-copy-id from openssh into a separate package so that nix-env -iA ssh-copy-id won't also link openssh into the user profile.

@SuperSandro2000
Copy link
Member

@midchildan Can you please resolve the merge conflict?

@midchildan midchildan changed the base branch from staging to master March 16, 2021 16:33
@midchildan
Copy link
Member Author

I've changed this PR to use runCommand instead following the suggestion from @dasJ. I also changed the target branch to master because no rebuilds would happen with this approach.

@ofborg ofborg bot requested review from Artturin and delroth March 16, 2021 16:42
@SuperSandro2000 SuperSandro2000 merged commit 2e2a36f into NixOS:master Apr 8, 2021
@midchildan midchildan deleted the package/ssh-copy-id branch April 8, 2021 13:28
cideM pushed a commit to cideM/nixpkgs that referenced this pull request Apr 9, 2021
@midchildan @SuperSandro2000 @cideM
ssh-copy-id: init at 8.4p1 (NixOS#105896)
6655898 
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@aneeshusa aneeshusa Awaiting requested review from aneeshusa

@edolstra edolstra Awaiting requested review from edolstra

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@Artturin Artturin Awaiting requested review from Artturin

@delroth delroth Awaiting requested review from delroth

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@midchildan @SuperSandro2000 @dasJ @matthewbauer