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

build: Fix quick hack for version string in releases #18349

Closed
wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Mar 14, 2020

This PR:

@hebasto hebasto changed the title build: Fix quick hack for version string in releases [WIP] build: Fix quick hack for version string in releases Mar 14, 2020
@hebasto hebasto force-pushed the 20200314-version-workaround branch from 1b1546c to 87845ef Compare March 14, 2020 20:05
@hebasto hebasto changed the title [WIP] build: Fix quick hack for version string in releases build: Fix quick hack for version string in releases Mar 14, 2020
@hebasto
Copy link
Member Author

hebasto commented Mar 14, 2020

Updated 1b1546c -> 87845ef (pr18349.01 -> pr18349.02, compare):

@hebasto hebasto force-pushed the 20200314-version-workaround branch from 87845ef to 24d9422 Compare March 15, 2020 11:00
@hebasto
Copy link
Member Author

hebasto commented Mar 15, 2020

Updated 87845ef -> 24d9422 (pr18349.02 -> pr18349.03, compare):

@hebasto hebasto force-pushed the 20200314-version-workaround branch from 24d9422 to f8fe1f0 Compare March 15, 2020 17:23
@hebasto
Copy link
Member Author

hebasto commented Mar 15, 2020

Updated 24d9422 -> f8fe1f0 (pr18349.03 -> pr18349.04, diff):

@hebasto hebasto force-pushed the 20200314-version-workaround branch from f8fe1f0 to 80190e7 Compare March 25, 2020 16:59
@hebasto
Copy link
Member Author

hebasto commented Mar 25, 2020

Rebased f8fe1f0 -> 80190e7 due to the merging of #18331.

@maflcko
Copy link
Member

maflcko commented Mar 25, 2020

cc @dongcarl since you opened #16588

@@ -37,6 +37,8 @@ if [ "${BITCOIN_GENBUILD_NO_GIT}" != "1" ] && [ -e "$(command -v git)" ] && [ "$
# otherwise generate suffix from git, i.e. string like "59887e8-dirty"
SUFFIX=$(git rev-parse --short HEAD)
git diff-index --quiet HEAD -- || SUFFIX="$SUFFIX-dirty"
elif [ -f "$FILE" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

we already have a if [ -f "$FILE" ]; then check above, would it make sense to merge this into that?

Copy link
Member Author

Choose a reason for hiding this comment

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

This condition handles a special case for gitian builds: with this PR the obj/build.h file is a part of the source tree that is fed to the make, and we do not want genbuild.sh to re-write it with the // No build information available string.

Copy link
Member

Choose a reason for hiding this comment

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

How does it get updated for local build then, if the file already exists?

Copy link
Member Author

Choose a reason for hiding this comment

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

How does it get updated for local build then, if the file already exists?

For local build the elif condition is not applied as the previous if condition is evaluated to true.

Copy link
Member

@laanwj laanwj Apr 1, 2020

Choose a reason for hiding this comment

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

Ok, ths logic is kind of tricky to me. Even what it's supposed to do in what case. Maybe we need a test to make this clearer, enumerate the cases and verify that they still work.

@laanwj laanwj added this to the 0.20.0 milestone Mar 27, 2020
@DrahtBot
Copy link
Contributor

Gitian builds

File commit 7f9dedb
(master)
commit 6a3fdb2
(master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz 8aa34845a916014d... 4a36df05dffe2b3e...
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz 0fb3dd16db30ea00... 3553185c8bf583aa...
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz 797f185a8e62f6fe... f5ddc5e8f221dd35...
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz 871094b510da0ac7... 35da0d9145e5d590...
bitcoin-0.19.99-osx-unsigned.dmg a159b1b99b607907... 47e528a694248d29...
bitcoin-0.19.99-osx64.tar.gz 1fb6370553afcb22... ff3e6095c7bcaf08...
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz 0b86916151aea5a3... 6e724946be7bde0d...
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz 4efb63771485711b... fae7ca9020f9dd33...
bitcoin-0.19.99-win64-debug.zip dd825e6168d39c90... a4173e1fe79fe3aa...
bitcoin-0.19.99-win64-setup-unsigned.exe d1f790ad8c257da0... cd88ce1150524f63...
bitcoin-0.19.99-win64.zip 49dc4119ae2a01c4... f0ed4bfa63013390...
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz 2a144f502390b6e3... 90b23159cdd83ed4...
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz af6a5d2d555569a4... 601b4637b9c32347...
bitcoin-0.19.99.tar.gz 900859565b6ee1fe... fd524a224feae8be...
bitcoin-core-linux-0.20-res.yml 70c720b679a19c93... d8b6e2b1e46b714a...
bitcoin-core-osx-0.20-res.yml 5766a2f7c8d8547a... 846eb26591fb059b...
bitcoin-core-win-0.20-res.yml a95a890f0d0cb882... dff3c2d415b90d84...
linux-build.log 8510c43606df6b17... 411cb639aa5c4757...
osx-build.log b2fce58471126628... 7bd0568c479dc0ff...
win-build.log 630e3d23ee0657f4... 4c7a28fde557f4c9...
bitcoin-core-linux-0.20-res.yml.diff f9ef5da1fe953b6f...
bitcoin-core-osx-0.20-res.yml.diff 3572f99bf005a178...
bitcoin-core-win-0.20-res.yml.diff 015ccb9ce0b68e13...
linux-build.log.diff f1b5df0530158a6f...
osx-build.log.diff fc33c8a71921263c...
win-build.log.diff ec0237196b0ea3d7...

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 31, 2020

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

Conflicts

No conflicts as of last run.

@hebasto
Copy link
Member Author

hebasto commented Apr 1, 2020

@laanwj

Ok, ths logic is kind of tricky to me. Even what it's supposed to do in what case. Maybe we need a test to make this clearer, enumerate the cases and verify that they still work.

Here are all of the cases:

source location head obj/build.h clientversion.cpp#L46 resulted version string
git repo root commit #define BUILD_SUFFIX COMMIT_HASH --- version v0.19.99.0-COMMIT_HASH
git repo root tagged commit #define BUILD_DESC "TAG" --- version TAG
git repo root not commited changes #define BUILD_SUFFIX COMMIT_HASH-dirty --- version v0.19.99.0-COMMIT_HASH-dirty
git archive commit // No build information available #define GIT_COMMIT_ID "COMMIT_HASH" version version v0.19.99.0-gCOMMIT_HASH
make dist used in gitian tagged commit BEFORE build: #define BUILD_DESC "TAG" #define GIT_COMMIT_ID "COMMIT_HASH" version TAG

In the latter case this PR prevents re-writing over the existing good obj/build.h.

Also these comments could be useful:

/**
* The following part of the code determines the CLIENT_BUILD variable.
* Several mechanisms are used for this:
* * first, if HAVE_BUILD_INFO is defined, include build.h, a file that is
* generated by the build environment, possibly containing the output
* of git-describe in a macro called BUILD_DESC
* * secondly, if this is an exported version of the code, GIT_ARCHIVE will
* be defined (automatically using the export-subst git attribute), and
* GIT_COMMIT will contain the commit id.
* * then, three options exist for determining CLIENT_BUILD:
* * if BUILD_DESC is defined, use that literally (output of git-describe)
* * if not, but GIT_COMMIT is defined, use v[maj].[min].[rev].[build]-g[commit]
* * otherwise, use v[maj].[min].[rev].[build]-unk
* finally CLIENT_VERSION_SUFFIX is added
*/

@maflcko
Copy link
Member

maflcko commented Apr 2, 2020

Why is this assigned 0.20.0? This is not a bugfix and even has the potential to break stuff.

@hebasto
Copy link
Member Author

hebasto commented Apr 7, 2020

Closed in favor of #18556.

@hebasto hebasto closed this Apr 7, 2020
fanquake added a commit that referenced this pull request Apr 28, 2020
2aa48ed refactor: Drop unused ${WRAP_DIR}/${HOST} directory (Hennadii Stepanov)
1362be0 build: Drop make dist in gitian builds (Hennadii Stepanov)

Pull request description:

  After the merge of #18331, the packaged source tarball is created by `git archive`, but the binaries are built from another one which is made by `make dist`.

  With this PR the only source tarball, created by `git archive`, is used both for binaries building and for packaging to users.

  Close #16588.
  Close #18547.

  As a good side-effect, #18349 becomes redundant.

  **Change in behavior**

  The following variables https://github.com/bitcoin/bitcoin/blob/1b151e3ffce7c1a2ee46bf280cc1d96775d1f91e/configure.ac#L2-L6

  are no longer used for naming of directories and tarballs.

  Instead of them the gitian descriptors use a git tag (if available) or a commit hash.

  ---

  Also a small refactor commit picked from #18404.

ACKs for top commit:
  dongcarl:
    ACK 2aa48ed
  MarcoFalke:
    ACK 2aa48ed
  fanquake:
    ACK 2aa48ed - I've had a quick look over this, and don't want to block merging if this actually gets as closer to finally having this all sorted out. Obviously we've still got #18741, and after speaking to Carl this morning, there will likely be even more changes after that (not Guix specific).

Tree-SHA512: d3b16f87e48d1790a3264940c28acd5d881bfd10f3ce94fb0c8a6af76d8039289d01e0cd4972adac49ae24362857251f6c1e5e09e3e9fbf636c10708b4015a7c
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 28, 2020
2aa48ed refactor: Drop unused ${WRAP_DIR}/${HOST} directory (Hennadii Stepanov)
1362be0 build: Drop make dist in gitian builds (Hennadii Stepanov)

Pull request description:

  After the merge of bitcoin#18331, the packaged source tarball is created by `git archive`, but the binaries are built from another one which is made by `make dist`.

  With this PR the only source tarball, created by `git archive`, is used both for binaries building and for packaging to users.

  Close bitcoin#16588.
  Close bitcoin#18547.

  As a good side-effect, bitcoin#18349 becomes redundant.

  **Change in behavior**

  The following variables https://github.com/bitcoin/bitcoin/blob/1b151e3ffce7c1a2ee46bf280cc1d96775d1f91e/configure.ac#L2-L6

  are no longer used for naming of directories and tarballs.

  Instead of them the gitian descriptors use a git tag (if available) or a commit hash.

  ---

  Also a small refactor commit picked from bitcoin#18404.

ACKs for top commit:
  dongcarl:
    ACK 2aa48ed
  MarcoFalke:
    ACK 2aa48ed
  fanquake:
    ACK 2aa48ed - I've had a quick look over this, and don't want to block merging if this actually gets as closer to finally having this all sorted out. Obviously we've still got bitcoin#18741, and after speaking to Carl this morning, there will likely be even more changes after that (not Guix specific).

Tree-SHA512: d3b16f87e48d1790a3264940c28acd5d881bfd10f3ce94fb0c8a6af76d8039289d01e0cd4972adac49ae24362857251f6c1e5e09e3e9fbf636c10708b4015a7c
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Oct 23, 2021
2aa48ed refactor: Drop unused ${WRAP_DIR}/${HOST} directory (Hennadii Stepanov)
1362be0 build: Drop make dist in gitian builds (Hennadii Stepanov)

Pull request description:

  After the merge of bitcoin#18331, the packaged source tarball is created by `git archive`, but the binaries are built from another one which is made by `make dist`.

  With this PR the only source tarball, created by `git archive`, is used both for binaries building and for packaging to users.

  Close bitcoin#16588.
  Close bitcoin#18547.

  As a good side-effect, bitcoin#18349 becomes redundant.

  **Change in behavior**

  The following variables https://github.com/bitcoin/bitcoin/blob/1b151e3ffce7c1a2ee46bf280cc1d96775d1f91e/configure.ac#L2-L6

  are no longer used for naming of directories and tarballs.

  Instead of them the gitian descriptors use a git tag (if available) or a commit hash.

  ---

  Also a small refactor commit picked from bitcoin#18404.

ACKs for top commit:
  dongcarl:
    ACK 2aa48ed
  MarcoFalke:
    ACK 2aa48ed
  fanquake:
    ACK 2aa48ed - I've had a quick look over this, and don't want to block merging if this actually gets as closer to finally having this all sorted out. Obviously we've still got bitcoin#18741, and after speaking to Carl this morning, there will likely be even more changes after that (not Guix specific).

Tree-SHA512: d3b16f87e48d1790a3264940c28acd5d881bfd10f3ce94fb0c8a6af76d8039289d01e0cd4972adac49ae24362857251f6c1e5e09e3e9fbf636c10708b4015a7c
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Oct 23, 2021
2aa48ed refactor: Drop unused ${WRAP_DIR}/${HOST} directory (Hennadii Stepanov)
1362be0 build: Drop make dist in gitian builds (Hennadii Stepanov)

Pull request description:

  After the merge of bitcoin#18331, the packaged source tarball is created by `git archive`, but the binaries are built from another one which is made by `make dist`.

  With this PR the only source tarball, created by `git archive`, is used both for binaries building and for packaging to users.

  Close bitcoin#16588.
  Close bitcoin#18547.

  As a good side-effect, bitcoin#18349 becomes redundant.

  **Change in behavior**

  The following variables https://github.com/bitcoin/bitcoin/blob/1b151e3ffce7c1a2ee46bf280cc1d96775d1f91e/configure.ac#L2-L6

  are no longer used for naming of directories and tarballs.

  Instead of them the gitian descriptors use a git tag (if available) or a commit hash.

  ---

  Also a small refactor commit picked from bitcoin#18404.

ACKs for top commit:
  dongcarl:
    ACK 2aa48ed
  MarcoFalke:
    ACK 2aa48ed
  fanquake:
    ACK 2aa48ed - I've had a quick look over this, and don't want to block merging if this actually gets as closer to finally having this all sorted out. Obviously we've still got bitcoin#18741, and after speaking to Carl this morning, there will likely be even more changes after that (not Guix specific).

Tree-SHA512: d3b16f87e48d1790a3264940c28acd5d881bfd10f3ce94fb0c8a6af76d8039289d01e0cd4972adac49ae24362857251f6c1e5e09e3e9fbf636c10708b4015a7c
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Dec 4, 2021
2aa48ed refactor: Drop unused ${WRAP_DIR}/${HOST} directory (Hennadii Stepanov)
1362be0 build: Drop make dist in gitian builds (Hennadii Stepanov)

Pull request description:

  After the merge of bitcoin#18331, the packaged source tarball is created by `git archive`, but the binaries are built from another one which is made by `make dist`.

  With this PR the only source tarball, created by `git archive`, is used both for binaries building and for packaging to users.

  Close bitcoin#16588.
  Close bitcoin#18547.

  As a good side-effect, bitcoin#18349 becomes redundant.

  **Change in behavior**

  The following variables https://github.com/bitcoin/bitcoin/blob/1b151e3ffce7c1a2ee46bf280cc1d96775d1f91e/configure.ac#L2-L6

  are no longer used for naming of directories and tarballs.

  Instead of them the gitian descriptors use a git tag (if available) or a commit hash.

  ---

  Also a small refactor commit picked from bitcoin#18404.

ACKs for top commit:
  dongcarl:
    ACK 2aa48ed
  MarcoFalke:
    ACK 2aa48ed
  fanquake:
    ACK 2aa48ed - I've had a quick look over this, and don't want to block merging if this actually gets as closer to finally having this all sorted out. Obviously we've still got bitcoin#18741, and after speaking to Carl this morning, there will likely be even more changes after that (not Guix specific).

Tree-SHA512: d3b16f87e48d1790a3264940c28acd5d881bfd10f3ce94fb0c8a6af76d8039289d01e0cd4972adac49ae24362857251f6c1e5e09e3e9fbf636c10708b4015a7c
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quick version string hack that was never fixed
5 participants