-
Notifications
You must be signed in to change notification settings - Fork 3
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
adapt NostrSDK to vcpkg build system #4
adapt NostrSDK to vcpkg build system #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. I left two minor comments. Please take a look at those, then feel free to merge when you're ready.
src/nostr_service.cpp
Outdated
uuid uuid = random_generator()(); | ||
return to_string(uuid); | ||
UUIDv4::UUIDGenerator<std::mt19937_64> uuidGenerator; | ||
UUIDv4::UUID uuid = uuidGenerator.getUUID(); | ||
return uuid.bytes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add using UUIDv4::UUIDGenerator
and using UUIDv4::UUID
to the usings block at the top of this file? That will keep the code a bit more readable in-line, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit
2c9956c
Boost::random | ||
Boost::system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stating for the record that we may need to re-add Boost at a later date. I believe websocketpp
uses it as a dependency, but we don't test the websocketpp
usage directly, as of yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, websocketpp uses it as a dependency but vcpkg downloads the required parts of boost (already compiled) for websocketpp
But if we directly use boost, vcpkg will have to download all of it fr and compile it from source which took me around 2hrs, and all that just for generating UUIDs... it's a lot of overhead in my opinion.
If we need it in the future, we'll cross that bridge when we get to it.
@buttercat1791
As you will see, I've scarcely changed anything other than adapt CMakeLists.txt to the more global build of
GitRepublic
Please clone GitRepublic and work from within it after you approve of this PR.