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

Build windows DLLs from vendor directory #45

Merged
merged 19 commits into from
Jun 12, 2024
Merged

Conversation

bhartnett
Copy link
Contributor

@bhartnett bhartnett commented Jun 6, 2024

This PR completes the following issue: #44

Changes included in the PR:

  • Added vcpkg to the vendor directory to support building specific RocksDB version for windows DLLs. Currently version v9.1.0.
  • Added both .bat and shell scripts for building windows DLLs.
  • Updated the Github CI to use the new method of buliding DLLs for windows.

@bhartnett bhartnett marked this pull request as ready for review June 7, 2024 05:34
@bhartnett bhartnett requested a review from arnetheduck June 7, 2024 05:52
@arnetheduck
Copy link
Member

I'm guessing getting rid of vcpkg here is difficult?

also, since we have a specific version pinned here, we should remove https://github.com/status-im/nim-rocksdb/tree/master/rocksdb/lib/headers and instead use the headers from the submodule (and make sure we can easily regenerate them - https://github.com/arnetheduck/nim-sqlite3-abi/blob/master/update.sh has a script for this purpose for example

@bhartnett
Copy link
Contributor Author

bhartnett commented Jun 7, 2024

I'm guessing getting rid of vcpkg here is difficult?

also, since we have a specific version pinned here, we should remove https://github.com/status-im/nim-rocksdb/tree/master/rocksdb/lib/headers and instead use the headers from the submodule (and make sure we can easily regenerate them - https://github.com/arnetheduck/nim-sqlite3-abi/blob/master/update.sh has a script for this purpose for example

Yes, vcpkg is the easiest way to build RocksDB on windows and this is the way cheatfate did it as well. Doing it manually using cmake would take more time. Adding vcpkg to your project as a submodule is recommended to pin the version of the tool being built.

The RocksDB project provides a combined header file here https://github.com/facebook/rocksdb/blob/main/include/rocksdb/c.h which I guess we could use but the difficult part is that there were some manual modifications at the top of the file at and below '#ifdef C2NIM'. We would probably need to copy the header file into the project and then modify it on the fly then generate the wrapper using c2nim. I'm not sure exactly why the modifications are required.

@bhartnett
Copy link
Contributor Author

@arnetheduck Even the librocksdb.nim wrapper has some manual modifications which makes it hard to automate regenerating it. My question would be do you think its worth spending the time on it now or should I just create an issue so we can come back to it?

@arnetheduck
Copy link
Member

I'm not sure exactly why the modifications are required.

I suspect it's because of an old c2nim version being used and/or to get the correct uintXX types - newer c2nim versions should hopefully not need this - that said, sqlite faces a similar problem and does the "adjustments" as part of its update script, see https://github.com/arnetheduck/nim-sqlite3-abi/blob/9e9adf6b70b79b0c48d08a4e81c629561936d40b/update.sh#L34 - it would be great to get this working for rocksdb as well so we always match header to vendor and can bump vendor at low cost

@arnetheduck
Copy link
Member

can you bump nimbus-eth1 as well to use this dll? then we know the full pipeline works with the rest of the build system

@bhartnett
Copy link
Contributor Author

I'm not sure exactly why the modifications are required.

I suspect it's because of an old c2nim version being used and/or to get the correct uintXX types - newer c2nim versions should hopefully not need this - that said, sqlite faces a similar problem and does the "adjustments" as part of its update script, see https://github.com/arnetheduck/nim-sqlite3-abi/blob/9e9adf6b70b79b0c48d08a4e81c629561936d40b/update.sh#L34 - it would be great to get this working for rocksdb as well so we always match header to vendor and can bump vendor at low cost

Ok, I'll try updating the header file and look at which parts can be automated. By the way we won't be able to automatically update the vcpkg rocksdb version which is used for the windows build because that github project doesn't have RocksDb specific git tags that we can point to. The update process would be as follows:

  • Pull the latest version of vcpkg submodule. If it contains a new version of RockDb then continue the rest of the steps.
  • Update the rocksdb submodule to the same version as used in vcpkg.
  • Copy the header file from vender/rocksdb include directory into the project.
  • Regenerate the nim wrapper.

@bhartnett
Copy link
Contributor Author

can you bump nimbus-eth1 as well to use this dll? then we know the full pipeline works with the rest of the build system

Yes, will do.

@bhartnett
Copy link
Contributor Author

Merging this for now. Additional improvements can go in another PR.

@bhartnett bhartnett merged commit a84cf5b into master Jun 12, 2024
9 checks passed
@bhartnett bhartnett deleted the build-windows-dlls branch June 12, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants