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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32397.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

That's too bad, we'd finally solved all the build-with-spaces issues with the old build system.

But yes i suppose it doesn't hurt too add a warning in the meantime...

from a directory with spaces in its path, CMake issues the following warning:

Warning: Paths with embedded space may be handled incorrectly by configure

This is a vcpkg upstream issue. Another one has already been documented:

This is not specific to CMake in general, nor to the Bitcoin Core build system.

@hebasto Thanks for your feedback! I fixed the wording in the commit message to mention vcpkg instead of CMake. I agree that this isn't Bitcoin Cores fault, but IMO it would still be "nice" for new devs to have a warning in place. Might save them a minor headache.

This is not specific to CMake in general, nor to the Bitcoin Core build system.

Happy about that. i know it was something we explicitly considered and tested when going to CMake so hence my surprise.

In general, Windows users are the most likely to have spaces in any path they're allowed to write to. For example, from that iconv vcpkg issue:

That would require me to reinstall my entire operating system (Windows 11 setup asks you for your name and then sets the user folder name to that, including spaces). Not sure if that is something that should be accounted for within vcpkg or within Windows itself

So hope this will get resolved upstream sooner than this documentation change would make it into a release.

@laanwj I don't think that the time of the next release is necessarily the important thing here. This is documentation about how to build bitcoin core, which I assume is often done with the master of this repo.

It's not clear what is failing to build? If it is libiconv, how is that related to our build; is this something we can just avoid? In either case, please adjust the hint to be explcit about spaces being unsupported, and remove vaugeries like "they have been known to cause build issues.". Either spaces are supported, in which case we can test the behaviour, or they are not supported.

It's not clear what is failing to build? If it is libiconv, how is that related to our build; is this something we can just avoid?

libiconv is listed as a direct dependency of the qt package (see the output of vcpkg depend-info qt).

According to the vcpkg docs, the default install directory (${CMAKE_BINARY_DIR}\vcpkg_installed) can be overridden by setting the VCPKG_INSTALLED_DIR variable to a path that contains no spaces. This resolves the issue.

What is the status of this? Do we need to add something to the docs?

What is the status of this? Do we need to add something to the docs?

I believe it would be helpful to mention a workaround in the build documentation. Here's a suggestion you could consider including.

Feel free to grab it.

cc @davidgumberg

It would be useful to document, but not in the way it is here. It's not a problem when cloning the repository. The current warning is confusing, it should be more specific (also to make it more clear when the problem is solved and the warning can be removed again).

I believe it would be helpful to mention a workaround in the build documentation. Here's a suggestion you could consider including.

+1

Closing for now. @hebasto maybe you can followup with your own suggestion.

Closing for now. @hebasto maybe you can followup with your own suggestion.

Sure. #32858.