git: Always use OpenSSL on win32

[HybridEidolon]

By default, libssh2 uses Windows Cryptography Next Generation when targeting win32. The wincng backend does not support ED25519, which is a widely-used algorithm among git hosting services, and in some cases may be the only option on remotes in certain configurations.

This change adds the feature openssl-on-win32, enabling the same one in libssh2-sys, and enables it in the CI release flow, ensuring that release builds are able to communicate with hosts using ED25519.

Fixes #3322

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• (N/A) I have updated the config schema (cli/src/config-schema.json)
• (N/A) I have added tests to cover my changes

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[HybridEidolon]

I updated my CLA signature.

[martinvonz]

Thanks! Hopefully someone who uses Windows can review and test this. If we don't hear from anyone in a few days, I can add a rubberstamp to get this in.

FYI, there's also draft PR #3191 for switching from libssh2 to OpenSSH.

[gulbanana]

I'm hopeful that this will actually fix several other git ssh issues, but I guess we shouldn't get ahead of ourselves :)

[gulbanana]

This dependency could be made Windows-only by moving it to a [target.'cfg(windows)'.dependencies] section.

[HybridEidolon]

This isn't in the dependencies key; it's in the workspace.dependencies key. I tried target.'cfg(windows)'.workspace.dependencies and it said it was not allowed.

[jhechtf]

I use windows, let me pull this down and give it a look.

EDIT: Build fails for me, is there something more I need to do?

[steveklabnik]

This change necessitates Windows users building jj to install openssl via vcpkg for the target triple x64-windows-static-md when building for MSVC, or otherwise have OpenSSL available at build time.

FWIW, this can be a huge pain. I really, really would prefer not to use openssl unless I have to, and I don't need the features in this PR. I think it might be nicer to have the option to use openssl if you want these extra features for some extra pain in building, but that's just my own personal opinion :)

[khionu]

Maybe this could be a feature flag?

[HybridEidolon]

Maybe this could be a feature flag?

It is technically already one, but enabled during release builds in CI.

This change necessitates Windows users building jj to install openssl via vcpkg for the target triple x64-windows-static-md when building for MSVC, or otherwise have OpenSSL available at build time.

FWIW, this can be a huge pain. ...

I hadn't actually updated the PR text yet; it now uses the vendored-openssl feature that's already turned on for release builds for Windows and macOS, so it's not disruptive and vcpkg is only needed if you specifically enable openssl-on-win32 and disable vendored-openssl.

To my knowledge, OpenSSL is only used for the cryptographic primitives in libssh2 and not for TLS relative stuff.

[jhechtf]

Clarifying question for me; would this allow me to use jj git push on windows now? I can't do it because my ID key for github is an ed2559 key so i have to manually do git push and it's a bit annoying.

[HybridEidolon]

Clarifying question for me; would this allow me to use jj git push on windows now? I can't do it because my ID key for github is an ed2559 key so i have to manually do git push and it's a bit annoying.

Yes. The issue goes both for your own key and the host's, I believe.

[Zoybean]

I'm on windows, tried using this but I can't get openssl-sys to compile. It needs the openssl libs and headers to be available, but I think I only have the libs as part of my git-for-windows install.

(looks like github ate my last comment. this was 2nd attempt)

[HybridEidolon]

I'm on windows, tried using this but I can't get openssl-sys to compile. It needs the openssl libs and headers to be available, but I think I only have the libs as part of my git-for-windows install.

You can use the vendored-openssl feature to compile it statically. This is normally turned on in CI for release builds already.

[farnoy]

I tried testing it with $ cargo install --locked --bin jj --git https://github.com/HybridEidolon/jj --branch push-pklwzruzswky jj-cli --features vendored-openssl,openssl-on-win32 --force but it fails on vcpkg when compiling openssl-sys. I thought this was supposed to work without having openssl installed on the system?

[HybridEidolon]

but it fails on vcpkg when compiling openssl-sys. I thought this was supposed to work without having openssl installed on the system?

It is; the vendored-openssl feature is never supposed to invoke vcpkg. Per the openssl crate documentation,

If the vendored Cargo feature is enabled, the openssl-src crate will be used to compile and statically link to a copy of OpenSSL. The build process requires a C compiler, perl (and perl-core), and make.

[Zoybean]

the vendored-openssl feature is never supposed to invoke vcpkg. Per the openssl crate documentation,

Not sure if having the log helps, but Installing the latest version (c59e79d) with: cargo install --path cli --locked --features packaging,openssl-on-win32,vendored-openssl, I get the following:

error: failed to run custom build command for `openssl-sys v0.9.101`

Caused by:
  process didn't exit successfully: `C:\..redacted..\jj\target\release\build\openssl-sys-88af3904954fd47d\build-script-main` (exit code: 101)
  --- stdout
  cargo:rerun-if-env-changed=X86_64_PC_WINDOWS_MSVC_OPENSSL_LIB_DIR
  X86_64_PC_WINDOWS_MSVC_OPENSSL_LIB_DIR unset
  cargo:rerun-if-env-changed=OPENSSL_LIB_DIR
  OPENSSL_LIB_DIR unset
  cargo:rerun-if-env-changed=X86_64_PC_WINDOWS_MSVC_OPENSSL_INCLUDE_DIR
  X86_64_PC_WINDOWS_MSVC_OPENSSL_INCLUDE_DIR unset
  cargo:rerun-if-env-changed=OPENSSL_INCLUDE_DIR
  OPENSSL_INCLUDE_DIR unset
  cargo:rerun-if-env-changed=X86_64_PC_WINDOWS_MSVC_OPENSSL_DIR
  X86_64_PC_WINDOWS_MSVC_OPENSSL_DIR unset
  cargo:rerun-if-env-changed=OPENSSL_DIR
  OPENSSL_DIR unset
  note: vcpkg did not find openssl: Could not find Vcpkg tree: No vcpkg installation found. Set the VCPKG_ROOT environment variable or run 'vcpkg integrate install'

  --- stderr
  thread 'main' panicked at C:\..redacted..\.cargo\registry\src\index.crates.io-6f17d22bba15001f\openssl-sys-0.9.101\build\find_normal.rs:190:5:


  Could not find directory of OpenSSL installation, and this `-sys` crate cannot
  proceed without this knowledge. If OpenSSL is installed and this crate had
  trouble finding it,  you can set the `OPENSSL_DIR` environment variable for the
  compilation process.

  Make sure you also have the development packages of openssl installed.
  For example, `libssl-dev` on Ubuntu or `openssl-devel` on Fedora.

  If you're in a situation where you think the directory *should* be found
  automatically, please open a bug at https://github.com/sfackler/rust-openssl
  and include information about your system as well as this message.

  $HOST = x86_64-pc-windows-msvc
  $TARGET = x86_64-pc-windows-msvc
  openssl-sys = 0.9.101


  It looks like you're compiling for MSVC but we couldn't detect an OpenSSL
  installation. If there isn't one installed then you can try the rust-openssl
  README for more information about how to download precompiled binaries of
  OpenSSL:

  https://github.com/sfackler/rust-openssl#windows


  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[HybridEidolon]

Not sure if having the log helps, but Installing the latest version (c59e79d) with: cargo install --path cli --locked --features packaging,openssl-on-win32,vendored-openssl, I get the following:

Could you try building it after checking out the commit with cargo build instead? I'm not sure what is happening here, but I am building for MSVC locally in a clean context and I don't see this at all.

[Zoybean]

building cli at c59e79d:

• cargo build --locked --features packaging,openssl-on-win32,vendored-openssl fails
• cargo build --locked --features openssl-on-win32,vendored-openssl fails
• cargo build --locked --features openssl-on-win32 fails
• cargo build --locked and no features succeeds of course

All the failures had what appeared to be the same error message, though I didn't bother to run them through diff.
(I forget why I added the packaging feature in the first place but it looks like it doesn't affect this error)

[HybridEidolon]

Oh boy, building openssl-sys with the vendored feature only seems to work under msys2 shell on Windows.

Huh...

[gulbanana]

Aha. I was also using msys2 - the version that comes with Git for Windows as "Git Bash".

[Zoybean]

If it helps, I've been building under nushell, but I get the same failure when running in git bash. I don't have an explicit msys2 install.

[HybridEidolon]

I guess I could alter this to not run in CI release packaging, since it does work, it just requires a specific build environment? All it does is forward the feature down to the libssh-sys crate ultimately. I do find it's the only way to make jj useful in my specific use case.

[jhechtf]

I would definitely like for jj git push to work on Windows for me, but AFAIK the only way to install it is from source, and I can never seem to get this pr to build.