-
Notifications
You must be signed in to change notification settings - Fork 13
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
WIP: Windows Support for v2.0 #25
Conversation
* flutter create --platforms=windows . I did not make any changes to these files, they were just generated using the command above. * generated plugin registrants autogenerated changes, no manual edits * add script to cross-compile secp256k1 for windows 77227a * include secp256k1 DLL in coinlib_flutter projects confirmed working with `flutter run -d windows` in `example` * ignore coinlib/src folder do not check secp256k1 submodule into git * document Windows builds * add tip about optionally skipping flutter installation in wsl2 instance * be more specific in docs re: building on windows without wsl2 * use dockerfile for native-ubuntu builds, use old approach only for wsl and update docs and docs formatting fix dockerfile * move depenedencies from script to docs * replace processSync with execWithStdio * move DLL to build/secp256k1.dll IAW lib/src/secp256k1/secp256k1_io.dart * remove unnecessary autogenerated files * use tmp dir to avoid polluting project dir * docs update and formatting * formatting remove double newlines and debugging print * remove unneeded gitignore line * correct relative path to secp256k1.dll * add ffiPlugin property to windows * correct relative path to secp256k1.dll * do not add unfruitful linux build of secp256k1 on windows TODO re-enable. TODO make CMake work on Windows natively * correct relative path to secp256k1.dll previous fixes fixed the example app but broke consumer apps. this version works for both the example app and consumers of this plugin the same way. the example just needs to get its secp256k1 built for it, too (but that makes sense) * bump version to rc 6 to denote windows support * bump example app pubspec bump version to rc 6 to denote windows support * place secp256k1.dll in build/windows * create build/windows folder if it doesn't exist * don't add extra windows folder in secp256k1 dll path it's included in the cmake var force pushing to fix typo * add a native windows build script and document its use * move dll output from build/windows to build in accordance with coinlib convention * minor docs update mention windows support and point to coinlib docs for lib-building guide * mention Visual Studio 17 2022 dependency * typofix: linux->windows in docs * clarify Windows library-building re: #24 (comment) * reduce example app version number to 1.0.0: partial reversion of 9573a05
Ah yes, there seems to be an undocumented dependency for Resolve via Sorry about that! |
@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 Thanks again. |
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... |
I confirmed
I tested with your DLL, it works just fine 👍 just had to remove the |
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, |
Are you able to find a solution in the CMakeLists.txt file such as using I'll delay publishing a test package until this is solved. |
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. |
OK, this issue should be resolved by #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. |
* use find_file to find secp256k1.dll because in testing we found that CMAKE_BINARY_DIR is usually app/build/windows/x64, but some Windows hosts use app/build/windows. This should support both force pushed to fix a small typo. * remove explicit references to secp256k1.dll * formatting limit lines to 180 characters, align comments
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. |
@sneurlax I've published a new release: https://pub.dev/packages/coinlib/versions/2.0.0-rc.8 Please test. |
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.
Tested working!
@sneurlax |
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. |
Same situation after waiting 8 hours, no change. |
@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. |
Doesn't include local source for macOS and iOS on flutter
@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 |
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
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?) |
@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? |
I have the same result as @sneurlax after the update. Completed more tests than before, but still not finishing. |
@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? |
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 |
@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. |
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 |
@sneurlax OK, does the Dart build work if you use a local directory in the script instead of |
No, using $workDir instead of $tmpDir didn't make any differences for me. |
@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. |
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 |
@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! |
@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 3752552) but it still fails at this step:
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.