refactor: Cleanup clientversion.cpp

hebasto commented at 8:42 am on April 13, 2020: member

This PR:

removes unused macros and duplicated code
renames macros in a way, that makes the code self-descriptive.

fanquake added the label Refactoring on Apr 13, 2020

DrahtBot commented at 4:40 pm on April 13, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18902 (Bugfix: Only use git for build info if the repository is actually the right one by luke-jr)

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.

DrahtBot added the label Needs rebase on Apr 28, 2020

Drop unused CLIENT_VERSION_SUFFIX macro 1e06bb68be

hebasto force-pushed on Apr 28, 2020

hebasto commented at 8:20 pm on April 28, 2020: member

Rebased 3335ab4b857c96999b390f92a9fe62a42d827f82 -> 098d26d0cc00cb1bca005a0cd289f4cae753c67b (pr18616.01 -> pr18616.02) due to the conflict with #18556.

hebasto commented at 8:20 pm on April 28, 2020: member

@dongcarl Mind looking into this?

DrahtBot removed the label Needs rebase on Apr 28, 2020

MarcoFalke commented at 3:10 pm on April 29, 2020: member

Concept ACK

MarcoFalke closed this on Apr 29, 2020

MarcoFalke reopened this on Apr 29, 2020

brakmic commented at 4:06 pm on May 1, 2020: contributor

ACK 098d26d0cc00cb1bca005a0cd289f4cae753c67b

Built, run and tested on macOS Catalina 10.15.4

info_window

dongcarl commented at 7:59 pm on May 1, 2020: member

068bcc017c695a450e132f060b936800954637ab

Perhaps let’s separate the pure-rename changes into another commit from the other changes? (I think the changes to share/genbuild.sh are the only pure-rename ones). Also, is the DO_STRINGIZE a mistake from before? Why is it not needed anymore?

098d26d0cc00cb1bca005a0cd289f4cae753c67b

Not quite sure how this mechanism works here, and wasn’t able to find a reference to it in the git-achive(1) man page, could you point me in the right direction?

hebasto commented at 8:34 pm on May 1, 2020: member

@dongcarl

098d26d

Not quite sure how this mechanism works here, and wasn’t able to find a reference to it in the git-achive(1) man page, could you point me in the right direction?

a) we set the export-subst attribute for src/clientversion.cpp in the .gitattributes file: https://github.com/bitcoin/bitcoin/blob/844d2070a2c0106bb7a54be5cad7d4da4d9cd55e/.gitattributes#L1

From the git docs:

If the attribute export-subst is set for a file then Git will expand several placeholders when adding this file to an archive. The expansion depends on the availability of a commit ID, i.e., if git archive has been given a tree instead of a commit or a tag then no replacement will be done. The placeholders are the same as those for the option --pretty=format: of git log, except that they need to be wrapped like this: $Format:PLACEHOLDERS$ in the file. E.g. the string $Format:%H$ will be replaced by the commit hash.

b) $Format:%n#define GIT_COMMIT_ID "%H"$ has two placeholders:

%n, which is expanded to a newline that makes a new uncommented LOC just after the comment that states the same thing :)

0//! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.

%H, which is expanded to a commit hash

hebasto commented at 8:49 pm on May 1, 2020: member

@dongcarl

Also, is the DO_STRINGIZE a mistake from before? Why is it not needed anymore?

It is still required: https://github.com/bitcoin/bitcoin/blob/844d2070a2c0106bb7a54be5cad7d4da4d9cd55e/src/clientversion.h#L17-L22

hebasto force-pushed on May 1, 2020

scripted-diff: Rename share/genbuild.sh macros to more meaningful ones

-BEGIN VERIFY SCRIPT-
sed -i 's/\bDESC\b/GIT_TAG/' share/genbuild.sh
sed -i 's/\bSUFFIX\b/GIT_COMMIT/g' share/genbuild.sh
-END VERIFY SCRIPT-

dc1fba9389

build: Rename BUILD_* macros and the code self-descriptive 35f1189ea7

hebasto force-pushed on May 1, 2020

hebasto commented at 10:05 pm on May 1, 2020: member

Updated 098d26d0cc00cb1bca005a0cd289f4cae753c67b -> 09ee167a6dd7b2d4ade828affc95a679acfaf6c6 (pr18616.02 -> pr18616.05, diff):

the pure-rename changes to share/genbuild.sh are separated into their own commit (see @dongcarl’s comment) @dongcarl Thank you for reviewing. All your comments have been addressed.

hebasto force-pushed on May 1, 2020

dongcarl commented at 4:08 pm on May 4, 2020: member

Code Review ACK 09ee167a6dd7b2d4ade828affc95a679acfaf6c6

Nit: personally, something like this in clientversion.cpp made it a bit more readable for me without sacrificing the DRY-ness:

 0#ifdef BUILD_GIT_TAG
 1    #define BUILD_DESC BUILD_GIT_TAG
 2#else
 3    #define BUILD_BASE_DESC "v" STRINGIZE(CLIENT_VERSION_MAJOR) "." STRINGIZE(CLIENT_VERSION_MINOR) \
 4                            "." STRINGIZE(CLIENT_VERSION_REVISION) "." STRINGIZE(CLIENT_VERSION_BUILD)
 5    #ifdef BUILD_GIT_COMMIT
 6        #define BUILD_DESC BUILD_BASE_DESC "-" BUILD_GIT_COMMIT
 7    #elif defined(GIT_COMMIT_ID)
 8        #define BUILD_DESC BUILD_BASE_DESC "-g" GIT_COMMIT_ID
 9    #else
10        #define BUILD_DESC BUILD_BASE_DESC "-unk"
11    #endif
12#endif
13
14const std::string CLIENT_BUILD(BUILD_DESC);

refactor: Remove duplicated code 8f9f4ba5e2

Drop unused GIT_COMMIT_DATE macro c269e618cf

hebasto force-pushed on May 4, 2020

hebasto commented at 4:56 pm on May 4, 2020: member

Updated 09ee167a6dd7b2d4ade828affc95a679acfaf6c6 -> c269e618cf53634d3cf270273ab1b0354dc3c119 (pr18616.05 -> pr18616.06, diff):

addressed @dongcarl’s comment

dongcarl commented at 7:08 pm on May 5, 2020: member

FYI: I tested a branch with both this and my changes in #18741, builds passed fine!

hebasto commented at 11:18 pm on May 5, 2020: member

@dongcarl

FYI: I tested a branch with both this and my changes in #18741, builds passed fine!

Does it mean “re-ACK” from you? :)

dongcarl commented at 2:21 pm on May 12, 2020: member

Yup! ACK c269e618cf53634d3cf270273ab1b0354dc3c119

laanwj merged this on May 13, 2020

laanwj closed this on May 13, 2020

hebasto deleted the branch on May 13, 2020

sidhujag referenced this in commit e5b50c3ff6 on May 14, 2020

Fabcien referenced this in commit 67d04b398c on Jan 28, 2021

PastaPastaPasta referenced this in commit 78338bba78 on Dec 22, 2021

PastaPastaPasta referenced this in commit 3644ea0494 on Dec 22, 2021

PastaPastaPasta referenced this in commit 5a138cee84 on Dec 22, 2021

PastaPastaPasta referenced this in commit 9263b1c27d on Dec 28, 2021

DrahtBot locked this on Feb 15, 2022

refactor: Cleanup clientversion.cpp #18616

Conflicts

068bcc017c695a450e132f060b936800954637ab

098d26d0cc00cb1bca005a0cd289f4cae753c67b

098d26d