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.

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

Remove openssl #1700

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Remove openssl #1700

wants to merge 1 commit into from

Conversation

mamcx
Copy link
Contributor

@mamcx mamcx commented Sep 11, 2024

Description of Changes

Using the openssl crate significantly increases the build times on Windows. Switching to rustls improves it:

# Windows 11
# 1.79.0-x86_64-pc-windows-msvc 
# AMD Ryzen 9 7940HS w/ Radeon 780M Graphics        4.00 GHz
#  32.0 GB (25.8 GB usable)

   Compiling spacetimedb-core v0.12.0 (C:\Proyectos\space\SpacetimeDB\crates\core)
   Compiling spacetimedb-client-api v0.12.0 (C:\Proyectos\space\SpacetimeDB\crates\client-api)
   Compiling spacetimedb-standalone v0.12.0 (C:\Proyectos\space\SpacetimeDB\crates\standalone)   

OpenSSL:

    Finished `dev` profile [unoptimized + debuginfo] target(s) in 4m 01s
    Finished `release` profile [optimized] target(s) in 4m 39s    

RusTls:

    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1m 21s
    Finished `release` profile [optimized] target(s) in 2m 02s    

Expected complexity level and risk

*How complicated do you think these changes are? Grade on a scale from 1 to 5,

3

It looks correct to me the generation and reading of keys. However, even if trivial, changing libraries that do crypto requires careful eyes.

Testing

  • * Check using https://8gwifi.org/PemParserFunctions.jsp that we use the same algorithms and settings as from openssl.*
  • Publish quickstart and re-read the keys
  • Manual inspection of the keys and config files

@mamcx mamcx added release-any To be landed in any release window no runtime change This change does not affect the final binaries labels Sep 11, 2024
@mamcx mamcx self-assigned this Sep 11, 2024
@gefjon
Copy link
Contributor

gefjon commented Sep 11, 2024

I strongly request that we not do potentially dangerous changes which are not intended to be API breaks within the 0.12 release window. This can, and therefore should, wait.

@mamcx mamcx marked this pull request as draft September 11, 2024 19:16
@mamcx
Copy link
Contributor Author

mamcx commented Sep 11, 2024

Changed to draft so we don't merge yet

@mamcx
Copy link
Contributor Author

mamcx commented Sep 12, 2024

Btw, this adds cmake as a requirement on Windows.

@RReverser
Copy link
Member

RReverser commented Sep 12, 2024

Btw, this adds [cmake](https://aws.github.io/aws-lc-rs/requirements/windows0 as a requirement on Windows.

Sigh. It's somewhat unfortunate to replace one external dependency for Windows devs (Perl or prebuilt OpenSSL) with another (CMake).

@mamcx
Copy link
Contributor Author

mamcx commented Nov 14, 2024

Is now good time to change this from Draft?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no runtime change This change does not affect the final binaries release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants