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.

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

Merged
merged 5 commits into from
Apr 11, 2024
Merged

Restore Windows support with MSVC #108

merged 5 commits into from
Apr 11, 2024

Conversation

conradoplg
Copy link
Contributor

Same as #38 but with MSVC. It's likely that we'll need to use MSVC since rocksdb only supports it.

@conradoplg conradoplg marked this pull request as draft October 18, 2023 15:21
@conradoplg
Copy link
Contributor Author

@conradoplg
Copy link
Contributor Author

conradoplg commented Nov 10, 2023

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:

  • Add a &* to a particular pointer usage in cache.h. It's basically this, so it sounds like something that should be fixed upstream.
  • Rename generated .c files to .cpp, because MSVC uses the file extension to choose the compiler.
  • Add amount.cpp, zip317.cpp and cache.cpp. No idea why that's not needed on Linux?
  • Comment a logging line in cache.cpp along with an include, which would otherwise require vendoring boost/filesystem

@conradoplg
Copy link
Contributor Author

  • Add a &* to a particular pointer usage in cache.h. It's basically this, so it sounds like something that should be fixed upstream.

Created zcash/zcash#6835

  • Add amount.cpp, zip317.cpp and cache.cpp. No idea why that's not needed on Linux?

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.

  • Comment a logging line in cache.cpp along with an include, which would otherwise require vendoring boost/filesystem

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.

@conradoplg conradoplg marked this pull request as ready for review March 1, 2024 16:39
Copy link
Contributor

@oxarbitrage oxarbitrage left a 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.

depend/zcash/src/zcash/cache.cpp Show resolved Hide resolved
depend/zcash/src/zcash/cache.cpp Show resolved Hide resolved
@mpguerra
Copy link

mpguerra commented Apr 4, 2024

We might want to do that in a follow up PR if we just want to get this working and released now

@mpguerra
Copy link

mpguerra commented Apr 4, 2024

@gustavovalverde once this merges I think we can re-enable all windows tests again

@oxarbitrage oxarbitrage merged commit cf9e0d2 into master Apr 11, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants