Skip to content

Add build tag to omit the x/crypto/ssh dependency#645

Open
espadolini wants to merge 1 commit intopkg:masterfrom
espadolini:omit-ssh-tag
Open

Add build tag to omit the x/crypto/ssh dependency#645
espadolini wants to merge 1 commit intopkg:masterfrom
espadolini:omit-ssh-tag

Conversation

@espadolini
Copy link
Copy Markdown

The only use of golang.org/x/crypto/ssh (and, transitively, of any package in crypto/...) is in the NewClient function, which is a small wrapper around running a subsystem request with an ssh.Client to then run a sftp client on the channel.

For binaries that don't otherwise depend on x/crypto/ssh (for example utility binaries that only do sftp through stdio) this can be a somewhat significant chunk of binary size; for example a stripped cgoless build of server_standalone (darwin arm64, go 1.26.1) goes from 2.9 to 2.0 MiB, which is (relatively) significant.

This PR moves NewClient to a new file which is omitted from the build if a new pkg_sftp.omit_ssh build tag is set during the build, so users of the library that don't use x/crypto/ssh can omit that dependency.

Copy link
Copy Markdown
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

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

I mean… this doesn’t remove it from the go.mod so it’s going to get pulled in anyways?

Dead-code analysis should let this be removed anyways?

If someone has serious concerns about 900 KiB of binary size, then they’ll probably want to vendor/fork and do significantly more manual dead code removal than add an esoteric, and poorly discoverable build flag option?

@espadolini
Copy link
Copy Markdown
Author

Dead-code analysis should let this be removed anyways?

DCE can't change the semantics of the code, and there's enough init time code in crypto/... to prevent meaningful DCE.

I mean… this doesn’t remove it from the go.mod so it’s going to get pulled in anyways?

Adding a build tag seemed less invasive than moving all code except for NewClient into a subpackage and reexporting everything from the top-level package. 😅

I'd happily take that as an option tho, it would work way better.

If someone has serious concerns about 900 KiB of binary size, then they’ll probably want to vendor/fork and do significantly more manual dead code removal

DCE seems to work quite well for sftp - at least in the case of a full featured server such as the one in server_standalone, I'm not really seeing any client code and github.com/kr/fs is also fully elided from the build - so it's just this one spurious dependency for x/crypto/ssh that's adding to the binary size.

900KiB is not a lot in absolute terms but it's a third of the binary, in the case of server_standalone, so it's just a little unfortunate, that's all.

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.

2 participants