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

New chmduquesne/rollinghash breaks tests #5334

Closed
aviau opened this Issue Nov 21, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@aviau
Copy link
Member

aviau commented Nov 21, 2018

Hello!

Apparently we have a newer version of rollinghash in Debian (4.0.0) and it broke syncthing tests:

ok  	github.com/syncthing/syncthing/lib/relay/protocol	0.281s [no tests to run]
=== RUN   TestBlocks
--- PASS: TestBlocks (0.00s)
=== RUN   TestAdler32Variants
--- FAIL: TestAdler32Variants (0.01s)
	blocks_test.go:160: At i=128, sum2=5ea93e99, sum3=5ea93e99
	blocks_test.go:160: At i=256, sum2=2cf17ba3, sum3=80683d0b
	blocks_test.go:162: Mismatch after roll; i=256, sum2=2cf17ba3, sum3=80683d0b

This may be a misdiagnosis, I have only spent little time on this for now.

Let me know if you think the problem is related or not.

Cheers,

@calmh

This comment has been minimized.

Copy link
Member

calmh commented Nov 22, 2018

I looked at it and this looks very much like a test that should pass, and must pass for our rolling hash support to work. Spontaneously it looks like a regression in the rollinghash/adler32 package. If you can narrow down the breaking commit I can look closer at it.

@calmh calmh added the build label Nov 22, 2018

@calmh

This comment has been minimized.

Copy link
Member

calmh commented Nov 22, 2018

Oh, strike that. They changed the API:

Write has become fully consistent with hash.Hash. As opposed to previous versions, where writing data would reinitialize the window, it now appends this data to the existing window. In order to reset the window, one should instead use the Reset method.

We need to adapt when upgrading. Good thing the test caught it.

In the version we use, Write() is equivalent to Reset()+Write(). So we can add that and be compatible with both. This needs changing wherever we do writes, not just in the test, of course.

@calmh

This comment has been minimized.

Copy link
Member

calmh commented Nov 22, 2018

Oh, and we're in luck, because we already do that, just not in the test. So we can just update the test.

calmh added a commit to calmh/syncthing that referenced this issue Nov 22, 2018

lib/scanner, vendor: Update github.com/chmduquesne/rollinghash (fixes s…
…yncthing#5334)

Updates the package and fixes a test that depended on the old behavior
of Write() being equivalent to Reset()+Write() which is no longer the
case. The scanner already did resets after each block write, so this is
fine.

@calmh calmh changed the title new chmduquesne-rollinghash breaks syncthing New chmduquesne/rollinghash breaks tests Nov 22, 2018

@calmh calmh closed this in c0a26c9 Nov 22, 2018

@aviau

This comment has been minimized.

Copy link
Member

aviau commented Nov 22, 2018

thanks! 🎉

fragtion added a commit to fragtion/syncthing that referenced this issue Nov 27, 2018

lib/scanner, vendor: Update github.com/chmduquesne/rollinghash (fixes s…
…yncthing#5334) (syncthing#5335)

Updates the package and fixes a test that depended on the old behavior
of Write() being equivalent to Reset()+Write() which is no longer the
case. The scanner already did resets after each block write, so this is
fine.

fragtion added a commit to fragtion/syncthing that referenced this issue Nov 28, 2018

lib/scanner, vendor: Update github.com/chmduquesne/rollinghash (fixes s…
…yncthing#5334) (syncthing#5335)

Updates the package and fixes a test that depended on the old behavior
of Write() being equivalent to Reset()+Write() which is no longer the
case. The scanner already did resets after each block write, so this is
fine.

fragtion added a commit to fragtion/syncthing that referenced this issue Nov 28, 2018

lib/scanner, vendor: Update github.com/chmduquesne/rollinghash (fixes s…
…yncthing#5334) (syncthing#5335)

Updates the package and fixes a test that depended on the old behavior
of Write() being equivalent to Reset()+Write() which is no longer the
case. The scanner already did resets after each block write, so this is
fine.

@calmh calmh added this to the v0.14.54 milestone Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment