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

Add UUID.v1, .v2, .v3, .v4, .v5 #13693

Merged
merged 14 commits into from
Oct 20, 2023

Conversation

threez
Copy link
Contributor

@threez threez commented Jul 22, 2023

add missing uuid methods v1-v5

fixes #13691

src/uuid.cr Outdated Show resolved Hide resolved
@threez threez force-pushed the add-missing-uuid-methods branch 2 times, most recently from 1679849 to 9974c8a Compare July 23, 2023 17:11
@threez threez requested a review from Sija July 23, 2023 17:12
src/uuid.cr Outdated Show resolved Hide resolved
@threez threez requested a review from Sija July 23, 2023 19:41
Copy link
Contributor

@Sija Sija left a comment

Choose a reason for hiding this comment

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

src/uuid.cr Outdated Show resolved Hide resolved
src/uuid.cr Outdated Show resolved Hide resolved
src/uuid.cr Outdated Show resolved Hide resolved
src/uuid.cr Show resolved Hide resolved
src/uuid.cr Show resolved Hide resolved
@threez threez force-pushed the add-missing-uuid-methods branch 2 times, most recently from 7610bd1 to 92403c6 Compare July 26, 2023 07:06
@beta-ziliani
Copy link
Member

Hi @threez please avoid force pushes, because that breaks the history of reviews and makes it harder for reviewers to re-review the work 🙏

benchmarks show, that for the regular use cases the native
implementations yield better performance then the openssl variant

crystal-lang#13691
@straight-shoota
Copy link
Member

Please use the OpenSSL implementations for hash calculations unless explicitly opted out (-Dwithout_openssl). The native implementations are just a weak kludge. See #13693 (comment)

@threez
Copy link
Contributor Author

threez commented Aug 27, 2023

@straight-shoota I used the {% if flag?(:without_openssl) %} which failes with wasm, but {% if flag?(:wasm32) %} would work. This is not exactly what you requested. Is there something regarding the build that needs to be fixed?

@straight-shoota
Copy link
Member

I suppose we should probably run the WASM specs with -Dwithout_openssl. @HertzDevil WDYT?

@HertzDevil
Copy link
Contributor

There are many use cases of UUIDs that don't involve the need to generate them via the two digests. IMO it is unacceptable to pull in OpenSSL stuffs from require "uuid". (Whether the runtime calls OpenSSL setup functions, and therefore mandatorily links to it when required, is an implementation detail; compare this with #13746 (comment))

@straight-shoota
Copy link
Member

Hm, that's a good point.

However, it's worth noting that this patch does not require OpenSSL explicitly. It just uses the stdlib APIs Digest::SHA1 and Digest::MD5. They are implemented with OpenSSL, but that's an implementation detail of Digest.

This situation is quite unsatisfactory, for sure.
I'm not sure what's the best way to move forward here.

Maybe a good idea could be to refactor Digest implementations to pull in only a subset of the entire OpenSSL bindings. Just the stuff that it needs (which is actually just some parts of lib_crypto). There's no need for any global initialization nor linking any symbols unless the respective methods are actually used.
Applying this diff to all Digest implementations based on OpenSSL should do the trick:

-require "openssl"
+require "openssl/digest"

(We would want to do this as a separate change, though - not as part of this PR)

@straight-shoota
Copy link
Member

straight-shoota commented Oct 7, 2023

The suggestion to minimize the digest dependency (☝️) has been implemented in #13818.
Now requiring digest/sha1 should not link libcrypto or libssl unless you explicitly call one of the methods that uses this functionality.

I think that's a reasonable solution and I'd be happy to move on with this PR. @HertzDevil WDYT?

The following patch should fix the breaking WASM job:

--- i/.github/workflows/wasm32.yml
+++ w/.github/workflows/wasm32.yml
@@ -40,7 +40,7 @@ jobs:
           rm wasm32-wasi-libs.tar.gz

       - name: Build spec/wasm32_std_spec.cr
-        run: bin/crystal build spec/wasm32_std_spec.cr -o wasm32_std_spec.wasm --target wasm32-wasi -Duse_pcre
+        run: bin/crystal build spec/wasm32_std_spec.cr -o wasm32_std_spec.wasm --target wasm32-wasi -Duse_pcre -Dwithout_opensssl
         env:
           CRYSTAL_LIBRARY_PATH: ${{ github.workspace }}/wasm32-wasi-libs

@threez
Copy link
Contributor Author

threez commented Oct 14, 2023

@straight-shoota merged master and updated the wasm32 target so that openssl would not be compiled using your diff.

src/uuid.cr Outdated Show resolved Hide resolved
threez and others added 2 commits October 17, 2023 21:02
this makes the build more generic and less specific to wasm

Co-authored-by: Quinton Miller <nicetas.c@gmail.com>
@straight-shoota straight-shoota added this to the 1.11.0 milestone Oct 18, 2023
@straight-shoota straight-shoota changed the title add missing uuid methods v1-v5 Add UUID.v1, .v2, .v3, .v4, .v5 Oct 18, 2023
@straight-shoota straight-shoota merged commit 008d76a into crystal-lang:master Oct 20, 2023
51 of 55 checks passed
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
Co-authored-by: Quinton Miller <nicetas.c@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add missing UUID related functions
6 participants