Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Aug 15, 2024

This PR deletes the Autotools-based build system.

The MSVC build system is deleted in #30731.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 15, 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
Concept ACK Sjors
Stale ACK vasild, maflcko

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:

  • #30635 (rpc: add optional blockhash to waitfornewblock by Sjors)
  • #30570 ([tests] New fuzz target wallet_rpc by kevkevinpal)
  • #30510 (multiprocess: Add IPC wrapper for Mining interface by ryanofsky)
  • #30509 (multiprocess: Add -ipcbind option to bitcoin-node by ryanofsky)
  • #30437 (multiprocess: add bitcoin-mine test program by ryanofsky)
  • #30239 (Ephemeral Dust by instagibbs)
  • #30110 (refactor: TxDownloadManager + fuzzing by glozow)
  • #30079 (Fee Estimation: Ignore all transactions that are CPFP'd by ismaelsadeeq)
  • #30075 (test, subprocess: Improve coverage report correctness by hebasto)
  • #29936 (fuzz: wallet: add target for CreateTransaction by brunoerg)
  • #29532 (Refactor BnB tests by murchandamus)
  • #29450 (build: replace custom MAC_OSX macro with existing __APPLE__ by l0rinc)
  • #29432 (Stratum v2 Template Provider (take 3) by Sjors)
  • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
  • #29409 (multiprocess: Add capnp wrapper for Chain interface by ryanofsky)
  • #29158 (PoC: fuzz chainstate and block managers by darosior)
  • #29039 (versionbits refactoring by ajtowns)
  • #28939 (memusage: let PoolResource keep track of all allocated/deallocated memory by martinus)
  • #28676 ([WIP] Cluster mempool implementation by sdaftuar)
  • #28574 (wallet: optimize migration process, batch db transactions by furszy)
  • #28031 (Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling by glozow)
  • #27837 (net: introduce block tracker to retry to download blocks after failure by furszy)
  • #27331 (refactor: extract CCheckQueue's data handling into a separate container "Bag" by martinus)
  • #26966 (index: initial sync speedup, parallelize process by furszy)
  • #26693 (build: special instruction check script by willcl-ark)
  • #26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)
  • #26334 (Add Signet launch shortcut for Windows by Sjors)
  • #26022 (Add util::ResultPtr class by ryanofsky)
  • #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)
  • #24539 (Add a "tx output spender" index by sstone)
  • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
  • #21283 (Implement BIP 370 PSBTv2 by achow101)

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 hebasto force-pushed the 240815-kill-autotools branch from 6e768be to 9c4cf4f Compare August 15, 2024 20:15
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Debug: https://github.com/bitcoin/bitcoin/runs/28830258425

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

This was referenced Aug 16, 2024
@@ -3,154 +3,21 @@
!/build-aux
!/build_msvc

*.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still not sure how I feel about removing all of these. We are going to switch a lot between the versions with and without cmake over the next two years. Is it really desirable to have all these files appear in the untracked list?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you suggest?

I'm still not sure how I feel about removing all of these. We are going to switch a lot between the versions with and without cmake over the next two years. Is it really desirable to have all these files appear in the untracked list?

From my personal experience during recent months, it isn't a problem at all.

Copy link
Contributor

@TheCharlatan TheCharlatan Aug 30, 2024

Choose a reason for hiding this comment

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

When I review a backport and come back again, this is how it looks like:

> git status
On branch master
Untracked files:
  (use "git add <file>..." to include in what will be committed)
	Makefile
	Makefile.in
	aclocal.m4
	autom4te.cache/
... another 420 lines

This makes using git status and a bunch of other git commands very annoying.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is generally unsupported to switch to old branches back-and-forth and expect the gitignore to function. Does make clean && make distclean before switching branches help? Alternatively, do out-of-tree autotools builds help to avoid the untracked files?

@hebasto hebasto force-pushed the 240815-kill-autotools branch from 6811a2b to faa382a Compare August 30, 2024 20:31
@DrahtBot
Copy link
Contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File commit 96b0a8f
(master)
commit 0f8c6a9
(master and this pull)
SHA256SUMS.part 27365d9d56b05cce... 784c3e2b804e07e7...
*-aarch64-linux-gnu-debug.tar.gz 04a5bb232114f782... c2133e8ddf8e0ff5...
*-aarch64-linux-gnu.tar.gz bd5d23a64ab7d779... c05c3d1da815228e...
*-arm-linux-gnueabihf-debug.tar.gz 0021ca776f7dcc8b... 8b1b558d4827116f...
*-arm-linux-gnueabihf.tar.gz 2340d1119a1ec632... 1cb0c6b13bd34139...
*-arm64-apple-darwin-unsigned.tar.gz ab00b80f00cd09b8... 40140f7e82682c13...
*-arm64-apple-darwin-unsigned.zip cb84320f60e26fed... 44aa0706622b8040...
*-arm64-apple-darwin.tar.gz a1a2786cfcecd350... 36894d0906c06caf...
*-powerpc64-linux-gnu-debug.tar.gz 9544adef5060dfab... 28eea1c218761abc...
*-powerpc64-linux-gnu.tar.gz faf390f38f51cc68... 3d3412d5ac36b43f...
*-riscv64-linux-gnu-debug.tar.gz 67b921646742efc8... fd9f661bbc433058...
*-riscv64-linux-gnu.tar.gz c4759b761c171f92... 46330a9dca896a01...
*-x86_64-apple-darwin-unsigned.tar.gz 459ba0d98d89d678... f9a861066bb1d832...
*-x86_64-apple-darwin-unsigned.zip d19b0e188ac78eb6... 1fb2374c3fcbe325...
*-x86_64-apple-darwin.tar.gz 7b3f41e90375e361... e74a163674a27fe1...
*-x86_64-linux-gnu-debug.tar.gz a1c953af5210e0d6... 545a5a9e20fdd2cd...
*-x86_64-linux-gnu.tar.gz 5d74858c83c440f2... d179bb44acebc7cf...
*.tar.gz b3128d336e5c9680... fd4ba27bf0577127...
guix_build.log 3e54d141b4a84788... 042744d2a0c05f33...
guix_build.log.diff 70d0ef7493c2b7a0...

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

Only change since last review is addressing feedback (removing a script and gitignore changes).

The gitignore still seem inconsistent, but I don't care much either way personally.

re-ACK faa382a 🍦

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK faa382ae7642da0e436ea2c7f7eac67386280a7e 🍦
Pa+7MqVHY/VDhJZ9Bdbr49ElHmQ1m0FcmiE8YrnUkMTICFE3Iy6QvS8NHDfZWYXhVTSI8h8SiPcuVU/h+1urAQ==

.deps
.dirstamp
.libs
.*.swp
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this? Seem unrelated to autotools.

.*.swp
*~
*.bak
*.rej
Copy link
Member

Choose a reason for hiding this comment

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

Same, etc

contrib/devtools/split-debug.sh

# Output from running db4 installation
db4/
Copy link
Member

Choose a reason for hiding this comment

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

Note: This is a follow-up to the install_db4.sh removal, but seems fine to do here.

Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

ACK faa382a

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK faa382a

The gitignore still seem inconsistent,

Especially given the comments in #30731 (comment). Seems those should have been removed for the same reasoning as #30664 (comment). Changes here seem completely arbitrary.

@fanquake fanquake merged commit ef6f49e into bitcoin:master Sep 2, 2024
16 checks passed
@maflcko
Copy link
Member

maflcko commented Sep 2, 2024

My latest review comments are on lines that are unrelated to switching branches. It is about temporary files in the source dir. Such files were created before cmake and will be created after cmake, because developers editing the source files will create them.

Personally I don't use gitignore, so I don't care, but it would be good to explain why the source-editing temporary-files changes are made and how they relate to autotools/cmake at all. Otherwise, there will be more follow-ups and confusion in the future.

@hebasto hebasto deleted the 240815-kill-autotools branch September 2, 2024 10:55
hebasto pushed a commit that referenced this pull request Sep 5, 2024
After the recent full removal of Autotools (PR #30664), these
macros are not needed anymore in the .cpp files according to the
TODO in qt's CMakeLists.txt. Tested building on OpenBSD 7.5, where
the XCB plugin was still imported according to the debug log.
hebasto added a commit that referenced this pull request Sep 5, 2024
…IN` macro calls

7346b01 qt, build: remove unneeded `Q_IMPORT_PLUGIN` macro calls (Sebastian Falbesoner)

Pull request description:

  After the recent full removal of Autotools (PR [#30664](#30664)), these macros are not needed anymore in the .cpp files according to the TODO in qt's CMakeLists.txt. Tested building on OpenBSD 7.5, where the XCB plugin was still imported according to the debug log:
  ```
  2024-09-02T21:13:27Z Bitcoin Core version v28.99.0-7346b0109208 (release build)
  2024-09-02T21:13:27Z Qt 5.15.12 (dynamic), plugin=xcb
  2024-09-02T21:13:27Z No static plugins.
  2024-09-02T21:13:27Z Style: fusion / QFusionStyle
  2024-09-02T21:13:27Z System: OpenBSD 7.5, x86_64-little_endian-lp64
  ```

ACKs for top commit:
  hebasto:
    ACK 7346b01.

Tree-SHA512: ffa033fc6e0412a99d2c167044cc7af89512a731172d6911db71434f5353e811802c268d853a76d3732e9da954444cf6c39a852aeb25938c435826e117a16fca
theStack added a commit to theStack/bitcoin that referenced this pull request Sep 12, 2024
Updates the .editorconfig file, first introduced in 2021
(see PR bitcoin#21123, commit 7a135d5) w.r.t. following changes:
- consider Rust .rs files (relevant since bitcoin#29076, commit bbbbdb0)
- reflect build system change to CMake (bitcoin#30454, bitcoin#30664)
- add setting for bare Makefile still used for depends builds

Can be tested e.g. by using the editorconfig-vim plugin
(https://github.com/editorconfig/editorconfig-vim).
theStack added a commit to theStack/bitcoin that referenced this pull request Sep 12, 2024
Updates the .editorconfig file, first introduced in 2021
(see PR bitcoin#21123, commit 7a135d5) w.r.t. following changes:
- consider Rust .rs files (relevant since bitcoin#28076, commit bbbbdb0)
- reflect build system change to CMake (bitcoin#30454, bitcoin#30664)
- add setting for the bare Makefile still used for depends builds

Can be tested e.g. by using the editorconfig-vim plugin
(https://github.com/editorconfig/editorconfig-vim).
theStack added a commit to theStack/bitcoin that referenced this pull request Sep 13, 2024
Updates the .editorconfig file, first introduced in 2021
(see PR bitcoin#21123, commit 7a135d5) w.r.t. following changes:
- consider Rust .rs files (relevant since bitcoin#28076, commit bbbbdb0)
- reflect build system change to CMake (bitcoin#30454, bitcoin#30664)
- add setting for the bare Makefile still used for depends builds

Can be tested e.g. by using the editorconfig-vim plugin
(https://github.com/editorconfig/editorconfig-vim).
fanquake added a commit that referenced this pull request Sep 13, 2024
9556061 code style: update .editorconfig file (Sebastian Falbesoner)

Pull request description:

  Updates the .editorconfig file, first introduced in 2021 (see PR #21123, commit 7a135d5) w.r.t. following changes:
  - consider Rust .rs files (relevant since #28076, commit bbbbdb0)
  - reflect build system change to CMake (#30454, #30664)
  - add setting for bare Makefile still used for depends builds

  Can be tested e.g. by using the editorconfig-vim plugin (https://github.com/editorconfig/editorconfig-vim). The PR is made under the assumption that the file is still considered useful, especially for new contributors. If people feel like that's not the case anymore, the alternative is to delete it, obviously.

Top commit has no ACKs.

Tree-SHA512: 8406b1caf31e310f7e17c607d97beac583481e71b4425e0be2bbd8207096aa374a70151b58aae5fdb648ef5ff5c7e1d0a2949e6de3355bdd2009d8353ee24af0
josibake pushed a commit to josibake/bitcoin that referenced this pull request Sep 17, 2024
Updates the .editorconfig file, first introduced in 2021
(see PR bitcoin#21123, commit 7a135d5) w.r.t. following changes:
- consider Rust .rs files (relevant since bitcoin#28076, commit bbbbdb0)
- reflect build system change to CMake (bitcoin#30454, bitcoin#30664)
- add setting for the bare Makefile still used for depends builds

Can be tested e.g. by using the editorconfig-vim plugin
(https://github.com/editorconfig/editorconfig-vim).
josibake pushed a commit to josibake/bitcoin that referenced this pull request Sep 17, 2024
Updates the .editorconfig file, first introduced in 2021
(see PR bitcoin#21123, commit 7a135d5) w.r.t. following changes:
- consider Rust .rs files (relevant since bitcoin#28076, commit bbbbdb0)
- reflect build system change to CMake (bitcoin#30454, bitcoin#30664)
- add setting for the bare Makefile still used for depends builds

Can be tested e.g. by using the editorconfig-vim plugin
(https://github.com/editorconfig/editorconfig-vim).
josibake pushed a commit to josibake/bitcoin that referenced this pull request Sep 17, 2024
Updates the .editorconfig file, first introduced in 2021
(see PR bitcoin#21123, commit 7a135d5) w.r.t. following changes:
- consider Rust .rs files (relevant since bitcoin#28076, commit bbbbdb0)
- reflect build system change to CMake (bitcoin#30454, bitcoin#30664)
- add setting for the bare Makefile still used for depends builds

Can be tested e.g. by using the editorconfig-vim plugin
(https://github.com/editorconfig/editorconfig-vim).
fanquake added a commit that referenced this pull request Sep 20, 2024
7025942 build: drop superfluous `HAVE_BUILD_INFO` define (Sebastian Falbesoner)
0dd6625 build: drop obj/ subdir for generated build.h, rename to bitcoin-build-info.h (Sebastian Falbesoner)

Pull request description:

  As indicated by the TODO, the obj subdirectory is not needed anymore now for the generated build.h header, since autotools are gone and we don't have in-source builds anymore (see #30454, #30664). In the second commit the superflous `HAVE_BUILD_INFO` macro is dropped, as suggested in #30856 (review).

ACKs for top commit:
  theuni:
    utACK 7025942

Tree-SHA512: 0a3b2cbbcf638344ceb74e5ba5a0fe2b1718427b23a18c8890258db36ce7177006a146178ed88d9c5ae956a5426f3844e86c1f4cca7c40946359742bffda983b
@bitcoin bitcoin deleted a comment from Rsed19901 Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: CMake follow-ups
Development

Successfully merging this pull request may close these issues.

7 participants