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

downgrade go-difflib and go-spew to tagged releases #5654

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jan 14, 2025

These dependencies were updated to "master" in some modules we depend on, but have no code-changes since their last release. Unfortunately, this also causes a ripple effect, forcing all users of the containerd module to also update these dependencies to an unrelease / un-tagged version.

Both these dependencies will unlikely do a new release in the near future, so exclude these versions so that we can downgrade to the current release.

For additional details, see this PR and links mentioned in it.

@thaJeztah thaJeztah self-assigned this Jan 14, 2025
@github-actions github-actions bot added the area/dependencies Pull requests that update a dependency file label Jan 14, 2025
go.mod Outdated
Comment on lines 188 to 203
exclude (
// These dependencies were updated to "master" in some modules we depend on,
// but have no code-changes since their last release. Unfortunately, this also
// causes a ripple effect, forcing all users of the containerd module to also
// update these dependencies to an unrelease / un-tagged version.
//
// Both these dependencies will unlikely do a new release in the near future,
// so exclude these versions so that we can downgrade to the current release.
//
// For additional details, see this PR and links mentioned in that PR:
// https://github.com/kubernetes-sigs/kustomize/pull/5830#issuecomment-2569960859
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2
)
Copy link
Member

Choose a reason for hiding this comment

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

iiuc we need to keep these exclusions until spf13/viper#1861 is part of a tagged release?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, and until other indirect dependencies are updated (I was hoping the patch in containerd (containerd/containerd#11222) would help, but the nydus-snapshotter and stargz-snapshotter modules still have the wrong version and are a direct dependency of BuildKit, so those now push them to the untagged versions.

@@ -184,3 +184,18 @@ require (
tags.cncf.io/container-device-interface v0.8.0 // indirect
tags.cncf.io/container-device-interface/specs-go v0.8.0 // indirect
)

exclude (
// These dependencies were updated to "master" in some modules we depend on,
Copy link
Member

Choose a reason for hiding this comment

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

what modules? (I guess nydus-snapshotter and stargz-snapshotter but not sure)

Copy link
Member Author

@thaJeztah thaJeztah Jan 14, 2025

Choose a reason for hiding this comment

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

spf13/viper was the initial one, but that made it through to kustomize, kustomize to kubernetes, kubernetes to containerd, containerd to stargz

Copy link
Member

Choose a reason for hiding this comment

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

If it is already in containerd then what is it breaking for "all users of the containerd module" ? Or doesn't this need to go to containerd's go.mod then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying to break the cycle; currently all users of buildkit and moby will be forced to update the dependency to a version that will never be released.

Copy link
Member

Choose a reason for hiding this comment

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

hmm, I'm looking at containerd v2.0.2 (vendored in buildkit) and it already has this same patch containerd/containerd@f341477 (from you). So the newer versions are not breaking anything for buildkit itself, but afaics your claim is that they may break something for someone importing buildkit (and through it containerd). But if these exclude rules do not carry over to the caller (as they have not carried from containerd to buildkit) then any caller would need to add their own exclude rules anyway. If that's the case then what's the improvement from buildkit adding these extra rules.

Copy link
Member Author

Choose a reason for hiding this comment

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

s/archived/ or marked "unmaintained"

Copy link
Member

Choose a reason for hiding this comment

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

Are these excludes more "dependency hygiene" than fixing an actual issue? I'm trying to figure out if we are going to keep these excludes "forever".

Copy link
Member

Choose a reason for hiding this comment

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

(Assuming exclude propagates but gets overwritten by other deps)

Can we add a comment that these are downgraded in containerd 2.0.2 but because nydus-snapshotter, stargz-snapshotter do not depend on 2.0.2 yet that downgrade does not work unless we make one explicitly.

Copy link
Contributor

@slonopotamus slonopotamus Jan 14, 2025

Choose a reason for hiding this comment

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

There is no particular issue. The world still exists, the wheels keep rolling.

I look at go.mod and see a noticeable amount of untagged deps. But for some reason, @thaJeztah cares a lot more about these two than about others.

I'm trying to figure out if we are going to keep these excludes "forever".

@thaJeztah hopes he can stop the plague by quick downgrade of every place where these deps already got (though nothing prevents this from happening again).

To be honest, I'd declare a dep with last commit in 2018 as dead and requiring a complete replacement.

Copy link
Member Author

Choose a reason for hiding this comment

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

The other untagged versions don't tag releases, so there's no release to choose from.

These dependencies were updated to "master" in some modules we depend on,
but have no code-changes since their last release. Unfortunately, this also
causes a ripple effect, forcing all users of the containerd module to also
update these dependencies to an unrelease / un-tagged version.

Both these dependencies will unlikely do a new release in the near future,
so exclude these versions so that we can downgrade to the current release.

For additional details, see [this PR][1] and links mentioned in it.

[1]: kubernetes-sigs/kustomize#5830 (comment)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the downgrade_tagged_releases branch from caa0299 to 60b4760 Compare January 14, 2025 19:00
@tonistiigi tonistiigi merged commit dd06922 into moby:master Jan 14, 2025
96 checks passed
@thaJeztah thaJeztah deleted the downgrade_tagged_releases branch January 14, 2025 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants