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

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

Merged
merged 3 commits into from May 17, 2017

Conversation

Projects
None yet
5 participants
@luke-jr
Member

luke-jr commented Feb 12, 2016

Also adds ability to disable check with BITCOIN_GENBUILD_NO_GIT=1 in the environment

@theuni @Flowdalic @laanwj

I believe this addresses all the possible use cases correctly.

(This commit is based on branch-0.10, so should merge cleanly into 0.10, 0.11, 0.12, and master branches.)

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

Also adds ability to disable check with BITCOIN_GENBUILD_NO_GIT=1 in the environment
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 14, 2016

Member

@theuni can you take a look here?

Member

laanwj commented Apr 14, 2016

@theuni can you take a look here?

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 15, 2016

Member

To save others the digging:

This verifies that share/genbuild.sh exists at the git root. If it exists at a path that isn't at the git root, the check fails. For out-of-tree builds, that's fine because share/genbuild.sh would only exist in the source, not in the build dir.

However, this breaks for nested builds where the source is copied into a subdir, because share/genbuild.sh does end up existing there. We currently build that way for Gitian and Travis because out-of-tree builds don't work. Which is why the Gitian descriptors needed to be changed.

So.. ACK, but I would very much prefer to get the out-of-tree build changes merged first, and Gitian/Travis fixed up accordingly.

Member

theuni commented Apr 15, 2016

To save others the digging:

This verifies that share/genbuild.sh exists at the git root. If it exists at a path that isn't at the git root, the check fails. For out-of-tree builds, that's fine because share/genbuild.sh would only exist in the source, not in the build dir.

However, this breaks for nested builds where the source is copied into a subdir, because share/genbuild.sh does end up existing there. We currently build that way for Gitian and Travis because out-of-tree builds don't work. Which is why the Gitian descriptors needed to be changed.

So.. ACK, but I would very much prefer to get the out-of-tree build changes merged first, and Gitian/Travis fixed up accordingly.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Apr 15, 2016

Member

Well, specifically it makes sure the script itself (genbuild.sh) is part of the git repository it's about to get information from.

If the source is simply copied into a subdir, it shouldn't be using git (by default), so the "breaks" @theuni mentions is in fact a bug fix. The Gitian descriptor change tells git to basically use the git repo even for the differing source dir.

Member

luke-jr commented Apr 15, 2016

Well, specifically it makes sure the script itself (genbuild.sh) is part of the git repository it's about to get information from.

If the source is simply copied into a subdir, it shouldn't be using git (by default), so the "breaks" @theuni mentions is in fact a bug fix. The Gitian descriptor change tells git to basically use the git repo even for the differing source dir.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 16, 2016

Member

So.. ACK, but I would very much prefer to get the out-of-tree build changes merged first,

Huray :)

Member

laanwj commented Apr 16, 2016

So.. ACK, but I would very much prefer to get the out-of-tree build changes merged first,

Huray :)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 26, 2016

Member

The out-of-tree build changes have been merged a while ago.

Anything left to do here?

Member

laanwj commented May 26, 2016

The out-of-tree build changes have been merged a while ago.

Anything left to do here?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 1, 2016

Member

@theuni So all good to merge now?

Member

sipa commented Jun 1, 2016

@theuni So all good to merge now?

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 1, 2016

Member

I just fixed up the rest of the out-of-tree stuff, and switched Travis to use it. After that, the changes here don't play nice with VPATH builds.

@luke-jr Mind having a look? I'll PR those changes now.

Member

theuni commented Jun 1, 2016

I just fixed up the rest of the out-of-tree stuff, and switched Travis to use it. After that, the changes here don't play nice with VPATH builds.

@luke-jr Mind having a look? I'll PR those changes now.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 2, 2016

Member

@theuni What is a "VPATH build"? If the changes here break it, then "VPATH builds" are already buggy and should probably be fixed before merging anyway...?

Member

luke-jr commented Jun 2, 2016

@theuni What is a "VPATH build"? If the changes here break it, then "VPATH builds" are already buggy and should probably be fixed before merging anyway...?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 17, 2016

Member

#8113 went in.
What is the status of this now?

Member

laanwj commented Jun 17, 2016

#8113 went in.
What is the status of this now?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 17, 2016

Member

Current status is that #8133 (comment) suggests this PR doesn't fix the problem in practice, so I will need to review why.

Member

luke-jr commented Jun 17, 2016

Current status is that #8133 (comment) suggests this PR doesn't fix the problem in practice, so I will need to review why.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 29, 2016

Member

Closing for now as the changes apparently don't solve the problem. Ask me to reopen or open a new pull when there is progress on this.

Member

laanwj commented Sep 29, 2016

Closing for now as the changes apparently don't solve the problem. Ask me to reopen or open a new pull when there is progress on this.

@laanwj laanwj closed this Sep 29, 2016

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Dec 30, 2016

Member

Reopen, as asked for by luke on irc.

Member

MarcoFalke commented Dec 30, 2016

Reopen, as asked for by luke on irc.

@MarcoFalke MarcoFalke reopened this Dec 30, 2016

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Dec 30, 2016

Member

Updated to actually fix the issue.

Member

luke-jr commented Dec 30, 2016

Updated to actually fix the issue.

@sipa

Concept ACK

@@ -101,6 +101,7 @@ script: |
export PATH=${WRAP_DIR}:${PATH}
# Create the release tarball using (arbitrarily) the first host
export GIT_DIR="$PWD/.git"

This comment has been minimized.

@sipa

sipa Apr 9, 2017

Member

Is this needed?

@sipa

sipa Apr 9, 2017

Member

Is this needed?

This comment has been minimized.

@luke-jr

luke-jr Apr 9, 2017

Member

Since we build from the exported tarball, this is needed if we want the build system to use git to create the versioninfo. Otherwise, it will detect it is built from the tarball and not from git.

@luke-jr

luke-jr Apr 9, 2017

Member

Since we build from the exported tarball, this is needed if we want the build system to use git to create the versioninfo. Otherwise, it will detect it is built from the tarball and not from git.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 2, 2017

Member

@theuni can you please take a look at this again? Seems to be the oldest open PR now, we should either merge it or close it.

Member

laanwj commented May 2, 2017

@theuni can you please take a look at this again? Seems to be the oldest open PR now, we should either merge it or close it.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni May 2, 2017

Member

Since we build from the exported tarball, this is needed if we want the build system to use git to create the versioninfo. Otherwise, it will detect it is built from the tarball and not from git.

This seems backwards. Though since it's strictly an improvement, utACK for now.

I think we should strive to make sure that building from the tarball results in the correct version string with no need for git, but I'll work on that separately.

Member

theuni commented May 2, 2017

Since we build from the exported tarball, this is needed if we want the build system to use git to create the versioninfo. Otherwise, it will detect it is built from the tarball and not from git.

This seems backwards. Though since it's strictly an improvement, utACK for now.

I think we should strive to make sure that building from the tarball results in the correct version string with no need for git, but I'll work on that separately.

@laanwj laanwj merged commit ed1fcdc into bitcoin:master May 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 17, 2017

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

ed1fcdc Bugfix: Detect genbuild.sh in repo correctly (Luke Dashjr)
e98e3dd Bugfix: Only use git for build info if the repository is actually the right one (Luke Dashjr)

Tree-SHA512: 510d7ec8cfeff4e8e0c7ac53631eb32c7acaada7017e7f8cc2e6f60d86afe1cd131870582e01022f961c85a783a130bcb8fef971f8b110070c9c02afda020726

laanwj added a commit that referenced this pull request Aug 21, 2017

Merge #11097: gitian: quick hack to fix version string in releases
4452829 gitian: quick hack to fix version string in releases (Cory Fields)

Pull request description:

  Credit: @luke-jr
  Release version strings were broken in Gitian by #7522. This is a minimal fix suitable for 0.15.

  After this, we should fix up version handling for good so that gitian packages the correct string in the release tarball, so that git is not required to get the tag name.

Tree-SHA512: fa609a744c46306b0809f08fed6e96eff41b13e82f3e213711e4abef370558b64a68972f283a038330882cb6c40b32547fbb0f89b8058cc2c6025bff134473c3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment