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

Fix release tarball generated by gitian #18818

Closed
wants to merge 2 commits into from

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Apr 29, 2020

Without losing the new and improved git-archive method of ensuring we get the entire source included, this fixes two one remaining regressions to standard source code distribution expectations:

  1. The tarball should be entirely within a single directory (not extract directly to the working directory). (merged via build: Ensure source tarball has leading directory name #20318)
  2. The tarball should be pre-autogen'd, so the user can begin with ./configure and not need maintainer tools.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 29, 2020

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 30, 2020

Gitian builds

File commit 978c5a2
(master)
commit b3ccb88
(master and this pull)
bitcoin-*-aarch64-linux-gnu-debug.tar.gz a7900f03fdbecc03... c028ccae2d68b45b...
bitcoin-*-aarch64-linux-gnu.tar.gz 36b4ba0eff91c058... ebd0ec03feb1445f...
bitcoin-*-arm-linux-gnueabihf-debug.tar.gz a21b10d10a75c2d2... 9c0008f526a0a713...
bitcoin-*-arm-linux-gnueabihf.tar.gz 63ae46d262582c69... 4c81f707cc4b75d6...
bitcoin-*-osx-unsigned.dmg 0422bc13b9dd88c5... 2aa016fc104b05de...
bitcoin-*-osx64.tar.gz 4b58e5ea071d1465... b7ba33216121ca9b...
bitcoin-*-riscv64-linux-gnu-debug.tar.gz 26930d1065f00e44... c913d2fbf930a643...
bitcoin-*-riscv64-linux-gnu.tar.gz 7ed0548483ac2399... 52862432c89e4e5d...
bitcoin-*-win64-debug.zip 8dfb2cfac8097b8f... 8994f550bd426890...
bitcoin-*-win64-setup-unsigned.exe 2add96057ae3fbda... be865006f9cd1f2e...
bitcoin-*-win64.zip fc566aacd3d3f90a... 2626ecaaf26a9b14...
bitcoin-*-x86_64-linux-gnu-debug.tar.gz 40e22958ff9865ff... 3481e8ff857eb030...
bitcoin-*-x86_64-linux-gnu.tar.gz 3b312c2c0456391d... 30a025b342414ebf...
bitcoin-*.tar.gz d02114e741cbf83f... b66c90d783fbb822...
bitcoin-core-linux-0.21-res.yml 9c0c036a266a3022... 337505633195587a...
bitcoin-core-osx-0.21-res.yml 8aaeef8dc2d63c5b... 644bcb3bc4331bc5...
bitcoin-core-win-0.21-res.yml 012d9094c6b92dd8... 1fa1e445bbdb1a3c...
linux-build.log 94a388ad77efc68f... c136bf16f81c0804...
osx-build.log cde6e82e5f552bec... 2e0a0044b22a6558...
win-build.log 8bc04e6d3fefaef9... 601fc37b1ad3ea9e...
bitcoin-core-linux-0.21-res.yml.diff fda54493f5f1bb18...
bitcoin-core-osx-0.21-res.yml.diff b80e6fdf7baaca2e...
bitcoin-core-win-0.21-res.yml.diff c6ea6a7d33fa1a81...
linux-build.log.diff 9ca9f7f27091faac...
osx-build.log.diff ccd37c6a8c7ef6d4...
win-build.log.diff e83cb7c3e107c1b9...

@MarcoFalke
Copy link
Member

MarcoFalke commented Apr 30, 2020

cc @dongcarl @fanquake

Does this break guix builds in any way?

@DrahtBot
Copy link
Contributor

DrahtBot commented May 1, 2020

Guix builds

File commit e5b9308
(master)
commit de73e4f
(master and this pull)
bitcoin-0.20.99-aarch64-linux-gnu-debug.tar.gz bfa9f239a7fe270a... f929954bd4f694d1...
bitcoin-0.20.99-aarch64-linux-gnu.tar.gz d013fc584dcdcad0... 82511f93d64c2625...
bitcoin-0.20.99-arm-linux-gnueabihf-debug.tar.gz 04e65cd9842e06e3... 2501877f77030144...
bitcoin-0.20.99-arm-linux-gnueabihf.tar.gz ac387cc990d16cbb... 64eb7acf282e398d...
bitcoin-0.20.99-riscv64-linux-gnu-debug.tar.gz 652bcc7554c6e41c... 15aa64b7bf5ba3ae...
bitcoin-0.20.99-riscv64-linux-gnu.tar.gz cb40c72590519edf... 7a6ca68d8bbd9ed3...
bitcoin-0.20.99-win-unsigned.tar.gz f0500f89bc7b736a... 44fe15916ccb6aee...
bitcoin-0.20.99-win64-debug.zip be8f2dd8bec85cae... d7832489713ca3c8...
bitcoin-0.20.99-win64-setup-unsigned.exe d4bc6bba8904dc65... d4bc6bba8904dc65...
bitcoin-0.20.99-win64.zip 48a9958e63b940a3... d4003d8f43d602df...
bitcoin-0.20.99-x86_64-linux-gnu-debug.tar.gz 92ef64b319a4b3b3... 9b3293b49b70a150...
bitcoin-0.20.99-x86_64-linux-gnu.tar.gz 592213321383a66c... fb847c977044c496...
bitcoin-0.20.99.tar.gz 63274a6760c28139... 13d90c6c5b49024f...
guix_build.log af29b815b77e0759... 158b2f0e5d39286e...
guix_build.log.diff 9eb6eefa94bf2817...

@dongcarl
Copy link
Contributor

dongcarl commented May 5, 2020

I think we previously decided specifically that we're not going to pre-autogen our tarballs.

See context:

100% on board with restoring the tar leading directory though.

@luke-jr
Copy link
Member Author

luke-jr commented May 5, 2020

This doesn't have the complexity of git ls-files etc in #18478. I see no decision-making relating to intentionally excluding configure (which is a standard/expected part of source code distributions), only a few people being okay with losing it as a side effect.

@dongcarl
Copy link
Contributor

dongcarl commented May 5, 2020

Okay, at this point, I don't have an opinion either way anymore. Will give it a review after others chime in!

@hebasto
Copy link
Member

hebasto commented May 5, 2020

Why not follow @MarcoFalke's suggestion on IRC:

<MarcoFalke> luke-jr: Maybe split out the first commit into a pull, so that gitian builds can be performed on it more easily?

?

@luke-jr
Copy link
Member Author

luke-jr commented May 6, 2020

fanquake added a commit that referenced this issue May 6, 2020
bfe1ba2 rel-builds: Specify core.abbrev for git-rev-parse (Carl Dong)
27e63e0 build: Accomodate makensis v2.x (Carl Dong)
1f2c39a guix: Remove logical cores requirement (Carl Dong)
a4f6ffa lint: Also enable source statements for non-gitian (Carl Dong)
d256f91 rel-builds: Directly deploy win installer to OUTDIR (Carl Dong)
fa791da nsis: Specify OutFile path only once (Carl Dong)
1470160 guix: Expose GIT_COMMON_DIR in container as readonly (Carl Dong)
f5a6ac4 guix: Make source tarball using git-archive (Carl Dong)
395c113 gitian: Limit sourced script to just assignments (Carl Dong)

Pull request description:

  Based on: #18556
  Related: #17595 (comment)

ACKs for top commit:
  fanquake:
    ACK bfe1ba2 - I agree with Carl, and am going to merge this. I'd like for Linux Guix builds to be working again, and we can rebase #18818.

Tree-SHA512: c87ada7e3de17ca0b692a91029b86573442ded5780fc081c214773f6b374a0cdbeaf6f6898c36669c2e247ee32aa7f82defb1180f8decac52c65f0c140f18674
@luke-jr luke-jr force-pushed the fix_gitian_src_202004 branch from 6d47ecc to 4186dc2 Compare May 6, 2020
@luke-jr
Copy link
Member Author

luke-jr commented May 6, 2020

Rebased

@MarcoFalke
Copy link
Member

MarcoFalke commented May 6, 2020

So if this is backported, the only additional commit that also needs backport is 395c113, I think (haven't checked).

--exclude autom4te.cache \
--exclude .deps \
Copy link
Contributor

@dongcarl dongcarl May 6, 2020

Choose a reason for hiding this comment

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

Are these the only things we need to exclude?

Would be nice to have a comparison between "make dist minus all the git-tracked files" vs "bootstrapped files created this way minus the git-tracked files"

Copy link
Member Author

@luke-jr luke-jr May 6, 2020

Choose a reason for hiding this comment

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

tar -tpf build/out/src/bitcoin-3897f3a2ec07.tar.gz | cut -b 22- | sort
git ls-files | perl -nle '$a=$_; while (s[/[^/]+/?$][/]) { if (not exists $d{$_}) { $d{$_} = undef; print } } print $a' | sort

Result:

Makefile.in
aclocal.m4
build-aux/compile
build-aux/config.guess
build-aux/config.sub
build-aux/depcomp
build-aux/install-sh
build-aux/ltmain.sh
build-aux/m4/libtool.m4
build-aux/m4/ltoptions.m4
build-aux/m4/ltsugar.m4
build-aux/m4/ltversion.m4
build-aux/m4/lt~obsolete.m4
build-aux/missing
build-aux/test-driver
configure
doc/man/Makefile.in
src/Makefile.in
src/config/bitcoin-config.h.in
src/secp256k1/Makefile.in
src/secp256k1/aclocal.m4
src/secp256k1/build-aux/compile
src/secp256k1/build-aux/config.guess
src/secp256k1/build-aux/config.sub
src/secp256k1/build-aux/depcomp
src/secp256k1/build-aux/install-sh
src/secp256k1/build-aux/ltmain.sh
src/secp256k1/build-aux/m4/libtool.m4
src/secp256k1/build-aux/m4/ltoptions.m4
src/secp256k1/build-aux/m4/ltsugar.m4
src/secp256k1/build-aux/m4/ltversion.m4
src/secp256k1/build-aux/m4/lt~obsolete.m4
src/secp256k1/build-aux/missing
src/secp256k1/build-aux/test-driver
src/secp256k1/configure
src/secp256k1/src/libsecp256k1-config.h.in
src/univalue/Makefile.in
src/univalue/aclocal.m4
src/univalue/build-aux/compile
src/univalue/build-aux/config.guess
src/univalue/build-aux/config.sub
src/univalue/build-aux/depcomp
src/univalue/build-aux/install-sh
src/univalue/build-aux/ltmain.sh
src/univalue/build-aux/m4/libtool.m4
src/univalue/build-aux/m4/ltoptions.m4
src/univalue/build-aux/m4/ltsugar.m4
src/univalue/build-aux/m4/ltversion.m4
src/univalue/build-aux/m4/lt~obsolete.m4
src/univalue/build-aux/missing
src/univalue/build-aux/test-driver
src/univalue/configure
src/univalue/univalue-config.h.in

@dongcarl
Copy link
Contributor

dongcarl commented May 6, 2020

For the record, my thinking in #18478 originally was this:

  • make dist seems to be designed to know exactly which bootstrapped files to include (or not include) in its archive
  • git archive or git ls-files seems to be good at knowing all the other files we want to include in its archive

Which means that if we combine these two, we'll get the best of both worlds, without having to list anything. This would also be immune to future changes in how bootstrapped files are named, and additional "bootstrapped but shouldn't be included" files like .deps or autom4te.cache.

Does this mean we'll have to keep listing files for make dist?

No it doesn't, because we only care that make dist produces an archive that includes the bootstrapped files, it can miss all the files in share, contrib, etc. and that would be fine, as we'll just either:

  1. Add them in via a git ls-files in EXTRA_DIST like in build: Automatically include both git-tracked and bootstrapped files. #18478, or
  2. Append the git archive archive

Both of which will include all the tracked files

@luke-jr
Copy link
Member Author

luke-jr commented May 6, 2020

@dongcarl Your (1) would miss the substitutions git-archive makes in src/clientversion.cpp. I don't see a problem with (2), although it's slightly more complex and requires care I don't know if we want to spend time reviewing (in particular, tar is picky about specific tar file formats when concatenating, and even if we just use --update, there's a risk that the git archive timestamp is after the gitian reference timestamp, which might make problems)

The solution I implemented here aims to keep it simple and obviously correct.

@hebasto
Copy link
Member

hebasto commented May 7, 2020

@luke-jr

  1. The tarball should be pre-autogen'd, so the user can begin with ./configure and not need maintainer tools.

I have no experience as a package maintainer so I kindly ask you to share the real-world cases when a user cannot begin with ./autogen.sh. It seems having maintainer tools is not a much burden for a user, no?

@laanwj laanwj added this to Blockers in High-priority for review Jun 4, 2020
@laanwj laanwj moved this from Blockers to Bugfixes in High-priority for review Jun 18, 2020
@theuni
Copy link
Member

theuni commented Jun 24, 2020

Including the configure script, and starting with just ./configure is standard, and what people expect both in general and even specifically for Bitcoin Core. There are no real benefits to suddenly not including it, and doing so this way does not add much complexity.

From UNIX BUILD NOTES -- To Build:

./autogen.sh
./configure
make
make install # optional

Docs seem to prevail over standard :)

This is standard procedure for the project's developers. Please don't conflate that with end-user procedure. We bootstrap because distro's and end-user-code-builders (read: not us) expect it. I don't see any reason to discontinue.

Honestly I really can't follow this anymore. I agree with @luke-jr's goals.

-0.

@fanquake fanquake modified the milestones: 0.20.1, 0.20.2 Jul 31, 2020
@laanwj
Copy link
Member

laanwj commented Aug 5, 2020

Just a data point: I haven't had, or otherwise heard of, any complaints about the 0.20.x tarball aren't bootstrapped.

@luke-jr
Copy link
Member Author

luke-jr commented Aug 5, 2020

Well, it's not like we've ever had complete/usable tarballs before either... Maybe people are just used to needing to use git? Dunno.

@MarcoFalke
Copy link
Member

MarcoFalke commented Nov 5, 2020

Removed from 0.21 milestone for now

(The autogen.sh part was controversial and is not a regression-bugfix)

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 8, 2021

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@fanquake
Copy link
Member

fanquake commented Aug 11, 2021

Closing this, as at this point, we aren't likely going back on the build / dist changes. I also don't think I'm aware of any complaints in regards to the release tarball being broken (that's for 0.20 and 0.21).

@fanquake fanquake closed this Aug 11, 2021
UdjinM6 pushed a commit to UdjinM6/dash that referenced this issue Oct 23, 2021
bfe1ba2 rel-builds: Specify core.abbrev for git-rev-parse (Carl Dong)
27e63e0 build: Accomodate makensis v2.x (Carl Dong)
1f2c39a guix: Remove logical cores requirement (Carl Dong)
a4f6ffa lint: Also enable source statements for non-gitian (Carl Dong)
d256f91 rel-builds: Directly deploy win installer to OUTDIR (Carl Dong)
fa791da nsis: Specify OutFile path only once (Carl Dong)
1470160 guix: Expose GIT_COMMON_DIR in container as readonly (Carl Dong)
f5a6ac4 guix: Make source tarball using git-archive (Carl Dong)
395c113 gitian: Limit sourced script to just assignments (Carl Dong)

Pull request description:

  Based on: bitcoin#18556
  Related: bitcoin#17595 (comment)

ACKs for top commit:
  fanquake:
    ACK bfe1ba2 - I agree with Carl, and am going to merge this. I'd like for Linux Guix builds to be working again, and we can rebase bitcoin#18818.

Tree-SHA512: c87ada7e3de17ca0b692a91029b86573442ded5780fc081c214773f6b374a0cdbeaf6f6898c36669c2e247ee32aa7f82defb1180f8decac52c65f0c140f18674
UdjinM6 pushed a commit to UdjinM6/dash that referenced this issue Oct 23, 2021
bfe1ba2 rel-builds: Specify core.abbrev for git-rev-parse (Carl Dong)
27e63e0 build: Accomodate makensis v2.x (Carl Dong)
1f2c39a guix: Remove logical cores requirement (Carl Dong)
a4f6ffa lint: Also enable source statements for non-gitian (Carl Dong)
d256f91 rel-builds: Directly deploy win installer to OUTDIR (Carl Dong)
fa791da nsis: Specify OutFile path only once (Carl Dong)
1470160 guix: Expose GIT_COMMON_DIR in container as readonly (Carl Dong)
f5a6ac4 guix: Make source tarball using git-archive (Carl Dong)
395c113 gitian: Limit sourced script to just assignments (Carl Dong)

Pull request description:

  Based on: bitcoin#18556
  Related: bitcoin#17595 (comment)

ACKs for top commit:
  fanquake:
    ACK bfe1ba2 - I agree with Carl, and am going to merge this. I'd like for Linux Guix builds to be working again, and we can rebase bitcoin#18818.

Tree-SHA512: c87ada7e3de17ca0b692a91029b86573442ded5780fc081c214773f6b374a0cdbeaf6f6898c36669c2e247ee32aa7f82defb1180f8decac52c65f0c140f18674
UdjinM6 pushed a commit to UdjinM6/dash that referenced this issue Dec 4, 2021
bfe1ba2 rel-builds: Specify core.abbrev for git-rev-parse (Carl Dong)
27e63e0 build: Accomodate makensis v2.x (Carl Dong)
1f2c39a guix: Remove logical cores requirement (Carl Dong)
a4f6ffa lint: Also enable source statements for non-gitian (Carl Dong)
d256f91 rel-builds: Directly deploy win installer to OUTDIR (Carl Dong)
fa791da nsis: Specify OutFile path only once (Carl Dong)
1470160 guix: Expose GIT_COMMON_DIR in container as readonly (Carl Dong)
f5a6ac4 guix: Make source tarball using git-archive (Carl Dong)
395c113 gitian: Limit sourced script to just assignments (Carl Dong)

Pull request description:

  Based on: bitcoin#18556
  Related: bitcoin#17595 (comment)

ACKs for top commit:
  fanquake:
    ACK bfe1ba2 - I agree with Carl, and am going to merge this. I'd like for Linux Guix builds to be working again, and we can rebase bitcoin#18818.

Tree-SHA512: c87ada7e3de17ca0b692a91029b86573442ded5780fc081c214773f6b374a0cdbeaf6f6898c36669c2e247ee32aa7f82defb1180f8decac52c65f0c140f18674
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants