Skip to content

MOVEONLY: Move common init code to init/common #21732

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 23, 2021
Merged

MOVEONLY: Move common init code to init/common #21732

merged 6 commits into from
Apr 23, 2021

Conversation

ryanofsky
Copy link
Contributor

This PR is part of the process separation project.

This change is move-only and can be easily reviewed with --color-moved=dimmed_zebra. The moves are needed to avoid duplicating common init code between different binaries (bitcoin-node, bitcoin-wallet, etc) in #10102. In #10102, each binary has it's own init file (src/init/bitcoin-node.cpp, src/init/bitcoin-wallet.cpp) so this PR moves the common code to src/init/common.cpp.

@ryanofsky
Copy link
Contributor Author

Updated cade20c -> 66bbbc0 (pr/initc.1 -> pr/initc.2, compare) to fix hidden_args CI errors https://cirrus-ci.com/task/4601873798791168?logs=ci#L3751 https://cirrus-ci.com/task/5446298728923136?logs=ci#L2663 https://cirrus-ci.com/task/5516667473100800?logs=ci#L4299 https://cirrus-ci.com/task/6079617426522112?logs=ci#L2817 https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38776308

promag
promag reviewed Apr 19, 2021
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Code review ACK 66bbbc0.

Verified commit by commit, with --color-moved=dimmed-zebra. Moved code is tidy up in nice functions. I don't observe changes in behavior.

@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

This was referenced Apr 20, 2021
Closed
Closed
Closed
@practicalswift
Copy link
Contributor

Concept ACK

Nice with the init namespace too: it makes it easier to reason about encapsulation and more specifically it allows for finding all non-init call sites just by doing git grep "init::".

More generally I think it would make sense to gradually introduce more of this type of namespace use in our project.

@practicalswift practicalswift mentioned this pull request Apr 20, 2021
Merged
This was referenced Apr 20, 2021
Closed
Closed
kiminuo
kiminuo reviewed Apr 21, 2021
View reviewed changes
maflcko
maflcko approved these changes Apr 21, 2021
View reviewed changes
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.

Left some unrelated doc feedback to remove nonsense comments like

// Sanity checks
SanityChecks();

--color-moved=dimmed-zebra ACK 66bbbc0 😹

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

- --color-moved=dimmed-zebra ACK  66bbbc09e9bb44546ac47829384cd3b088c5efd7 😹
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjyPQwAltkH6kZvb6r7SNqiODJqjD0Ta96ZqC0rKjnSwMrLmObSuMu8OgWJ1zJW
c2q68lGd1PGZHvDiIrlJu6939yGby5c+hj84iCkgJoL720ZrkXVQynPjtkzcOcEc
vfa5VGH18547lkFyih1LYepSJ9MBnU+XH4WYZJMyeu75NVEc3OZTCj3xG1U519n4
NyAdTJ8WK9KLZF8rYRP0/sBgXBawZtjaS43YSrdz061Ht8kTlU/YqQinbRs4mm0K
EdRybSUXzDeFxGHCq2Kz6vuooSDasiTZUoVf66crNDN5XEIpmI2NgBDt8SjssQIg
RQQ9vNGoHDqM4b0lI+zWc9kkply/82ZL1TJP1HBYy/gdVh+Z5M2L3q9RVOt315EJ
VZnfl2fp2NNJoqcP/mLx2ZF4eZ5hNWJLiFw5bdDvmnbb9Gz3rS4lWg+0MrW3P6Bs
XSb+eDLPgbluTlH5Azkn/j2KtuZf5ODk57igT6hg4ZxVwrspkoXIFckgitA3ddXu
eN/gtak+
=4TTx
-----END PGP SIGNATURE-----

Timestamp of file with hash 27c6f524949cc5fa807c0a855609234ba5afe961ff88e1b28074374eea19342f -

@theStack
Copy link
Contributor

Concept ACK

ryanofsky
ryanofsky commented Apr 21, 2021
View reviewed changes
Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Updated 66bbbc0 -> 615965c (pr/initc.2 -> pr/initc.3, compare) with suggested changes

@maflcko
Copy link
Member

maflcko commented Apr 22, 2021

review ACK 615965c 🖱

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 615965cfd1ef1e0627d69970d99bdfedb9176833 🖱
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUj7CQv/Uj0PueOUes2yUiwCDP3lWhYevwJ0p5jaA85RtfTwsWnIAClUV0UmHAJh
cNpmWT73Dgg8Ku0jQbHO0cf9adIeKv8sbA3f+jQeStiz8FogOR6CWtgX4RzbLAWZ
W+x2Dq2ReHgToHggkQLWbEATGcZr2cQtub5NSjEwyfHnjIf8OvI7VOV7Ga8tsAWk
3oHSdjnWkfOcJLAkUZD9SF40fTjjPWFzI4KTwvp8SrQhL+uvuA4/3nk74idlCYE9
tYmWuiJ5MFocDOF5MVV/OtmWt+vfI7w2wbRYGu0N7LQU5lB9y2UgB8RzTmOCpKDz
qIrjsPdBNTiFcMOSYpJTE0s+Efuixi53nbHCWaJX2mgu7yzHZs/ktxw8NGeTUU2u
5nNs147u2YUaQ//aNcdbvRc7etvCT+do3N4zAYvNw9GNSHTjjPWT4uStX1qXMNnd
UqbUfMUGUTDjqgIcG8bpPX6WegkUIrCi7GZwbCLVf0FjXvDd4rWkxfS4Ijpy9rh/
/CfJrvyw
=XCNq
-----END PGP SIGNATURE-----

Timestamp of file with hash 460d9d89b06872cded3ec1cd1b35c0c9337114cc3b7071cebe084cad1768397b -

@practicalswift
Copy link
Contributor

cr ACK 615965c: dimmed zebra looks correct

@maflcko maflcko merged commit 66fd3b2 into bitcoin:master Apr 23, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 23, 2021
Merge bitcoin#21732: MOVEONLY: Move common init code to init/common
bdb105b
@ryanofsky ryanofsky mentioned this pull request Jun 16, 2021
Draft
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge 66fd3b2 into merged_master (Bitcoin PR bitcoin/bitcoin#21732)
0f00842
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@promag promag promag left review comments

@maflcko maflcko maflcko approved these changes

@kiminuo kiminuo kiminuo left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system Refactoring
Projects
Process Separation
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ryanofsky @DrahtBot @practicalswift @theStack @maflcko @promag @kiminuo