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
+13 −0

Conversation

midchildan
Copy link
Member

@midchildan midchildan commented Dec 4, 2020 •
edited
Loading

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.

Sorry, something went wrong.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Dec 4, 2020
@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
Loading

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

@midchildan midchildan Dec 15, 2020 •
edited
Loading

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.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 2.status: merge conflict This PR has merge conflicts with the target branch labels Dec 15, 2020
@ofborg ofborg bot removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 2.status: merge conflict This PR has merge conflicts with the target branch labels Dec 15, 2020
@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 SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 27, 2020
@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
@midchildan midchildan force-pushed the package/ssh-copy-id branch from 4ca56f1 to 7b61164 Compare April 6, 2021 01:47
@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)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
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

@SuperSandro2000 SuperSandro2000

@aneeshusa aneeshusa

@edolstra edolstra

@FRidh FRidh

@jonringer jonringer

@Artturin Artturin

@delroth delroth

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 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