-
Notifications
You must be signed in to change notification settings - Fork 85
Ensure that we use correct version of protoc and its deps #233
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
Conversation
576fde9 to
e2cbd88
Compare
@marquiz No, I think this is better this way, as a single PR split to commits. These form a coherent collection which together make sense. The one question I have (or maybe two or three) is, do we really need to change the tool versions we install and use, do we really need to change them as part of this PR, and if we change them as part of this PR, why don't we sync them with current containerd main/HEAD which is what we have deliberately been doing so far in NRI ? |
We don't change any tools versions, except for protoc-gen-go which we cannot avoid as now the version comes from go.mod where we already have google.golang.org/protobuf v1.34.1. You can see from the last commit that the effect this is ~nil |
@marquiz Okay, so sorry for my stupid questions which I'm asking since I have never tried to manage toolchain dependencies using the |
Ah okay, so we need to pass the desired version to |
e2cbd88 to
a6d40a3
Compare
Yes. BUT, if we now did |
Makes it possible to cleanly and reliably wipe protoc and it's dependencies without affecting other tools that might get installed under the build/tools dir in the future. Signed-off-by: Markus Lehtonen <[email protected]>
Check the protoc version. Wipe the old version and install the new one if correct version not found. Signed-off-by: Markus Lehtonen <[email protected]>
Maintain the versions in go.mod. Signed-off-by: Markus Lehtonen <[email protected]>
Signed-off-by: Markus Lehtonen <[email protected]>
Signed-off-by: Markus Lehtonen <[email protected]>
a6d40a3 to
0f331d5
Compare
|
The PR is now rebased and updated:
EDIT: #232 is merged so ready for review @klihub @chrishenzie @mikebrow |
mikebrow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM nit.. squash :-)
Split into multiple self-contained commits.
Makes it possible to cleanly and reliably wipe protoc and it's dependencies without affecting other tools that might get installed under the build/tools dir in the future.
Maintain the versions in go.mod.
NOTE: Could be submitted as multiple smaller PRs, too, that feels better