-
Notifications
You must be signed in to change notification settings - Fork 37.3k
Fix release tarball generated by gitian #18818
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
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
0743dfe
to
6d47ecc
Compare
Does this break guix builds in any way?
|
Guix builds
|
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. |
This doesn't have the complexity of |
Okay, at this point, I don't have an opinion either way anymore. Will give it a review after others chime in! |
Why not follow @MarcoFalke's suggestion on IRC:
? |
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
6d47ecc
to
4186dc2
Compare
Rebased |
4186dc2
to
3897f3a
Compare
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 \ |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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
For the record, my thinking in #18478 originally was this:
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 Does this mean we'll have to keep listing files for
|
@dongcarl Your (1) would miss the substitutions git-archive makes in The solution I implemented here aims to keep it simple and obviously correct. |
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 |
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. |
Just a data point: I haven't had, or otherwise heard of, any complaints about the 0.20.x tarball aren't bootstrapped. |
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. |
3897f3a
to
b3a3a87
Compare
Removed from 0.21 milestone for now (The autogen.sh part was controversial and is not a regression-bugfix) |
b3a3a87
to
1a606ef
Compare
🐙 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". |
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). |
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
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
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
Without losing the new and improved
git-archive
method of ensuring we get the entire source included, this fixestwoone remaining regressionsto standard source code distribution expectations: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)./configure
and not need maintainer tools.