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.

Sign up for GitHub

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

Jump to bottom

Enable and fix for windows build by cmake project file #462

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

Enable and fix for windows build by cmake project file #462

wants to merge 1 commit into from

Conversation

qwerity
Copy link

@qwerity qwerity commented Oct 19, 2024

Enabled by default cmake project files.
Also fixed the Windows build.
Tested for VS2019/VS2022.

@qwerity qwerity force-pushed the fix-windows-build branch 2 times, most recently from c3113c0 to a4d9349 Compare October 20, 2024 12:34
@jgriffiths
Copy link
Contributor

jgriffiths commented Oct 20, 2024 •
edited
Loading

Hi @qwerity

Thanks for this! Can I ask you to make the following changes?

  • The secp update should be in its own commit. I'm about to update master shortly so I will update secp there and you can rebase this PR from master.
  • Please remove the indentation from the #include line protected by #ifndef
  • Please either remove the cmake file renaming or add it as a separate commit. We currently have a downstream fork of wally that uses cmake and are in the process of merging them. Until that is complete (i.e. they can switch to our upstream version here) we are keeping the cmake files renamed so they can continue to take our changes. Once that is done we will rename the files.

edit: master is now updated if you'd like to rebase.

jgriffiths
jgriffiths reviewed Nov 26, 2024
View reviewed changes
Copy link
Contributor

@jgriffiths jgriffiths left a comment

Choose a reason for hiding this comment

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

Hi,

Left some review comments. It appears you are not the original author of this code - is the original author OK with you submitting this PR?

.gitignore
@@ -100,3 +100,6 @@ wallycore.wasm
*.pdb
*.ilk
*.exp

.idea
cmake-build-*
Copy link
Contributor

Choose a reason for hiding this comment

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

.idea is already in this file, and cmake build directories should be placed outside the repo rather than listed here.

CMakeLists.txt
@@ -23,15 +27,15 @@ set(SECP256K1_ENABLE_MODULE_ECDH ON)
set(SECP256K1_ENABLE_MODULE_RECOVERY ON)
set(SECP256K1_ENABLE_MODULE_EXTRAKEYS ON)
set(SECP256K1_ENABLE_MODULE_SCHNORRSIG ON)
set(SECP256K1_ENABLE_MODULE_ELLSWIFT OFF)
set(SECP256K1_ENABLE_MODULE_ELLSWIFT ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

The enabled modules here should match the ones set by configure (which they currently do). Why are you turning on extra modules here by default?

cmake/config.h.in
#if (!defined(_SSIZE_T_DECLARED)) && (!defined(_ssize_t)) && (!defined(ssize_t))
#define ssize_t long long
#endif
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I've PR'd a different fix for this in #469

src/ccan/ccan/tap/tap.c

#ifndef _WIN32
#include <unistd.h>
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is not compiled when building, so I don't think this change is needed.

src/ctest/CMakeLists.txt
target_include_directories(test_clear PRIVATE ${CMAKE_BINARY_DIR})
target_link_libraries(test_clear PRIVATE wallycore pthread)
add_test(test_clear test_clear)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

added to #469

src/CMakeLists.txt
@@ -25,7 +25,7 @@ file(
)

# wallycore
add_library(wallycore)
add_library(wallycore STATIC)
Copy link
Contributor

Choose a reason for hiding this comment

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

wally can (or should be able to be) build as a DLL for windows too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgriffiths jgriffiths jgriffiths left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@qwerity @jgriffiths