When previous releases are fetched and the specified version wasn't added to the checksum list we used to get a "Checksum did not match" which isn't true (https://github.com/bitcoin-core/bitcoincore.org/issues/753#issuecomment-879546719).
If the specified version number is not on the list, it now logs cannot do the comparison instead.
Concept ACK, will test later
103 | @@ -104,7 +104,12 @@ def download_binary(tag, args) -> int: 104 | tarballHash = hasher.hexdigest() 105 | 106 | if tarballHash not in SHA256_SUMS or SHA256_SUMS[tarballHash] != tarball: 107 | - print("Checksum did not match") 108 | + if sum_exists(tag[1:]): 109 | + print("Checksum did not match") 110 | + return 1 111 | + 112 | + print("Cannot compare checksum for version {version}".format(
To me it seems like the new message isn't explaining the failure reason clearly.
Maybe a "Checksum for given version doesn't exist" or something like that could help more?
Agree, my wording wasn't the best! Thank you for the suggestion. Updated.
Concept ACK with an error message comment.
122 | @@ -119,6 +123,13 @@ def download_binary(tag, args) -> int: 123 | return 0 124 | 125 | 126 | +def checksum_exists(version) -> bool: 127 | + for sum in SHA256_SUMS: 128 | + if "bitcoin-" + version in SHA256_SUMS[sum]: 129 | + return True 130 | + return False
I think this could be simplified to a one-liner (maybe it could directly be used above then instead of introducing an extra function):
return "bitcoin-" + version in SHA256_SUMS.values()
Looking a second time at it, that was too simple-thought; it should be more like
return any("bitcoin-" + version in str for str in SHA256_SUMS.values())
Yes absolutely! My python is a little bit rusty it's been a while. I didn't know I could do that, it looks cleaner. Thank you!
103 | @@ -104,7 +104,11 @@ def download_binary(tag, args) -> int: 104 | tarballHash = hasher.hexdigest() 105 | 106 | if tarballHash not in SHA256_SUMS or SHA256_SUMS[tarballHash] != tarball: 107 | - print("Checksum did not match") 108 | + if checksum_exists(tag[1:]):
nit: could also pass the full filename (tarball) here, then the function checksum_exists below wouldn't need to add the "bitcoin-" prefix and is more specific?
I purposely avoided passing the full name in case the naming convention changes it will still work. Thinking more about it would be helpful to check the full name as it will throw the checksum doesn't exist if we are missing a specific OS/arch on the list even if we included some for that same version.
Left two possible improvements that came up while reviewing the code. Couldn't test yet (the machine I am using right now is running OpenBSD, where the script doesn't work; even if the platform is overrided to e.g. 'x86_64-linux-gnu', the tar command fails), but will do tomorrow on a Linux machine.
even if the platform is overrided to e.g. 'x86_64-linux-gnu', the tar command fails),
That sounds like something we should be able to fixup.
If a requested version doesn't exist on our list of checksums it says it cannot do a checksum comparision instead of saying it did not match
tACK 179a051704321ba40277a5855d6ac0dbb45689dd, tested on Debian bullseye/sid
master branch:
$ ./test/get_previous_releases.py -b v0.21.0
Releases directory: releases
Fetching: https://bitcoincore.org/bin/bitcoin-core-0.21.0/bitcoin-0.21.0-x86_64-linux-gnu.tar.gz
[ ... ]
Checksum did not match
$ echo $?
1
PR branch:
$ ./test/get_previous_releases.py -b v0.21.0
Releases directory: releases
Fetching: https://bitcoincore.org/bin/bitcoin-core-0.21.0/bitcoin-0.21.0-x86_64-linux-gnu.tar.gz
[ ... ]
Checksum for given version doesn't exist
$ echo $?
1
By the way, welcome as a new contributor NelsonGaldeman! 🎉
even if the platform is overrided to e.g. 'x86_64-linux-gnu', the tar command fails),
That sounds like something we should be able to fixup.
Yes, will take a look into that.
cr ACK 179a051704321ba40277a5855d6ac0dbb45689dd
Warm welcome as a new contributor @NelsonGaldeman! 🥇
