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

Re-vendor dependencies #89

Merged
merged 2 commits into from
Dec 7, 2020
Merged

Conversation

kevpar
Copy link

@kevpar kevpar commented Dec 7, 2020

Signed-off-by: Kevin Parsons [email protected]

@kevpar
Copy link
Author

kevpar commented Dec 7, 2020

This pulls in a new version of github.com/containerd/ttrpc from a fork
to fix the deadlock issue in containerd/ttrpc#72. Will revert back to
the upstream ttrpc vendor once the fix is merged (containerd/ttrpc#73).

This fix also included some vendoring cleanup from running "vndr".

Signed-off-by: Kevin Parsons <[email protected]>
@kevpar kevpar force-pushed the ttrpc-deadlock-fix branch from 9da51a7 to 16caa11 Compare December 7, 2020 20:50
Copy link

@anmaxvl anmaxvl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I assume that extra changes being vendored in are OK?

@kevpar
Copy link
Author

kevpar commented Dec 7, 2020

LGTM. I assume that extra changes being vendored in are OK?

Yes, that's just some vendor cleanup that was performed automatically.

@dcantah
Copy link

dcantah commented Dec 7, 2020

Are we supposed to focus our reviewing efforts of the deadlock ttrpc changes here or the upstream PR? I take it this is a "this fixes the issue and barring any critical flaws we saw, just want a set of eyes on the vendored files", am I off?

@kevpar
Copy link
Author

kevpar commented Dec 7, 2020

Are we supposed to focus our reviewing efforts of the deadlock ttrpc changes here or the upstream PR? I take it this is a "this fixes the issue and barring any critical flaws we saw, just want a set of eyes on the vendored files", am I off?

Mostly need focus on the upstream ttrpc PR. Comments here would be good if there are any concerns unique to this PR, such as from the vendoring or other ttrpc changes being brought in.

Copy link

@dcantah dcantah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevpar Gotcha. LGTM then vendor wise

@kevpar
Copy link
Author

kevpar commented Dec 7, 2020

Re-purposing this to be a general revendor PR and pulling in one more change. Will need another look in a moment.

@kevpar kevpar changed the title Update ttrpc vendor to forked version to fix deadlock Re-vendor dependencies Dec 7, 2020
Copy link

@dcantah dcantah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants