script: previous_release.sh rewritten in python #19205

bliotti · 2020-06-08T01:01:11Z

Added functionality:

checks file hash before untarring when using the binary download option

Sjors

Concept ACK, thanks for working on this. 6e4a07c works on macOS 10.15.4 for the standard set of binaries we download, release candidates, and for compiling v0.20.0.

Can you squash your commits?

You can also remove previous_release.sh and switch over to this Python version (in a separate commit).

Are you using Python 3.5.6? (pyenv should do that automatically)

If you check the checksums then it makes sense to also check their PGP signature. Alternatively we might as well hard code the checksum for each tar.gz release in the source code, here. That way Travis might catch someone messing with bitcoincore.org.

I initially added -f Configure for functional tests as a way to opt-out of building QT. Looking at it again, I think we shouldn't build QT period. So I added inline suggestions to drop QT everywhere.

contrib/devtools/previous-release.py

practicalswift · 2020-06-08T11:16:48Z

Concept ACK

Nice first-time contribution @bliotti - welcome as a contributor! :)

bliotti · 2020-06-08T13:25:23Z

Thanks, I have always wanted to contribute! I will follow up with the mentioned suggestions. :)

contrib/devtools/previous-release.py

bliotti · 2020-06-08T20:05:34Z

Concept ACK, thanks for working on this. 6e4a07c works on macOS 10.15.4 for the standard set of binaries we download, release candidates, and for compiling v0.20.0.

Can you squash your commits?

You can also remove previous_release.sh and switch over to this Python version (in a separate commit).

Are you using Python 3.5.6? (pyenv should do that automatically)

If you check the checksums then it makes sense to also check their PGP signature. Alternatively we might as well hard code the checksum for each tar.gz release in the source code, here. That way Travis might catch someone messing with bitcoincore.org.

I initially added -f Configure for functional tests as a way to opt-out of building QT. Looking at it again, I think we shouldn't build QT period. So I added inline suggestions to drop QT everywhere.

Yes I will squash my commits.

Just to confirm, I need to squash all these commits into one commit that encompasses the addition of the py script, and then... push up one more commit for the removal of the bash script.

Most of the suggestions have been incorporated I just need to add the PGP signature check and I should have it up soon.

Sjors

I restarted Travis for an unrelated failure (details in #19221)

It's probably better to move closes #18132 into the commit message body instead of the subject (needs a blank line). Title could be: test: switch usage of previous_release.sh to .py . Right now the changes are mixed between both commits; a single commit is fine too if it's tricky to untangle.

Let's not download and add PGP keys on behalf on the user. You could check if the key is present and fail otherwise.

contrib/devtools/previous-release.py

fjahr · 2020-06-18T14:05:08Z

Concept ACK

Great first contribution @bliotti! I will take a closer look one you have squashed the commits. You can make yourself a co-author as well, I think. And I think @Sjors mistyped, it should be script: ....

fjahr

tested ACK 60faac3

However, the docs should be updated. CI failure seems to be unrelated. I could restart Appveyor but it seems I can't restart that Travis build.

Also, the formatting changes in the second commit make this a longer review than necessary and I think many of them are not worth the noise they create. In some cases, the original authors even made an effort to line up comments etc. I don't use that style but it's not necessary to remove it unless you are editing that particular part of the file. For me, it's ok to keep them but other reviewers might disagree. If you keep them they should be moved into their own commit so that their current commit does what the commit message says: script: removed previous_release.sh. I would suggest to make it:

script: switch previous_release from .sh to .py version

Remove previous_release.sh

(the switch is more significant than the removal of the file).

And then the formatting changes move to refactor: Clean up functional compat test formatting.

EDIT: actually, the first commit message should be changed as well then because it doesn't do the switch now. Should be just something like script: Add previous_release.py.

test/functional/wallet_upgradewallet.py

contrib/devtools/previous-release.py

test/functional/feature_backwards_compatibility.py

test/functional/wallet_upgradewallet.py

DrahtBot · 2020-07-02T23:46:36Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test #19013 (test: add v0.20.0 to backwards compatibility test by Sjors)

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.

contrib/devtools/previous_release.py

fjahr · 2020-07-04T12:03:39Z

I think for the "co-authored-by:" to work as intended it needs to be at the end of the commit message. You can check that it shows both profile pictures and in Github it says 'bliotti and bboot committed'. You also only need to list the additional co-authors, not the author of the commit itself (yourself in this case).

See 4950e5d (#19055) for example.

closes bitcoin#18132 added GPG verify for binaries co-authored-by: bboot <bboot@cisco.com>

fjahr · 2020-07-05T21:30:42Z

re-ACK 9c34aff

Side-note: Waiting for author label can be removed.

bliotti · 2020-07-05T22:04:52Z

thanks for the help @fjahr 👍

Sjors · 2020-07-09T16:22:07Z

tACK 9c34aff

bliotti · 2020-07-20T11:21:26Z

Is there anything I need to do now?

fjahr · 2020-07-20T11:28:49Z

Is there anything I need to do now?

Not really, but you could respectfully ping someone to get another review/ACK. With three ACKs this should be ready for merge and if maintainers don't take notice then you could post a message in IRC that this might be RFM after a few days.

jonatack · 2020-07-20T12:44:18Z

Is there anything I need to do now?

@bliotti welcome! May I suggest this section (particularly the last item) https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#finding-reviewers, followed by these steps:

Participate in https://bitcoincore.reviews and check out the resources listed at the bottom of the home page
Help out reviewing some of the hundreds of open issues and PRs that are waiting for testing or review, for instance among these by your kind reviewers 🙂 https://github.com/bitcoin/bitcoin/pulls/fjahr and https://github.com/bitcoin/bitcoin/pulls/Sjors

emsheets · 2020-07-20T16:35:55Z

Concept ACK, will test later today if test ACKs still needed at that time. Great stuff, @bliotti!!

maflcko · 2020-07-21T09:07:29Z

I did some minor cleanups in #19560

9c34aff Remove previous_release.sh (Brian Liotti) e1e5960 script: Add previous_release.py (Brian Liotti) Pull request description: Closes bitcoin#18132 Added functionality: 1) checks file hash before untarring when using the binary download option ACKs for top commit: fjahr: re-ACK 9c34aff Sjors: tACK 9c34aff Tree-SHA512: 323f11828736a372a47f048592de8b027ddcd75b38f312dfc73f7b495d1e078bfeb384d9cdf434b3e70f2c6c0ce2da2df48e9a6460ac0e1967c6829a411c52d5

bliotti · 2020-07-21T18:19:03Z

I did some minor cleanups in #19560

Thanks! @MarcoFalke

hebasto · 2020-08-26T12:12:01Z

Alternatively we might as well hard code the checksum for each tar.gz release in the source code, here. That way Travis might catch someone messing with bitcoincore.org.

Please see #19813.

0374e82 util: Hard code previous release tarball checksums (Hennadii Stepanov) bd897ce scripted-diff: Move previous_release.py to test/get_previous_releases.py (Hennadii Stepanov) Pull request description: #19205 introduced signature verifying for the downloaded `SHA256SUMS.asc`. This approach is brittle and does not work in CI environment for many reasons: - bitcoin/bitcoin#19812 (comment) - bitcoin/bitcoin#19013 (comment) This PR: - implements **Sjors**' [idea](bitcoin/bitcoin#19205 (review)): > Alternatively we might as well hard code the checksum for each `tar.gz` release in the source code, here. - is an alternative to 5a2c31e528e6bd60635096f233252f3c717f366d (#19013) - fixes #19812 - updates v0.17.1 to v0.17.2 ACKs for top commit: MarcoFalke: cr ACK 0374e82 Sjors: tACK 0374e82 Tree-SHA512: cacdcf9f5209eae7da357abb3445585ad2f980920fd5bf75527ce89974d3f531a4cf8b5b35edfc116b23bfdfb45c0437cb14cbc416d76ed2dc5b9e6d33cdad71

…cksums 0374e82 util: Hard code previous release tarball checksums (Hennadii Stepanov) bd897ce scripted-diff: Move previous_release.py to test/get_previous_releases.py (Hennadii Stepanov) Pull request description: bitcoin#19205 introduced signature verifying for the downloaded `SHA256SUMS.asc`. This approach is brittle and does not work in CI environment for many reasons: - bitcoin#19812 (comment) - bitcoin#19013 (comment) This PR: - implements **Sjors**' [idea](bitcoin#19205 (review)): > Alternatively we might as well hard code the checksum for each `tar.gz` release in the source code, here. - is an alternative to 5a2c31e (bitcoin#19013) - fixes bitcoin#19812 - updates v0.17.1 to v0.17.2 ACKs for top commit: MarcoFalke: cr ACK 0374e82 Sjors: tACK 0374e82 Tree-SHA512: cacdcf9f5209eae7da357abb3445585ad2f980920fd5bf75527ce89974d3f531a4cf8b5b35edfc116b23bfdfb45c0437cb14cbc416d76ed2dc5b9e6d33cdad71

fanquake added the Scripts and tools label Jun 8, 2020

Rickyricky1987 approved these changes Jun 8, 2020

View reviewed changes

Sjors suggested changes Jun 8, 2020

View reviewed changes

fanquake reviewed Jun 8, 2020

View reviewed changes

contrib/devtools/previous-release.py Outdated Show resolved Hide resolved

fanquake reviewed Jun 8, 2020

View reviewed changes

contrib/devtools/previous-release.py Outdated Show resolved Hide resolved

bliotti force-pushed the previous-release-py branch from 6e4a07c to 53d1068 Compare June 9, 2020 02:15

Sjors reviewed Jun 9, 2020

View reviewed changes

contrib/devtools/previous-release.py Outdated Show resolved Hide resolved

contrib/devtools/previous-release.py Outdated Show resolved Hide resolved

maflcko added the Waiting for author label Jun 16, 2020

bliotti force-pushed the previous-release-py branch 4 times, most recently from a93248f to 60faac3 Compare July 2, 2020 08:25

fjahr approved these changes Jul 2, 2020

View reviewed changes

test/functional/wallet_upgradewallet.py Outdated Show resolved Hide resolved

contrib/devtools/previous-release.py Outdated Show resolved Hide resolved

test/functional/feature_backwards_compatibility.py Show resolved Hide resolved

maflcko reviewed Jul 2, 2020

View reviewed changes

test/functional/wallet_upgradewallet.py Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jul 2, 2020

test: Fix intermittent test failure in feature_backwards_compatibility #19332

Closed

This was referenced Jul 3, 2020

test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test #19013

Merged

wallet: upgradewallet fixes and additional tests #18836

Merged

bliotti force-pushed the previous-release-py branch from 60faac3 to 58b3fdf Compare July 4, 2020 06:02

maflcko reviewed Jul 4, 2020

View reviewed changes

contrib/devtools/previous_release.py Outdated Show resolved Hide resolved

contrib/devtools/previous_release.py Outdated Show resolved Hide resolved

bliotti and others added 2 commits July 5, 2020 04:04

script: Add previous_release.py

e1e5960

closes bitcoin#18132 added GPG verify for binaries co-authored-by: bboot <bboot@cisco.com>

Remove previous_release.sh

9c34aff

bliotti force-pushed the previous-release-py branch from 58b3fdf to 9c34aff Compare July 5, 2020 08:10

jnewbery removed the Waiting for author label Jul 9, 2020

maflcko merged commit ea595d3 into bitcoin:master Jul 21, 2020

hebasto mentioned this pull request Aug 26, 2020

util, ci: Hard code previous release tarball checksums #19813

Merged

bliotti mentioned this pull request Nov 27, 2021

Bitcoin 😻 Conf 2022 🧑🏽‍💻 BTCMedia/B24-Open-Source-Program#4

Open

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

script: previous_release.sh rewritten in python #19205

script: previous_release.sh rewritten in python #19205

bliotti commented Jun 8, 2020 •

edited by fanquake

Loading

Sjors left a comment

practicalswift commented Jun 8, 2020

bliotti commented Jun 8, 2020

bliotti commented Jun 8, 2020

Sjors left a comment

fjahr commented Jun 18, 2020

fjahr left a comment •

edited

Loading

DrahtBot commented Jul 2, 2020 •

edited

Loading

fjahr commented Jul 4, 2020

fjahr commented Jul 5, 2020 •

edited

Loading

bliotti commented Jul 5, 2020

Sjors commented Jul 9, 2020

bliotti commented Jul 20, 2020

fjahr commented Jul 20, 2020

jonatack commented Jul 20, 2020

emsheets commented Jul 20, 2020

maflcko commented Jul 21, 2020

bliotti commented Jul 21, 2020

hebasto commented Aug 26, 2020

script: previous_release.sh rewritten in python #19205

script: previous_release.sh rewritten in python #19205

Conversation

bliotti commented Jun 8, 2020 • edited by fanquake Loading

Sjors left a comment

Choose a reason for hiding this comment

practicalswift commented Jun 8, 2020

bliotti commented Jun 8, 2020

bliotti commented Jun 8, 2020

Sjors left a comment

Choose a reason for hiding this comment

fjahr commented Jun 18, 2020

fjahr left a comment • edited Loading

Choose a reason for hiding this comment

DrahtBot commented Jul 2, 2020 • edited Loading

Conflicts

fjahr commented Jul 4, 2020

fjahr commented Jul 5, 2020 • edited Loading

bliotti commented Jul 5, 2020

Sjors commented Jul 9, 2020

bliotti commented Jul 20, 2020

fjahr commented Jul 20, 2020

jonatack commented Jul 20, 2020

emsheets commented Jul 20, 2020

maflcko commented Jul 21, 2020

bliotti commented Jul 21, 2020

hebasto commented Aug 26, 2020

bliotti commented Jun 8, 2020 •

edited by fanquake

Loading

fjahr left a comment •

edited

Loading

DrahtBot commented Jul 2, 2020 •

edited

Loading

fjahr commented Jul 5, 2020 •

edited

Loading