Skip to content

add a dependency on libssh2 and use a systemIntegrationOption - #27

Open
dasimmet wants to merge 2 commits into
allyourcodebase:mainfrom
dasimmet:feature/libssh2-dependency
Open

dasimmet wants to merge 2 commits into
allyourcodebase:mainfrom
dasimmet:feature/libssh2-dependency

Conversation

@dasimmet

Copy link
Copy Markdown
Contributor

can we enable ssh support without system libssh?

needs this mr to build libssh2 without linking system mbedtls:

allyourcodebase/libssh2#11

@dasimmet
dasimmet force-pushed the feature/libssh2-dependency branch from 63f07f3 to 417b10d Compare October 23, 2026 16:33
@dasimmet
dasimmet force-pushed the feature/libssh2-dependency branch from 417b10d to 39e5de6 Compare October 23, 2026 16:35
@Parzival-3141

Copy link
Copy Markdown
Collaborator

I was just thinking of this! Do you know if we can deduplicate the crypto implementations between libgit2 and libssh2? (And zlib for that matter too)

@dasimmet

Copy link
Copy Markdown
Contributor Author

According to my build summary and zig-pkg directory, mbedtls is already reused (probably only since its the same dependency hash):

Build Summary: 18/18 steps succeeded
install success
+- install git2 success
+- compile lib git2 Debug native success 1s MaxRSS:88M
+- WriteFile mbedtls success
+- compile lib mbedtls Debug native success 1s MaxRSS:115M
+- WriteFile cached
+- compile lib ntlmclient Debug native success 361ms MaxRSS:87M
| +- WriteFile mbedtls (reused)
+- WriteFile success
+- compile lib ssh2 Debug native success 443ms MaxRSS:106M
| +- configure cmake header dependency to libssh2_config.h success
| +- WriteFile mbedtls (reused)
| +- compile lib mbedtls Debug native (reused)
| +- compile lib mbedtls Debug native (reused)
+- WriteFile cached
+- compile lib llhttp Debug native success 786ms MaxRSS:129M
+- WriteFile success
+- compile lib pcre Debug native success 886ms MaxRSS:178M
| +- configure cmake header dependency to config.h success
+- WriteFile cached
+- compile lib z Debug native success 376ms MaxRSS:91M
+- configure cmake header dependency to git2_features.h success
+- compile lib mbedtls Debug native (reused)
+- compile lib ntlmclient Debug native (+1 more reused dependencies)
+- compile lib ssh2 Debug native (+4 more reused dependencies)
+- compile lib llhttp Debug native (reused)
+- compile lib pcre Debug native (+1 more reused dependencies)
+- compile lib z Debug native (reused)

When trying to enable zlib on libssh the vendored zlib from the libgit2 sources will still be compiled and i guess we rely on the linker surviving both versions somehow, right?
We could

  • use some extra code to get and link the transitive zlib dependency from libssh2
  • add the same zlib dependency to our build.zig.zon, meaning we need to keep them in sync.

@Parzival-3141

Parzival-3141 commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Sweet. For zlib I'm leaning towards the second option. It has a higher chance of reusing the binary and it won't break the build script if they get out of sync. The only hiccup is providing some specific C flags (-DNO_VIZ and -DNO_GZIP). But I think something like this will work?

zlib_dep.artifact("z").root_module.addCMacro("NO_GZIP");

EDIT: on second thought, messing with C flags will probably prevent reuse if the configurations are different. Ignore that last part!

@dasimmet

dasimmet commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

Sweet. For zlib I'm leaning towards the second option. It has a higher chance of reusing the binary and it won't break the build script if they get out of sync. The only hiccup is providing some specific C flags (-DNO_VIZ and -DNO_GZIP). But I think something like this will work?

zlib_dep.artifact("z").root_module.addCMacro("NO_GZIP");

EDIT: on second thought, messing with C flags will probably prevent reuse if the configurations are different. Ignore that last part!

on a build system level we need to have the exact same compile command to reuse. NO_VIZ does only appear in the changelog of upstream libz, i think its a legacy option the libgit2 maintainers left in there and it can be dropped.
NO_GZIP is something libssh probably also could use (we would need to add it to the zlib zig package options first i guess).
But it makes me curious how build.zig behaves if we do either option and add a macro to an artifact from a dependency in post...for option 1 does it alter the library libssh2 is linked against as well?

@Parzival-3141

Copy link
Copy Markdown
Collaborator

But it makes me curious how build.zig behaves if we do either option and add a macro to an artifact from a dependency in post...for option 1 does it alter the library libssh2 is linked against as well?

For option 1 I would assume so, since we'd be reaching into its dependency. For option 2, I would assume it's just a cache miss and our version gets built separately. Worth investigating! In the meantime lets add the matching zlib dependency to our zon file.

@Parzival-3141 Parzival-3141 mentioned this pull request Oct 24, 2026
3 tasks
@dasimmet

dasimmet commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

I just tried option1, and your right, zlib_dep.artifact("z").root_module.addCMacro("NO_GZIP", "") adds a -DNO_GZIP= argument to the transient dependency lib without building it twice.

Added the zlib dep now.

When it gets really complicated i guess is if we were to enable linking system zlib.
I don't think we can pass "b.systemIntegrationOption" to dependencies and if we link system zlib but libssh2 ships its own (or the other way around) we still have one of them linked statically.

@Parzival-3141 Parzival-3141 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Whoops I somehow missed a commit :P

Comment thread build.zig
if (b.lazyDependency("libssh2", .{
.target = target,
.optimize = optimize,
.@"crypto-backend" = .mbedtls,

@Parzival-3141 Parzival-3141 Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should choose the appropriate setting for the target platform and selected tls backend, falling back to mbedtls when necessary.

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