fix: Windows replace use of non-ascii filename chars #1017
Conversation
If I understand rust-rocksdb/rust-rocksdb#319 correctly, then shouldn't RocksDB already be built with this flag? |
|
It would have been, but this recent PR removed it rust-rocksdb/rust-rocksdb#493 but the main rocksdb still uses that flag https://github.com/facebook/rocksdb/blob/d89483098ff4a77bd0ef12454a64554805984cc5/port/win/port_win.cc |
|
Ah, I see. That's weird 🤔 |
| } | ||
|
|
||
| return { | ||
| path: `${userDataPath}${sep}${WALLET_STORAGE_DIRECTORY}${sep}${profileDirName}`, |
There was a problem hiding this comment.
Do we need to do this for the userDataPath too? For example if the path is C:\Users\üsername\AppData\Roaming\Firefly
There was a problem hiding this comment.
Oh yeah, actually that means this PR won't work as we don't have control over the replacing the characters in a user name, it has to be fixed with the RocksDB version :(
There was a problem hiding this comment.
Ah that's annoying. I've opened rust-rocksdb/rust-rocksdb#512 to see if there was a reason that flag was removed
There was a problem hiding this comment.
Although was is also strange is it looks like the wallet is using v0.15 https://github.com/iotaledger/wallet.rs/blob/b68f181604824db86da0a4bef9ad6cb11f3e9949/Cargo.toml#L29
which should have included the fix https://github.com/rust-rocksdb/rust-rocksdb/blob/b91299ca2e2278cd5ed0db35f5687fb5e37118cd/librocksdb-sys/build.rs#L137
There was a problem hiding this comment.
I wonder if the issue is in wallet.rs or stronghold then 🤔
There was a problem hiding this comment.
I traced through the code and its definitely in RocksDB its failing, specifically this line https://github.com/facebook/rocksdb/blob/cc1c3ee54eace876ad18c39f931e8e5039823930/port/win/env_win.cc#L1179
I am going to build a version locally to see if I can get it to work with the correct options.
There was a problem hiding this comment.
Managed to build a version 0.16.0 with the ROCKSDB_WINDOWS_UTF8_FILENAMES flag set, built wallet.rs to reference, and then used the new version with FF, and it works 🎉
Although there was some C++ code in rocksdb that wouldn't compile with the flag set, so had to update it 😅
|
Closing this as it will be fixed with an updated wallet.rs PR |
Description of change
On windows profile names with non ascii characters caused an exception e.g.
useräThis is because the windows version of RocksDB does not support the wider character set facebook/rocksdb#3408 and so the profile storage folders which use the profile names were not accessible.
On windows the non ASCII characters will now be replaced with their sequence numbers instead for folder names e.g.
userä=user00e4An alternative fix is to do a custom build of the rust RocksDB using this flag facebook/rocksdb#4469 which enables UTF8 support for windows filenames.
Links to any relevant issues
Fixes #1015 #992
Type of change
How the change has been tested
Tested on windows with profile names with non ascii characters.
Change checklist