got: add openssh as input and fix path to the ssh(1) executable

[omar-polo]

Description of changes

got uses an hardcoded path /usr/bin/ssh, so got clone/send/fetch don't work, the other commands are fine.

I'm not sure if openssh should be added to buildInputs too: got itself works without openssh, ssh is needed "only" for the network activity.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[afh]

Great find @omar-polo!!

Happy to report that the proposed changes work and indeed use ssh from nixpkgs on my aarch64-darwin machine, even though macOS provides ssh at the got hard-coded path /usr/bin/ssh.

Unfortunately I do not have a clear answer regarding your question about whether openssh should be added to buildInputs (or maybe propagatedBuildInputs?) or not.

@vcunat, @wegank as folks who have contributed/committed to the got or openssh package could you provide some guidance here or involve folks you think would be able to offer advice?

[vcunat]

In terms of build inputs, I'd leave this as it is.

[wahjava]

In terms of build inputs, I'd leave this as it is.

How about we replace /usr/bin/ssh with ssh, patch it to use execvp instead of execv, so whichever ssh user chose to have in their PATH (as we have n variants of OpenSSH) it will work with all of them as expected without depending on any in particular.

[afh]

Interesting approach, @wahjava. I'd like to recommend that any such patches would also be proposed upstream.

If we wanted more flexibility with the actual ssh package being used I'd like to suggest a custom input on the got package that can be overridden, e.g.:

pkgs/applications/version-management/got/default.nix:

{
# …
, ssh
# …
}: 

pkgs/applications/version-management/got/default.nix

got = darwin.apple_sdk_11_0.callPackage ../applications/version-management/got {
  ssh = openssh;
};

[vcunat]

You can override it even if you keep the name as it is. Just got.override { openssh = something; }

I'm not sure about conventions around this in nixpkgs. Probably nothing restrictive.

[afh]

Thanks for chiming in, @vcunat, d'oh (to myself 🤦😅): of course you override openssh; although introducing a new ssh input might aid expressing more explicitly that any ssh implementation may be suitable here.

[wahjava]

Thanks for chiming in, @vcunat, d'oh (to myself 🤦😅): of course you override openssh; although introducing a new ssh input might aid expressing more explicitly that any ssh implementation may be suitable here.

The disadvantage of this approach is, creating unneeded explicit dependency between ssh, and got, which is not even present in the upstream sources itself either, so everytime ssh is bumped, this will also get rebuilt, and someone wanting to use a different SSH flavour have to override this themselves, and triggering a source rebuild.

I agree that we should consider upstream's advice to this problem (aka their blessing) esp. if we go for the patching it downstream part, or just submit an bug report on their bug tracker.

[omar-polo]

@wahjava I haven't realized that going with this approach got needs to be rebuilt every time openssh is, which is a bit annoying. So, should we go with changing the execv with execvp and set the path to just ssh? I'll update the PR soon, and will ask the other developers upstream what they think of the change.

[jrick]

If we want to run all of the regression tests then we would need openssh as a build input.

That said, I don't see checks being disabled and was quite curious why they were passing. According to the logs nothing is being tested! The regress directories are being entered by make, but no tests are ran. I'm unsure if this is expected or a issue with the got-portable port.

(I also imagine it would not be very useful to drop in a different ssh implementation, since got is developed on openbsd and built with openssh in mind. Some of the features rely on openssh-specific behaviors and flags.)

[afh]

FYI: got 0.98.2 depends on libressl rather than openssl (see #307538)

[AndersonTorres]

The disadvantage of this approach is, creating unneeded explicit dependency between ssh, and got,

use a Boolean flag, sshSupport ? false, that will control if SSH dependency will be bundled or not.
The attribute ssh can have any default you desire.

[afh]

@omar-polo is what @AndersonTorres suggested 👆 something you'd like to take on and add to this PR?

[omar-polo]

@afh I think it could work, but I think it's outside of my nix comfort zone to add a flag to a derivation ^^"

Do we really want to add a knob for this though? Is using a distributed version control without the ability to fetch/push useful? (ok, plain git:// and read-only https:// still works, but...)

edit: in future versions of got-portable I believe the tests will be bundled, and those will also need openssh.

[eclairevoyant]

I've created #322202 as a different approach, let me know if that works

[afh]

Closing in favor of merged PR #322600