-
Notifications
You must be signed in to change notification settings - Fork 11
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
Restore Windows support with MSVC #108
Conversation
One of the errors seems related to this https://stackoverflow.com/questions/72907548/expected-c-compiler-error-in-yvals-core-h |
3d17c33
to
cfc745d
Compare
I managed to get it to work, pending CI. I had to add some ugly workarounds (i.e. comment out some code) in our copy of zcashd that would be get wiped in the next dependency upgrade, so next time I look into this I'll try to sort them out (either manage to remove the workaround, or submit upstream). In particular, the linking errors are weird. I had to add some files to be compiled in build.rs, but why is it working on Linux then? I'll also investigate this. Summary of the changes:
|
Created zcash/zcash#6835
I looked into this a bit but couldn't find a good explanation quickly. It seems easier to just ignore it. My hunch is that is something related to differences between Linux and Windows linking. Maybe these unresolved symbols in Windows are not actually being used in the final binary, and in Linux it's smart enough to detect that and ignore them.
In the end I left the manual patch and committed a patch file and instructions on how to patch the zcash source when it's updated. |
591b79d
to
f785f09
Compare
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.
It seems to be working and looks good to me. I was wondering if we can just remove the commented lines in cache.cpp and in the patch.
We might want to do that in a follow up PR if we just want to get this working and released now |
@gustavovalverde once this merges I think we can re-enable all windows tests again |
Same as #38 but with MSVC. It's likely that we'll need to use MSVC since rocksdb only supports it.