util: remove WSL 1 workaround in fs #25898
pull sinetek wants to merge 1 commits into bitcoin:master from sinetek:master changing 1 files +8 −24-
sinetek commented at 1:04 am on August 22, 2022: contributorFollowing discussion, the WSL1 patch will be removed, as WSL1 is no longer being developed by Microsoft. Instead, please upgrade to a mainstream WSL2 version. More information can be found on the official website.
-
sinetek renamed this:
clarify that the patch targets WSL1.
clarify that fs IsWSL patch targets WSL1.
on Aug 22, 2022 -
NorrinRadd commented at 2:46 am on August 22, 2022: noneLGTM
-
hebasto commented at 6:50 am on August 22, 2022: memberAre there any limitations why a Windows user cannot upgrade their WSL1 to WSL2?
-
sinetek commented at 3:08 pm on August 22, 2022: contributor
Are there any limitations why a Windows user cannot upgrade their WSL1 to WSL2?
hi @hebasto. As far as I know WSL1 is no longer being developed. Documentation indicates that WSL1 is slightly advantageous in some cases where access to the native filesystem is required, and WSL2 might interact with other virtualization solutions. I’m not particularly attached to this hack, however it is safe to assume that it will never be fixed on MS’s side.
-
hebasto commented at 3:09 pm on August 22, 2022: memberWhy not just drop support for WSL1 then?
-
ghost commented at 2:19 pm on August 23, 2022: none
I think this is reasonable to specify, as I myself spent a few minutes looking why the call IsWSL() was failing on my WSL environment. @sinetek how can I test this?
Why not just drop support for WSL1 then? @hebasto
Everyone would not have upgraded to WSL2 yet and there are some trade-offs according to the docs: https://docs.microsoft.com/en-us/windows/wsl/compare-versions#exceptions-for-using-wsl-1-rather-than-wsl-2
-
sinetek commented at 2:41 pm on August 23, 2022: contributor
I think this is reasonable to specify, as I myself spent a few minutes looking why the call IsWSL() was failing on my WSL environment.
@sinetek how can I test this?
Run the app under GDB and set a breakpoint on this function. Alternatively you can see the result of
utsname.version
with the commanduname -v
. The result on my box is#1 SMP Wed Mar 2 00:30:59 UTC 2022
.Why not just drop support for WSL1 then?
Everyone would not have upgraded to WSL2 yet and there are some trade-offs according to the docs: https://docs.microsoft.com/en-us/windows/wsl/compare-versions#exceptions-for-using-wsl-1-rather-than-wsl-2
I tend to agree. On the other hand I can understand why the removal is warranted, there might be some other unseen bugs in WSL1 and building a complete syscall emulation of Linux is beyond the scope of this project.
-
fanquake commented at 3:48 pm on August 24, 2022: memberI agree that we could probably just remove support for WSL1 here.
-
sinetek commented at 1:14 am on August 26, 2022: contributorupdated.
-
unknown approved
-
unknown commented at 1:23 am on August 26, 2022: none
ACK https://github.com/bitcoin/bitcoin/pull/25898/commits/7566cd63c8ec6e1009f18697c8076d5c9380f239
nit: can fix lint error (whitespace)
-
fanquake commented at 6:19 am on August 26, 2022: member
nit: can fix lint error (whitespace)
That isn’t a nit. It needs fixing before merge.
The PR title and description need to be updated to reflect the new changes.
-
sinetek renamed this:
clarify that fs IsWSL patch targets WSL1.
Remove old fs IsWSL patch that targets WSL1. (Please use WSL2)
on Aug 26, 2022 -
sinetek commented at 3:54 pm on August 26, 2022: contributor
nit: can fix lint error (whitespace)
That isn’t a nit. It needs fixing before merge.
The PR title and description need to be updated to reflect the new changes.
That’s a cute way to enforce spaces. Updated.
-
luke-jr commented at 10:41 pm on August 26, 2022: memberConsidering IBD bottleneck is often I/O, and Microsoft’s documentation says WSL1 is better for that, I think someone should do some benchmarks before we drop WSL1 support.
-
unknown approved
-
unknown commented at 10:41 pm on August 26, 2022: none
-
ghost commented at 0:22 am on August 27, 2022: none
Considering IBD bottleneck is often I/O, and Microsoft’s documentation says WSL1 is better for that, I think someone should do some benchmarks before we drop WSL1 support.
WSL2 is better if files (data directory) is on WSL linux and WSL1 is better if files (data directory) is on Windows host.
I tried syncing bitcoind (signet) for 5 minutes with default dbcache and txindex enabled. WSL2 was better and synced 20% compared to 14% in WSL1.
Memory usage is a concern in WSL2 however there is a workaround suggested in this issue: https://github.com/microsoft/WSL/issues/4166#issuecomment-526725261
-
maflcko commented at 9:44 am on September 9, 2022: memberPlease revert to the previous commit, we usually do not allow merge commits in the commit history unless there is a reason to use them.
-
fs: drop old WSL1 hack. 5669afb80e
-
sinetek commented at 8:09 pm on September 9, 2022: contributor
Please revert to the previous commit, we usually do not allow merge commits in the commit history unless there is a reason to use them.
Thanks for alerting me. That was not intentional and fixed.
-
unknown approved
-
unknown commented at 8:13 pm on September 9, 2022: none
-
DrahtBot commented at 9:31 am on September 23, 2022: contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviews
See the guideline for information on the review process.
Type Reviewers ACK 1440000bytes, fanquake If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
No conflicts as of last run.
-
hebasto commented at 9:23 pm on November 30, 2022: member
Is reverting of the original e8fa0a3d2025509fcddc59fc618e91371542cf87 commit cleaner?
Maybe it’s worth updating
doc/build-windows.md
: https://github.com/bitcoin/bitcoin/blob/e2bfd41f832dc7c7be6f17e928352f0eb2865f66/doc/build-windows.md#L9 as well, I mean, s/WSL/WSL 2/? -
fanquake approved
-
fanquake commented at 2:54 pm on February 6, 2023: memberACK 5669afb80e350027705dc378d2ab16232567511a - seems ok as-is.
-
fanquake renamed this:
Remove old fs IsWSL patch that targets WSL1. (Please use WSL2)
fs: remove WSL 1 workaround
on Feb 6, 2023 -
maflcko renamed this:
fs: remove WSL 1 workaround
util: remove WSL 1 workaround in fs
on Feb 6, 2023 -
DrahtBot added the label Utils/log/libs on Feb 6, 2023
-
fanquake merged this on Feb 16, 2023
-
fanquake closed this on Feb 16, 2023
-
sidhujag referenced this in commit 14ce44f24c on Feb 16, 2023
-
bitcoin locked this on Feb 16, 2024
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 03:12 UTC
More mirrored repositories can be found on mirror.b10c.me