peercoin / coinlib

The most feature-complete *coin library in the entire Dart/Flutter ecosystem.
BSD 3-Clause "New" or "Revised" License
5 stars 8 forks source link

WIP: Windows Support for v2.0 #25

Closed MatthewLM closed 5 months ago

MatthewLM commented 6 months ago

@sneurlax I've made a few fixes to the windows branch. Unfortunately the build_windows_crosscompile.dart file is not working for me. I've made some fixes to the Dockerfile (See https://github.com/peercoin/coinlib/commit/37525526182b2d05bbff735c93cd4240ef7d679a) but it still fails at this step:

STEP 5/8: RUN cmake .. -DCMAKE_TOOLCHAIN_FILE=../cmake/x86_64-w64-mingw32.toolchain.cmake
-- The C compiler identification is unknown
-- Configuring incomplete, errors occurred!
See also "/secp256k1/build/CMakeFiles/CMakeOutput.log".
See also "/secp256k1/build/CMakeFiles/CMakeError.log".
Build of coinlib_build_secp256k1_windows failed

I'm guessing a windows cross compiler needs to be installed but I'm not sure what. Is this something you are able to take a look into?

I will delay publishing anything until this is working.

sneurlax commented 6 months ago

Ah yes, there seems to be an undocumented dependency for gcc-mingw-w64.

Resolve via sudo apt-get install gcc-mingw-w64.

Sorry about that!

MatthewLM commented 6 months ago

@sneurlax Are you able to test that the DLL I compiled on Linux works for you? I've been able to compile but I don't have a Windows machine available to test. I've uploaded it here: https://drive.proton.me/urls/2KPTYE02MC#vr1z9WkgTj1j

Have you tested both the build_wsl and build_windows build methods? I'll publish a windows test package and ask you and some others to give it a test.

Thanks again.

sneurlax commented 6 months ago

Yes, the build_wsl and build_windows scripts both work for us over here (3 devs using coinlib on windows)

I'll test your DLL right now, few moments...

sneurlax commented 6 months ago

I confirmed

@sneurlax Are you able to test that the DLL I compiled on Linux works for you? I've been able to compile but I don't have a Windows machine available to test. I've uploaded it here: https://drive.proton.me/urls/2KPTYE02MC#vr1z9WkgTj1j

Have you tested both the build_wsl and build_windows build methods? I'll publish a windows test package and ask you and some others to give it a test.

Thanks again.

I tested with your DLL, it works just fine 👍 just had to remove the lib prefix on the filename

sneurlax commented 6 months ago

One thing to note that our team at Cypher Stack is finding is that CMAKE_BINARY_DIR in the Windows CMakeLists here can vary from system-to-system: for me, CMAKE_BINARY_DIR=app/build/windows/x64. For another teammember, one of his Windows systems uses CMAKE_BINARY_DIR=app/build/windows. Looking through the variables available to cmake, I'm not sure if any offer a solution: or rather, I'm not sure the solution will be found in the CMakeLists, but probably rather in the build script. We are considering hacking the build script to copy multiple DLLs so one is always found, but this isn't ideal (not very elegant/clean). Maybe more testers will shed more light on the situation?

MatthewLM commented 6 months ago

One thing to note that our team at Cypher Stack is finding is that CMAKE_BINARY_DIR in the Windows CMakeLists here can vary from system-to-system: for me, CMAKE_BINARY_DIR=app/build/windows/x64. For another teammember, one of his Windows systems uses CMAKE_BINARY_DIR=app/build/windows. Looking through the variables available to cmake, I'm not sure if any offer a solution: or rather, I'm not sure the solution will be found in the CMakeLists, but probably rather in the build script. We are considering hacking the build script to copy multiple DLLs so one is always found, but this isn't ideal (not very elegant/clean). Maybe more testers will shed more light on the situation?

Are you able to find a solution in the CMakeLists.txt file such as using find_file?

I'll delay publishing a test package until this is solved.

sneurlax commented 6 months ago

I have never used find_file, I was hoping something like a relative path from CMAKE_CURRENT_SOURCE_DIR or another CMake variable would work. I'll try to get one of those working now.

sneurlax commented 6 months ago

One thing to note that our team at Cypher Stack is finding is that CMAKE_BINARY_DIR in the Windows CMakeLists here can vary from system-to-system: for me, CMAKE_BINARY_DIR=app/build/windows/x64. For another teammember, one of his Windows systems uses CMAKE_BINARY_DIR=app/build/windows. Looking through the variables available to cmake, I'm not sure if any offer a solution: or rather, I'm not sure the solution will be found in the CMakeLists, but probably rather in the build script. We are considering hacking the build script to copy multiple DLLs so one is always found, but this isn't ideal (not very elegant/clean). Maybe more testers will shed more light on the situation?

Are you able to find a solution in the CMakeLists.txt file such as using find_file?

I'll delay publishing a test package until this is solved.

OK, this issue should be resolved by https://github.com/peercoin/coinlib/pull/26, which should be merged before this one goes to testing.

In order to confirm this I had to resolve some issues in the example app that appeared to be due to changes in 2.0, but they're outside the scope of Windows support so are neither here nor there. We merged windows support in on our side and it resolved the CMAKE_BINARY_DIR mismatch issue mentioned before.

MatthewLM commented 6 months ago

In order to confirm this I had to resolve some issues in the example app that appeared to be due to changes in 2.0, but they're outside the scope of Windows support so are neither here nor there.

I'm unaware of any issues with the example app. Please let me know if there are any.

I merged your find_file changes, thank you. I'm going to resolve conflicts and publish a test package soon.

MatthewLM commented 6 months ago

@sneurlax I've published a new release: https://pub.dev/packages/coinlib/versions/2.0.0-rc.8 Please test.

Nagalim commented 5 months ago

@sneurlax error2 (1) I am trying to cmake the .dll, but it seems to hang after performing the tests. I do not normally use dart or flutter or VS code or any of this stuff, so please forgive if it's something dumb, but I can't seem to get the .dll to appear.

sneurlax commented 5 months ago

@sneurlax error2 (1) I am trying to cmake the .dll, but it seems to hang after performing the tests. I do not normally use dart or flutter or VS code or any of this stuff, so please forgive if it's something dumb, but I can't seem to get the .dll to appear.

No need to be sorry, this is a valid issue I also see on my older Intel workstation. I don't know why the tests take so long to run prior to a build. My newer AMD CPU chugs through them quickly (just like on Linux). For now I can only recommend to just wait for the tests to complete

Edit: ah, I see the last message indicates test success, not that another test is running. Are you sure that's the last test? I have basically the same issue (long build time on old workstation), though, so it's not just you.

Nagalim commented 5 months ago

Same situation after waiting 8 hours, no change.

MatthewLM commented 5 months ago

@sneurlax @Nagalim My plan is to test the latest 0.4.1 release of secp256k1 on Linux and Android. If all is well, I'll update the commit hash for the Windows build and hopefully that resolves the issue for Windows too. Apparently some improves to the CMake builds have been introduced.

I'll keep the macOS and iOS builds on the older version for now and later I'll see if I can update the apple builds to no longer require a local copy of the secp256k1 source.

MatthewLM commented 5 months ago

@sneurlax @Nagalim I've published 2.0.0-rc.10. This includes secp256k1 0.4.1 which should hopefully resolve the issue. To test, the latest commit can be pulled and then flutter pub get can be run before trying dart run coinlib:build_windows again. If dart run coinlib:build_windows fails then the WSL build could be tried instead. If that works I can add a note to the README to suggest that Visual Studio builds sometimes hang and WSL should be tried instead.

sneurlax commented 5 months ago

On the faster AMD machine it builds fine. On the slower (older) Intel system, I proceed past @Nagalim's point and the first few tests quickly, but stall after Performing Test C_SUPPORTS__WD4267 - Success

PS C:\src\coinlib\coinlib_flutter\example> dart run coinlib:build_windows
Building package executable...
Built coinlib:build_windows.
-- The C compiler identification is MSVC 19.38.33133.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.38.33130/bin/Hostx64/x64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Performing Test HAVE_X86_64_ASM
-- Performing Test HAVE_X86_64_ASM - Failed
-- Could NOT find Valgrind (missing: Valgrind_INCLUDE_DIR Valgrind_WORKS)
-- Performing Test C_SUPPORTS__W3
-- Performing Test C_SUPPORTS__W3 - Success
-- Performing Test C_SUPPORTS__WD4146
-- Performing Test C_SUPPORTS__WD4146 - Success
-- Performing Test C_SUPPORTS__WD4244
-- Performing Test C_SUPPORTS__WD4244 - Success
-- Performing Test C_SUPPORTS__WD4267
-- Performing Test C_SUPPORTS__WD4267 - Success

in both PowerShell and "Developer PowerShell for VS". I don't actually think it's the architecture difference (AMD vs. Intel) causing issues, btw, but I'm not sure how to proceed. Altering the docs would probably be best. I wonder if tests could be skipped? (Probably can but shouldn't?)

MatthewLM commented 5 months ago

@sneurlax The CMake configuration tests are required to compile correctly. I could make a note in the README. Does the WSL build work on the older Intel system?

Nagalim commented 5 months ago

I have the same result as @sneurlax after the update. Completed more tests than before, but still not finishing.

MatthewLM commented 5 months ago

@Nagalim Thanks for testing it out. It would be good to see if the WSL builds work or not. If it works, adding a suggestion to do the WSL build in the README may be the a solution for now.

I'm not sure if building works for secp256k1 directly: https://github.com/bitcoin-core/secp256k1?tab=readme-ov-file#building-on-windows

If it fails it may be best to create an issue on that repository and hope it gets solved. If it succeeds then that is strange and I wonder why the dart code causes the build to fail. May there be a limitation of using a temporary directory for the build?

sneurlax commented 5 months ago

@sneurlax The CMake configuration tests are required to compile correctly. I could make a note in the README. Does the WSL build work on the older Intel system?

Yes, WSL builds work just fine. I'll have to test building bitcoin-core/secp256k1 natively on Windows later, I have Linux dev tasks to complete for now... but for whatever it's worth, I followed their instructions for this native Windows build process

MatthewLM commented 5 months ago

@sneurlax OK thank you. I'm not sure if something like running in a temp directory might affect things, so it might be worth trying to build secp256k1 according to instructions from the repo outside of the Dart script. If there is indeed a problem with that, one of us could make an issue on the bitcoin-core/secp256k1 repo. If the problem is specific to Dart then maybe alternations are needed.

sneurlax commented 5 months ago

bitcoin-core/secp256k1 built quickly

git clone https://github.com/bitcoin-core/secp256k1
cd secp256k1
cmake -G "Visual Studio 17 2022" -A x64 -S . -B build
cmake --build build --config RelWithDebInfo

and 0.4.1 built quickly on the same machine which stalls during the Dart build.

But the tests don't build at all... Strange situation

MatthewLM commented 5 months ago

@sneurlax OK, does the Dart build work if you use a local directory in the script instead of createTmpDir?

sneurlax commented 5 months ago

@sneurlax OK, does the Dart build work if you use a local directory in the script instead of createTmpDir?

No, using $workDir instead of $tmpDir didn't make any differences for me.

MatthewLM commented 5 months ago

@sneurlax OK, that's unfortunate. I don't know what the problem might be and I can't investigate further myself. For now it seems that a note should be added to the README to suggest using WSL. Even if it doesn't work flawlessly all the time, it shall be acceptable for now.

sneurlax commented 5 months ago

Sorry I couldn't make it perfect! Support through WSL is better than no support whatsoever, and some users may not have the stalling issue... Thanks for taking the PR anyhow. We've been using Coinlib on Windows for Stack Wallet for some time now! Now we just need to add Peercoin, eh? :P

MatthewLM commented 5 months ago

@sneurlax Many thanks again. The windows support is much appreciated. I've merged into v2.0 ready for the final 2.0 release. I'm hoping to have some feedback/review on the Taproot support before making 2.0 final.

Peercoin support in the Stack wallet would be great!