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
  1. sinetek commented at 1:04 am on August 22, 2022: contributor
    Following 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.
  2. sinetek renamed this:
    clarify that the patch targets WSL1.
    clarify that fs IsWSL patch targets WSL1.
    on Aug 22, 2022
  3. NorrinRadd commented at 2:46 am on August 22, 2022: none
    LGTM
  4. hebasto commented at 6:50 am on August 22, 2022: member
    Are there any limitations why a Windows user cannot upgrade their WSL1 to WSL2?
  5. 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.

  6. hebasto commented at 3:09 pm on August 22, 2022: member
    Why not just drop support for WSL1 then?
  7. 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

  8. 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 command uname -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?

    @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

    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.

  9. fanquake commented at 3:48 pm on August 24, 2022: member
    I agree that we could probably just remove support for WSL1 here.
  10. sinetek commented at 1:14 am on August 26, 2022: contributor
    updated.
  11. unknown approved
  12. 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)

  13. 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.

  14. 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
  15. 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.

  16. luke-jr commented at 10:41 pm on August 26, 2022: member
    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.
  17. unknown approved
  19. 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

  20. maflcko commented at 9:44 am on September 9, 2022: member
    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.
  21. fs: drop old WSL1 hack. 5669afb80e
  22. 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.

  23. unknown approved
  25. 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.

  26. 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/?

  27. fanquake approved
  28. fanquake commented at 2:54 pm on February 6, 2023: member
    ACK 5669afb80e350027705dc378d2ab16232567511a - seems ok as-is.
  29. fanquake renamed this:
    Remove old fs IsWSL patch that targets WSL1. (Please use WSL2)
    fs: remove WSL 1 workaround
    on Feb 6, 2023
  30. maflcko renamed this:
    fs: remove WSL 1 workaround
    util: remove WSL 1 workaround in fs
    on Feb 6, 2023
  31. DrahtBot added the label Utils/log/libs on Feb 6, 2023
  32. fanquake merged this on Feb 16, 2023
  33. fanquake closed this on Feb 16, 2023
  34. sidhujag referenced this in commit 14ce44f24c on Feb 16, 2023
  35. bitcoin locked this on Feb 16, 2024


sinetek NorrinRadd hebasto ghost fanquake luke-jr maflcko DrahtBot

Labels
Utils/log/libs

github-metadata-mirror

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
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me