Re-enable windows path tests disabled by #20744 #24251
Conversation
This should also fix an assert error if a -datadir with a trailing slash is used on windows. This appears to be a real error and regression introduced with bitcoin#20744. On windows (or at least wine), fs calls that actuallly access the filesystem like fs::equivalent or fs::exists seem to treat directory paths with trailing slashes as not existing, so it's necessary to normalize these paths before using them. This fix adds a path::lexically_normal() call to the failing assert so it passes.
…itcoin#20744 This should also fix an init error if a -walletdir with a trailing slash is used on windows. This appears to be a real error and regression introduced with bitcoin#20744. On windows (or at least wine), fs calls that actuallly access the filesystem like fs::equivalent or fs::exists seem to treat directory paths with trailing slashes as not existing, so it's necessary to normalize these paths before using them. This change passes canonical paths to fs calls validating the -walletdir path to fix this.
|
Updated 8287489 -> d216bc8 ( I think #20744 might have actually caused real regressions on windows. If Not good to disable tests like this! |
You're talking about Windows builds only, right? |
You left off the important part where I said "... in these two test cases." And I actually only know about wine, not real windows. With wine, I saw fs::exists and similar functions treat directory paths with trailing dots and slashes as if they did not exist. Normalizing or canonicalizing the arguments made these functions work again and let me re-enable these windows tests. I think adding back test coverage here, and making minor changes to make code more reliable are already good things to do, and would have improved #20744. But it also hints that #20744 could also break some real windows configurations. I don't know for sure. |
|
Shouldn't this be named "wallet: Use canonical path in VerifyWallets"? |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
…#20744 d216bc8 Re-enable walletinit_verify_walletdir_no_trailing2 test disabled in bitcoin#20744 (Ryan Ofsky) 80cd64e Re-enable util_datadir check disabled in bitcoin#20744 (Ryan Ofsky) Pull request description: Reenable some broken tests as discussed bitcoin#20744 (comment) and bitcoin#20744 (comment) Fix windows test cases broken in bitcoin#20744, by passing normalized path arguments to fs::equivalent, fs::exists, and fs::is_directory, instead of non-normalized arguments. Also re-enable the tests. It is possible these changes also fix real init behavior on windows when -datadir or -walletdir paths with trailing dots or dashes are used, but it's not clear because I only tested on wine. ACKs for top commit: hebasto: ACK d216bc8, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 2099ddfa1a3ad70f7ac2ff413929414a1851d257b280da25c0f5cefb46fd1372b580a1f1ee5280681a1c16e6031f119185cadd4f7a6121298562cf001f711068
Reenable some broken tests as discussed #20744 (comment) and #20744 (comment)
Fix windows test cases broken in #20744, by passing normalized path arguments to fs::equivalent, fs::exists, and fs::is_directory, instead of non-normalized arguments. Also re-enable the tests.
It is possible these changes also fix real init behavior on windows when -datadir or -walletdir paths with trailing dots or dashes are used, but it's not clear because I only tested on wine.