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

depends: update the comparison tool to a more recent version #5006

Merged
merged 2 commits into from
Sep 30, 2014

Conversation

TheBlueMatt
Copy link
Contributor

Updated version of #4837

@TheBlueMatt TheBlueMatt force-pushed the travis-new-comparisontool branch 2 times, most recently from a3c09fa to 6056fa0 Compare September 30, 2014 05:51
@theuni
Copy link
Member

theuni commented Sep 30, 2014

Great, thanks for doing this. What's the current status of it, though? Is this just an updated snapshot, or does it include some of the more extensive tests that have been discussed lately?

Also, it's going to need a distinct file-name, otherwise this will clash with the next update. Could you please append the upstream revision to the jar's filename, so that we can keep track of what this build corresponds to? Notice how #4837 used pull-tests-$($(package)_version).jar for the file-name, which addresses both concerns at once.

@TheBlueMatt
Copy link
Contributor Author

This version supports headers-first with one slight tweak to sipa's current branch. Note the download link includes the commithash to avoid update conflicts.

@theuni
Copy link
Member

theuni commented Sep 30, 2014

That won't help here, though. Next update, it'll see that it has already fetched pull-tests.jar, so it will skip it this time. Then the checksum will fail.

There's no indication of what upstream revision this corresponds to, so we're in the same position as current pull-tester (blind). Even if it's some random commit on one of your personal branches, please make a note of what this corresponds to. The filename is the most obvious place for that.

@TheBlueMatt
Copy link
Contributor Author

Fixed.

@theuni
Copy link
Member

theuni commented Sep 30, 2014

@TheBlueMatt I realize it sounds nitpicky, but please build the filename from the version string. It's used to calculate the dependency chain. With any other dep/lib it makes sense, but this one's a bit weird since it's not a normal source release. This should work:

$(package)_version=adfd3de7
$(package)_download_path=https://github.com/TheBlueMatt/test-scripts/raw/10222bfdace65a0c5f3bd4a766eeb6b3a8b869fb/
$(package)_file_name=pull-tests-$($(package)_version).jar

You can also use the branch name since filenames will be unique, that way bumping the version/hash is all it takes to update in the future:

$(package)_download_path=https://github.com/TheBlueMatt/test-scripts/raw/master

But that's up to you.

After that, ACK from me on the deps side of things, but the actual behavior changes still need review. paging @sipa

@TheBlueMatt
Copy link
Contributor Author

Fixed.

@TheBlueMatt
Copy link
Contributor Author

(I'd prefer to be able to clear random crap from master, so I'm gonna avoid using master for the download path for now)

@theuni
Copy link
Member

theuni commented Sep 30, 2014

Thanks. Looks good now.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p5006_e166c177bccdaf5b3c1b2238e8e04d53554d138e/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@sipa
Copy link
Member

sipa commented Sep 30, 2014

Tested ACK

@sipa
Copy link
Member

sipa commented Sep 30, 2014

This was tested using #5011, #5012 and #5009.

@theuni
Copy link
Member

theuni commented Sep 30, 2014

ACK

gavinandresen added a commit that referenced this pull request Sep 30, 2014
depends: update the comparison tool to a more recent version
@gavinandresen gavinandresen merged commit 7def85e into bitcoin:master Sep 30, 2014
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants