refactor: Cleanup clientversion.cpp #18616

hebasto · 2020-04-13T08:42:29Z

This PR:

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

DrahtBot · 2020-04-13T16:40:07Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Bugfix: Only use git for build info if the repository is actually the right one #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.

hebasto · 2020-04-28T20:20:20Z

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

hebasto · 2020-04-28T20:20:54Z

@dongcarl Mind looking into this?

maflcko · 2020-04-29T15:10:58Z

Concept ACK

brakmic · 2020-05-01T16:06:44Z

ACK 098d26d

Built, run and tested on macOS Catalina 10.15.4

dongcarl · 2020-05-01T19:59:51Z

`068bcc0`

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?

`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?

hebasto · 2020-05-01T20:34:17Z

@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:

bitcoin/.gitattributes

Line 1 in 844d207

src/clientversion.cpp export-subst

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 :)

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

%H, which is expanded to a commit hash

hebasto · 2020-05-01T20:49:34Z

@dongcarl

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

It is still required:

bitcoin/src/clientversion.h

Lines 17 to 22 in 844d207

    
           /** 
        
            * Converts the parameter X to a string after macro replacement on X has been performed. 
        
            * Don't merge these into one macro! 
        
            */ 
        
           #define STRINGIZE(X) DO_STRINGIZE(X) 
        
           #define DO_STRINGIZE(X) #X

-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-

hebasto · 2020-05-01T22:05:58Z

Updated 098d26d -> 09ee167 (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.

dongcarl · 2020-05-04T16:08:32Z

Code Review ACK 09ee167

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

#ifdef BUILD_GIT_TAG
    #define BUILD_DESC BUILD_GIT_TAG
#else
    #define BUILD_BASE_DESC "v" STRINGIZE(CLIENT_VERSION_MAJOR) "." STRINGIZE(CLIENT_VERSION_MINOR) \
                            "." STRINGIZE(CLIENT_VERSION_REVISION) "." STRINGIZE(CLIENT_VERSION_BUILD)
    #ifdef BUILD_GIT_COMMIT
        #define BUILD_DESC BUILD_BASE_DESC "-" BUILD_GIT_COMMIT
    #elif defined(GIT_COMMIT_ID)
        #define BUILD_DESC BUILD_BASE_DESC "-g" GIT_COMMIT_ID
    #else
        #define BUILD_DESC BUILD_BASE_DESC "-unk"
    #endif
#endif

const std::string CLIENT_BUILD(BUILD_DESC);

hebasto · 2020-05-04T16:56:54Z

Updated 09ee167 -> c269e61 (pr18616.05 -> pr18616.06, diff):

addressed @dongcarl's comment

dongcarl · 2020-05-05T19:08:01Z

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

hebasto · 2020-05-05T23:18:44Z

@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 · 2020-05-12T14:21:08Z

Yup! ACK c269e61

c269e61 Drop unused GIT_COMMIT_DATE macro (Hennadii Stepanov) 8f9f4ba refactor: Remove duplicated code (Hennadii Stepanov) 35f1189 build: Rename BUILD_* macros and the code self-descriptive (Hennadii Stepanov) dc1fba9 scripted-diff: Rename share/genbuild.sh macros to more meaningful ones (Hennadii Stepanov) 1e06bb6 Drop unused CLIENT_VERSION_SUFFIX macro (Hennadii Stepanov) Pull request description: This PR: - removes unused macros and duplicated code - renames macros in a way, that makes the code self-descriptive. ACKs for top commit: dongcarl: Yup! ACK c269e61 Tree-SHA512: c469f6269b578ccfae33d960e317eca8efaf27d49638f4c3830948c11b12ef728494d7e18c31e4a410945b7d83af5b246c7b83661b4eca17cf41ee4c4583649b

Summary: > Drop unused CLIENT_VERSION_SUFFIX macro > 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- > build: Rename BUILD_* macros and the code self-descriptive > refactor: Remove duplicated code > Drop unused GIT_COMMIT_DATE macro This is a backport of Core [[bitcoin/bitcoin#18616 | PR18616]] Test Plan: ``` grep -r CLIENT_VERSION_SUFFIX src/ grep -r GIT_COMMIT_DATE src/ mkdir build cd build cmake .. -GNinja ninja all check-all ``` Testing the version number: `cmake .. -GNinja && ninja && src/bitcoind -h | grep version` It should display something like `v0.22.13-ba26a24e0` or `v0.22.13-ba26a24e0-dirty` (dirty tree) Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D9099

c269e61 Drop unused GIT_COMMIT_DATE macro (Hennadii Stepanov) 8f9f4ba refactor: Remove duplicated code (Hennadii Stepanov) 35f1189 build: Rename BUILD_* macros and the code self-descriptive (Hennadii Stepanov) dc1fba9 scripted-diff: Rename share/genbuild.sh macros to more meaningful ones (Hennadii Stepanov) 1e06bb6 Drop unused CLIENT_VERSION_SUFFIX macro (Hennadii Stepanov) Pull request description: This PR: - removes unused macros and duplicated code - renames macros in a way, that makes the code self-descriptive. ACKs for top commit: dongcarl: Yup! ACK c269e61 Tree-SHA512: c469f6269b578ccfae33d960e317eca8efaf27d49638f4c3830948c11b12ef728494d7e18c31e4a410945b7d83af5b246c7b83661b4eca17cf41ee4c4583649b

fanquake added the Refactoring label Apr 13, 2020

DrahtBot mentioned this pull request Apr 13, 2020

build: Drop make dist in gitian builds #18556

Merged

DrahtBot mentioned this pull request Apr 22, 2020

guix: Make source tarball using git-archive #18741

Merged

DrahtBot added the Needs rebase label Apr 28, 2020

Drop unused CLIENT_VERSION_SUFFIX macro

1e06bb6

hebasto force-pushed the 20200413-version branch from 3335ab4 to 098d26d Compare April 28, 2020 20:16

DrahtBot removed the Needs rebase label Apr 28, 2020

maflcko closed this Apr 29, 2020

maflcko reopened this Apr 29, 2020

hebasto force-pushed the 20200413-version branch from 098d26d to 7a89e9f Compare May 1, 2020 21:54

hebasto added 2 commits May 2, 2020 00:59

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

dc1fba9

-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-

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

35f1189

hebasto force-pushed the 20200413-version branch from 7a89e9f to d88195c Compare May 1, 2020 22:00

hebasto force-pushed the 20200413-version branch from d88195c to 09ee167 Compare May 1, 2020 22:08

hebasto added 2 commits May 4, 2020 19:53

refactor: Remove duplicated code

8f9f4ba

Drop unused GIT_COMMIT_DATE macro

c269e61

hebasto force-pushed the 20200413-version branch from 09ee167 to c269e61 Compare May 4, 2020 16:54

DrahtBot mentioned this pull request May 7, 2020

Bugfix: Only use git for build info if the repository is actually the right one #18902

Closed

laanwj merged commit fc895d7 into bitcoin:master May 13, 2020

hebasto deleted the 20200413-version branch May 13, 2020 18:18

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Cleanup clientversion.cpp #18616

refactor: Cleanup clientversion.cpp #18616

Uh oh!

hebasto commented Apr 13, 2020

Uh oh!

DrahtBot commented Apr 13, 2020 •

edited

Loading

Uh oh!

hebasto commented Apr 28, 2020

Uh oh!

hebasto commented Apr 28, 2020

Uh oh!

maflcko commented Apr 29, 2020

Uh oh!

brakmic commented May 1, 2020

Uh oh!

dongcarl commented May 1, 2020

Uh oh!

hebasto commented May 1, 2020

098d26d

Uh oh!

hebasto commented May 1, 2020

Uh oh!

hebasto commented May 1, 2020 •

edited

Loading

Uh oh!

dongcarl commented May 4, 2020

Uh oh!

hebasto commented May 4, 2020

Uh oh!

dongcarl commented May 5, 2020

Uh oh!

hebasto commented May 5, 2020 •

edited

Loading

Uh oh!

dongcarl commented May 12, 2020

Uh oh!

Uh oh!

refactor: Cleanup clientversion.cpp #18616

refactor: Cleanup clientversion.cpp #18616

Uh oh!

Conversation

hebasto commented Apr 13, 2020

Uh oh!

DrahtBot commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Apr 28, 2020

Uh oh!

hebasto commented Apr 28, 2020

Uh oh!

maflcko commented Apr 29, 2020

Uh oh!

brakmic commented May 1, 2020

Uh oh!

dongcarl commented May 1, 2020

068bcc0

098d26d

Uh oh!

hebasto commented May 1, 2020

098d26d

Uh oh!

hebasto commented May 1, 2020

Uh oh!

hebasto commented May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongcarl commented May 4, 2020

Uh oh!

hebasto commented May 4, 2020

Uh oh!

dongcarl commented May 5, 2020

Uh oh!

hebasto commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongcarl commented May 12, 2020

Uh oh!

Uh oh!

DrahtBot commented Apr 13, 2020 •

edited

Loading

`068bcc0`

`098d26d`

hebasto commented May 1, 2020 •

edited

Loading

hebasto commented May 5, 2020 •

edited

Loading