Skip to content

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Aug 6, 2024

This PR effectively reverts commit 4f890ba from #11437, which fixed some build issues on WSL seven years ago.

Testing the current master branch @ 31a3ff5 on Windows 11 + WSL using Ubuntu 24.04 or Debian images, I noticed that the workaround is no longer required. Moreover, it doesn't affect the build process at all, which means the hashes of the built packages in depends remain the same and the configure log in the main build system remains the same as well.

This change effectively reverts 4f890ba.
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 6, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK sipsorcery

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #30454 (build: Introduce CMake-based build system by hebasto)

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.

@hebasto
Copy link
Member Author

hebasto commented Aug 6, 2024

cc @sipsorcery @fanquake @m3dwards

@sipsorcery
Copy link
Contributor

Testing the current master branch @ 31a3ff5 on Windows 11 + WSL using Ubuntu 24.04 or Debian images, I noticed that the workaround is no longer required.

The command was originally added because some people had Windows environment variables that when carried across to WSL caused errors with the configure script.

I suspect that condition still exists. Maybe it's safer to leave the command there until the configure step is removed by cmake?

@hebasto
Copy link
Member Author

hebasto commented Aug 6, 2024

I suspect that condition still exists. Maybe it's safer to leave the command there until the configure step is removed by cmake?

I would be great to document such a condition more explicitly.

However, I'm okay with either outcome.

@hebasto
Copy link
Member Author

hebasto commented Aug 6, 2024

Maybe it's safer to leave the command there until the configure step is removed by cmake?

I've read #10269 again, and it seems all complaints were about building a native package in depends, which is no longer the case.

@sipsorcery
Copy link
Contributor

I've read #10269 again, and it seems all complaints were about building a native package in depends, which is no longer the case.

That does ring a bell.

utACK 16d8261.

@fanquake
Copy link
Member

fanquake commented Aug 6, 2024

were about building a native package in depends, which is no longer the case.

Qt still builds a native package ?

@hebasto
Copy link
Member Author

hebasto commented Aug 6, 2024

were about building a native package in depends, which is no longer the case.

Qt still builds a native package ?

Right. It's a qmake itself. I meant the native_* packages that are handled by the depends build subsystem in its own way.

@fanquake fanquake merged commit c831c9a into bitcoin:master Aug 12, 2024
@hebasto hebasto deleted the 240806-wsl-note branch August 12, 2024 10:26
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 27, 2024
16d8261 doc: Drop no longer needed workaround for WSL (Hennadii Stepanov)

Pull request description:

  This PR effectively reverts commit 4f890ba from bitcoin#11437, which fixed some build issues on WSL seven years ago.

  Testing the current master branch @ 31a3ff5 on Windows 11 + WSL using Ubuntu 24.04 or Debian images, I noticed that the workaround is no longer required. Moreover, it doesn't affect the build process at all, which means the hashes of the built packages in depends remain the same and the `configure` log in the main build system remains the same as well.

ACKs for top commit:
  sipsorcery:
    utACK 16d8261.

Tree-SHA512: 703a2ac4647125c91aad47131e2723fd05af30b0cfae5677a26d3e89a77c2779e0197584208f3b378ed64dd7305512d9064fd073ec06517f86e9905af4ec8838
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 27, 2024
b654479 Merge bitcoin#30705: test: Avoid intermittent block download timeout in p2p_ibd_stalling (merge-script)
745a819 Merge bitcoin#30690: devtools, utxo-snapshot: Fix block height out of range in script (Ava Chow)
01b570e Merge bitcoin#29999: guix: fix suggested fake date for openssl-1.1.1l (Ava Chow)
432f352 Merge bitcoin#30580: doc: Add note about distro's `g++-mingw-w64-x86-64-posix` version (merge-script)
1bd090e Merge bitcoin#30597: doc: Drop no longer needed workaround for WSL (merge-script)
8a12237 Merge bitcoin#30630: doc: Update ccache website link (merge-script)
f66547f Merge bitcoin#30588: depends: fix ZMQ CMake getcachesize check (merge-script)
ddaec96 Merge bitcoin#30565: depends: Fix `zeromq` build on OpenBSD (merge-script)
e4e5605 Merge bitcoin#30552: test: fix constructor of msg_tx (merge-script)
df3c239 Merge bitcoin#26950: cleanse: switch to SecureZeroMemory for Windows cross-compile (merge-script)
57945ce Merge bitcoin#30506: depends: Cleanup postprocess commands after switching to CMake (merge-script)
e016ffa Merge bitcoin#29878: depends: build expat with CMake (merge-script)
62dcd43 Merge bitcoin#29880: depends: build FreeType with CMake (merge-script)
745addf Merge bitcoin#30245: net: Allow -proxy=[::1] on nodes with IPV6 lo only (Ava Chow)
4e144be Merge bitcoin-core/gui#795: Keep focus on "Hide" while ModalOverlay is visible (Hennadii Stepanov)
69c04b2 Merge bitcoin#30372: util: Use SteadyClock in RandAddSeedPerfmon (merge-script)
ebed8af Merge bitcoin#30336: depends: update doc in Qt pwd patch (merge-script)
9793fb1 Merge bitcoin#30340: test: Added coverage to Block not found error using gettxoutsetinfo (Ava Chow)
479cb8b Merge bitcoin#30312: contrib: add R(UN)PATH check to ELF symbol-check (merge-script)
ca83773 Merge bitcoin#30283: upnp: fix build with miniupnpc 2.2.8 (merge-script)
63e139d Merge bitcoin#30185: guix: show `*_FLAGS` variables in pre-build output (merge-script)
3be0d3e Merge bitcoin#30097: crypto: disable asan for sha256_sse4 with clang and -O0 (merge-script)
3070c3e Merge bitcoin#30078: depends: set AR & RANLIB for CMake (merge-script)

Pull request description:

  ## Issue being fixed or feature implemented
  Trivial backports

  ## What was done?

  ## How Has This Been Tested?
  built locally

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK b654479
  kwvg:
    utACK b654479

Tree-SHA512: 10b5af4e92c83fa9d6764b20bf066bba8e4c600402966fd5c1d6dad07b0549d8a42151a33f21e2f8263336c12a810a6f3fc2828d90bc98153e09c165d9e5b043
@bitcoin bitcoin locked and limited conversation to collaborators Aug 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants