Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: Remove legacy MSVC build system #117

Merged
merged 3 commits into from
Apr 6, 2024
Merged

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Mar 9, 2024

At this stage, the new CMake-based build system offers a comprehensive set of features, surpassing those of the legacy build system located in the build_msvc directory.

As part of the transition to the new build system, the legacy MSVC build system has been removed.

Additionally, a new "Windows / MSVC Build Guide" has been introduced, based on https://gist.github.com/hebasto/32c77c0cd592d13685437271ac6b79f6. Notably, the guide lacks information regarding the disabling of base address randomization (see bitcoin#21045). Even with -DHARDENING=OFF, this feature remains enabled by default.

@hebasto
Copy link
Owner Author

hebasto commented Mar 9, 2024

Friendly ping @sipsorcery @pablomartin4btc @m3dwards @TheCharlatan @vasild :)

Also cc @EthanHeilman regarding the base address randomization feature.

@hebasto hebasto added the enhancement New feature or request label Mar 9, 2024
@EthanHeilman
Copy link

--disable-hardening doesn't work anymore? Is this just a windows think or is it broken on linux as well?

I ran into issues with this on OSX:

To disable PIE in bitcoind on OSX run:
make clean
make all LDFLAGS="$LDFLAGS -Wl,-no_pie" 

to check it worked (compare imgs below):
otool -hv src/bitcoind  

@hebasto
Copy link
Owner Author

hebasto commented Mar 15, 2024

--disable-hardening doesn't work anymore? Is this just a windows think or is it broken on linux as well?

I ran into issues with this on OSX:

To disable PIE in bitcoind on OSX run:
make clean
make all LDFLAGS="$LDFLAGS -Wl,-no_pie" 

to check it worked (compare imgs below):
otool -hv src/bitcoind  

On the master branch, while --disable-hardening means "do not attempt to harden the resulting executables", which implies it does not force non-hardening preprocessor macros, compiler and linker flags.

The new CMake-based build system preserves that behaviour.

@EthanHeilman
Copy link

On the master branch, while --disable-hardening means "do not attempt to harden the resulting executables", which implies it does not force non-hardening preprocessor macros, compiler and linker flags.
The new CMake-based build system preserves that behaviour.

So if we wanted the ability to disable windows base address randomization we would need to add it as a CMake flag?

@hebasto
Copy link
Owner Author

hebasto commented Mar 16, 2024

On the master branch, while --disable-hardening means "do not attempt to harden the resulting executables", which implies it does not force non-hardening preprocessor macros, compiler and linker flags.
The new CMake-based build system preserves that behaviour.

So if we wanted the ability to disable windows base address randomization we would need to add it as a CMake flag?

Yes. Something like that:

cmake -B build --preset vs2022 -DHARDENING=OFF -DCMAKE_EXE_LINKER_FLAGS="/DYNAMICBASE:NO"

I'm still curios about the use cases for binaries (1) with disabled ASLR and (2) with disabled PIE. Is it worth considering adding dedicated build configuration options for such cases?

@EthanHeilman
Copy link

I'm still curios about the use cases for binaries (1) with disabled ASLR and (2) with disabled PIE.

My main usecase is that bitcoin uses process memory as a source for randomness for it's RNG. In measuring and testing the randomnesss provided by the RNG, I turn all this off to make it is easier to isolate randomness problems. This can also be useful when attempting to replicate heisenbugs. That's a very boutique use case though,

Is it worth considering adding dedicated build configuration options for such cases?

I'll probably add them back in in a PR when restarting the RNG measurement project. It would save me some time if someone else readded them, but given that as far as I am aware I am the primary user of this feature it seems a little unfair to ask someone else to do the work.

@hebasto
Copy link
Owner Author

hebasto commented Mar 26, 2024

Rebased.

Amend "displayName" fields.
@hebasto
Copy link
Owner Author

hebasto commented Apr 3, 2024

Rebased.

@sipsorcery
Copy link

utACK (winx64) 9c0bf9b.

cmake -S .. --preset vs2022-static # It might take a while if the vcpkg binary cache is unpopulated or invalidated
cmake --build . --config Release
ctest --build-config Release
cmake --install . --config Release # Optional
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why install is listed as part of the static linking build and not the dynamic linking build?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For dynamic linking build, dependency DLLs must be located in the same folder as a binary. That is not implemented for now.

One can skip vcpkg manifest default features to speedup the configuration step.
For example, the following invocation will skip all features except for "wallet" and "tests" and their dependencies:
```
cmake -S .. --preset vs2022 -DVCPKG_MANIFEST_NO_DEFAULT_FEATURES=ON -DVCPKG_MANIFEST_FEATURES="wallet;tests"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to explain or list what the other manifest features are?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The features are listed in the <root>\vcpkg.json file. What wording would you suggest to list them in the docs?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Available features are listed in the <root>\vcpkg.json file.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Added.

@m3dwards
Copy link

m3dwards commented Apr 4, 2024

Should the guide mention ccache?

What about only building release versions of dependencies with:

Add-Content -Path "$env:VCPKG_ROOT\triplets\x64-windows.cmake" -Value "set(VCPKG_BUILD_TYPE release)"
Add-Content -Path "$env:VCPKG_ROOT\triplets\${{ matrix.conf.triplet }}.cmake" -Value "set(VCPKG_BUILD_TYPE release)"

@hebasto
Copy link
Owner Author

hebasto commented Apr 4, 2024

Should the guide mention ccache?

It is mentioned in https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md. No other platform-specific build doc mentions it explicitly.

What about only building release versions of dependencies with:

Add-Content -Path "$env:VCPKG_ROOT\triplets\x64-windows.cmake" -Value "set(VCPKG_BUILD_TYPE release)"
Add-Content -Path "$env:VCPKG_ROOT\triplets\${{ matrix.conf.triplet }}.cmake" -Value "set(VCPKG_BUILD_TYPE release)"

We use this optimization for the CI to speed up the build. However, on the user's machine this vcpkg patching could break other software builds.

@m3dwards
Copy link

m3dwards commented Apr 4, 2024

I've found that when I change configuration I have to delete the build dir and start again with a fresh one. I assume this is normal and if it is, should this be mentioned?

@hebasto
Copy link
Owner Author

hebasto commented Apr 4, 2024

I've found that when I change configuration I have to delete the build dir and start again with a fresh one. I assume this is normal and if it is, should this be mentioned?

These words confuse me a bit, as the MSVC generator is multi-configuration. Could you be more specific please?

@m3dwards
Copy link

m3dwards commented Apr 4, 2024

If I run:

mkdir build
cd build
cmake -S .. --preset vs2022-static
cmake -S .. --preset vs2022 -DVCPKG_MANIFEST_NO_DEFAULT_FEATURES=ON -DVCPKG_MANIFEST_FEATURES="wallet;tests"

I get:

-- Configuring incomplete, errors occurred!

So moving between static and dynamic presets for example.

If we think that's it's perfectly obvious that this would be expected then no probs. I did expect it as it's common with autotools / make when running the configure step again without a distclean.

@m3dwards
Copy link

m3dwards commented Apr 4, 2024

ACK 8d3bb2b

Tested with PowerShell on Windows 11 but I want to give the caveat that these steps were not run on a clean install of windows. I do intend to wipe and reinstall my windows box soon and so will go through the steps again.

@hebasto
Copy link
Owner Author

hebasto commented Apr 5, 2024

If I run:

mkdir build
cd build
cmake -S .. --preset vs2022-static
cmake -S .. --preset vs2022 -DVCPKG_MANIFEST_NO_DEFAULT_FEATURES=ON -DVCPKG_MANIFEST_FEATURES="wallet;tests"

I get:

-- Configuring incomplete, errors occurred!

So moving between static and dynamic presets for example.

If we think that's it's perfectly obvious that this would be expected then no probs. I did expect it as it's common with autotools / make when running the configure step again without a distclean.

Right. When using CMake 3.24 and newer, which is usually the case on Windows, the --fresh command-line option is available as an alternative.

@hebasto hebasto merged commit 8bcdf2a into cmake-staging Apr 6, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants