-
Notifications
You must be signed in to change notification settings - Fork 704
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
Fix 32-bit ARM build #754
Fix 32-bit ARM build #754
Conversation
librocksdb-sys/build.rs
Outdated
@@ -226,6 +226,11 @@ fn build_rocksdb() { | |||
config.define("ROCKSDB_IOURING_PRESENT", Some("1")); | |||
} | |||
|
|||
if std::env::var("CARGO_CFG_TARGET_POINTER_WIDTH").unwrap() != "64" { |
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.
I would propose to use matches!
macro here rather than unwrap
:
if std::env::var("CARGO_CFG_TARGET_POINTER_WIDTH").unwrap() != "64" { | |
if matches!(env::var("CARGO_CFG_TARGET_POINTER_WIDTH"), Ok(width) if width != "64") { |
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.
The other places also use unwrap
, so I thunk this would be more consistent
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.
I changed it to use env::var
though, I think this way it looks the cleanest. CARGO_CFG_TARGET_POINTER_WIDTH is guaranteed to be defined in build scripts, so IMO it's better to have it panic and let us know something went terribly wrong.
Building this crate on `arm-linux-gnueabihf` is fine, HOWEVER, opening a rocksdb database using the built code fails: We get a "Value too large for defined data type" in our functional tests, see [build output](https://build.bitcoinabc.org/repository/download/BitcoinABC_BitcoinAbcStaging/515561:id/artifacts.tar.gz!/functional/test_runner_%E2%82%BF%E2%82%B5_%F0%9F%8F%83_20230302_193244/chronik_block_214/node0/regtest/debug.log). After a bit of digging, it turns out this refers to a [missing environment variable specifically for 32-bit systems that want to open files using 64-bit offsets](https://stackoverflow.com/a/23372153). Therefore, this PR [fixes our build](https://build.bitcoinabc.org/viewLog.html?buildId=515991&buildTypeId=BitcoinABC_BitcoinAbcStaging&tab=buildResultsDiv&branch_BitcoinAbcDiffs=refs%2Ftags%2Fphabricator%2Fdiff%2F38261) by adding those two env variables when CARGO_CFG_TARGET_POINTER_WIDTH is not 64.
c1896c5
to
b64e9d5
Compare
We're currently blocking on this fix @aleksuss @stanislav-tkach, do you guys think it will be deployed soon? |
I will merge it into the master, but a new release with this fix will appear after two or three months, I guess. |
You can always release a minor version, like 0.20.2, or is there something preventing that? |
You can always use a dependency directly from the |
This reverts commit a6103ef.
This change seems to break windows x86 (i686) compile. |
It says 'unistd.h' not found for Reverting this commit fix compiling on windows i686. Moreover, it also runs correctly on win32. FYI: comparing these two Github Action Runs: https://github.com/Congyuwang/RocksDict/actions/runs/4476861228/jobs/7867755946 (reverted) |
Building this crate on
arm-linux-gnueabihf
is fine, HOWEVER, opening a rocksdb database using the built code fails:On our CI, we get a "Value too large for defined data type" in our functional tests, see build output (you can login as guest if necessary).
After a bit of digging, it turns out this refers to a missing environment variable specifically for 32-bit systems that want to open files using 64-bit offsets.
Therefore, this PR fixes our build by adding those two env variables when CARGO_CFG_TARGET_POINTER_WIDTH is not 64.