travis: enforce generated code, add 1.x #521
Conversation
tamird
changed the title
|
|
||
| env: | ||
| - PATH=$HOME/protoc/bin:$PATH | ||
|
|
||
| script: | ||
| - make all test | ||
| - make all | ||
| - make regenerate |
There was a problem hiding this comment.
This doesn't seem right--if we run regenerate before test, the golden tests will always pass even if something has unexpectedly changed.
There was a problem hiding this comment.
I'm happy to rearrange this, but note that if anything at all changes as a result of make regenerate, the stanza immediately following will fail.
There was a problem hiding this comment.
Ahh, I see what you're doing; I hadn't quite followed the purpose of that stanza.
FWIW, the tests in protoc-gen-go should already pass independent of the Go version (they ignore the compressed portion of the generated files). Would it make more sense to fix any of the tests which don't have that behavior than to change the Travis config?
There was a problem hiding this comment.
FWIW, the tests in protoc-gen-go should already pass independent of the Go version (they ignore the compressed portion of the generated files). Would it make more sense to fix any of the tests which don't have that behavior than to change the Travis config?
Again, while those tests might pass, the files will have changed and the aforementioned stanza will fail.
There was a problem hiding this comment.
Also note that no tests are being skipped here; make test is run unconditionally.
There was a problem hiding this comment.
I guess my question is: If this is doing something that make test doesn't already do, should we just fix make test?
("Fixing make test is hard" is a valid answer. :) )
|
It's probably not appropriate to pull this into `make test`; doing so will
mean that `make test` will fail any time there are uncommitted changes,
which is basically always during development.
…On Feb 21, 2018 12:34, "Damien Neil" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .travis.yml
<#521 (comment)>:
>
env:
- PATH=$HOME/protoc/bin:$PATH
script:
- - make all test
+ - make all
+ - make regenerate
I guess my question is: If this is doing something that make test doesn't
already do, should we just fix make test?
("Fixing make test is hard" is a valid answer. :) )
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#521 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPJIKX_FarQpfIpiWO6D_nM1oNbHdks5tXFOngaJpZM4SN0dN>
.
|
.travis.yml
Outdated
| # | ||
| # Since we test against versions older than 1.8, we limit the check that | ||
| # generated files are stable to go versions higher than 1.7. | ||
| - if (( $(cut -f2 -d. <<< $TRAVIS_GO_VERSION) > 7 )); then |
There was a problem hiding this comment.
Black-listing Go1.7 and below is the wrong approach IMO.
We should enable this check only for a single version of Go (e.g., Go1.10). Thus, we wouldn't care if compression changed from Go1.7 to 1.8 or changes again in the future, or something elses changes like gofmt.
At some point in time, we can bump it to Go1.11 and regenerate any differing files as appropriate.
This does require that all developers a regenerating using the same version of Go, and that seems to be true already.
There was a problem hiding this comment.
@dsnet if that's your preference, I am happy to make the change. My motivation in blacklisting in this way was to avoid rot on future Go versions being added - it would be a shame for issues with new Go versions to go unnoticed until some user tries to build this package and finds oddities. I'll wait for your response.
There was a problem hiding this comment.
I understand the risk, but I think it's minimal.
- Suppose we pin the generated test files to Go1.10.
- At some point in the future, Go1.11 is released, and developers like you and me start to use Go1.11 on our machines.
- We work on some protobuf change send it out for review.
- The CI tool complains to us that the generated files mismatch. At that point in time, it will be a signal that we should bump the pinned version from Go1.10 to Go1.11.
That being said, I filed #524 to make it even more obvious why the output mismatches.
There was a problem hiding this comment.
@dsnet another risk is the release of go1.10.1 -- if we do an exact version comparison, this may break. The only differences between my approach and yours are:
- mine is slightly less fragile (as explained in the previous sentence)
- step 3 above is not required - whether or not developers of protobuf are using the new version of Go, if that version introduces changes in behaviour, CI will complain
The second bullet above seems better to me, since it pushes this burden more on the developers of this package (and thus away from its users).
There was a problem hiding this comment.
release of go1.10.1 -- if we do an exact version comparison, this may break.
I think that's okay:
- Minor releases of Go don't happen often
- Contributors are typically the type of people eager to use the latest Go toolchain
- The number of contributors is small (even smaller for just those working on protoc-gen-go), so requiring them all to use the same go toolchain is fairly tenable
- Even minor releases can change the output. IIRC, I submitted a fix for one of the point releases to fix a correctness regression in
compress/flate.
The second bullet above seems better to me, since it pushes this burden more on the developers of this package (and thus away from its users).
I'm not sure I understand. Anything in the .travis.yml file is for developers and not for users. So I'm not sure any of this matters for the vast majority of people out there.
There was a problem hiding this comment.
OK, changed to look for 1.10* with a TODO to do something better when gimme improves.
Update protoc to v3.5.1 while I'm here.
|
@dsnet anything else you'd like to see here? |
|
LGTM. Leaving for @neild, for whether he is still concerned that |
Update protoc to v3.5.1 while I'm here.
Spiritual successor to #37.