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

Subpackage version imports repo client_golang which imports this repo… #223

Open
puellanivis opened this issue Feb 18, 2020 · 8 comments

Comments

@puellanivis
Copy link

This does not cause an import cycle while compiling, so no one’s builds have broken, and go does a decent job at caching the go.mod downloads, so no one has really noticed the stress yet. But just importing the most recent version of client_golang (v1.4.1) ends up cascading through three different versions of client_golang and two versions of common.

https://github.com/prometheus/client_golang/blob/v1.4.1/go.mod#L11 depends on common v0.9.1
https://github.com/prometheus/common/blob/v0.9.1/go.mod#L13 depends on client_golang v1.0.0
https://github.com/prometheus/client_golang/blob/v1.0.0/go.mod#L10 depends on common v0.4.1
https://github.com/prometheus/common/blob/v0.4.1/go.mod#L17 depends on client_golang v0.9.1
https://github.com/prometheus/client_golang/blob/v0.9.1 has no go.mod so this is the end of the line.

As interdependent versions progress, this cyclic dependency is likely to only grow longer and longer, potentially exponentially exploding the results of go mod graph in turn. It might be a good idea to consider how to deal with it now, rather than later.

@brian-brazil
Copy link
Contributor

See prometheus/client_golang#701

@beorn7
Copy link
Member

beorn7 commented Feb 18, 2020

It would be interesting to know if this actually causes problems in practice. As concluded in prometheus/client_golang#701 (comment) , it looks like the behavior is by design and not really problematic.

If it ever grows into an actual problem, we already know the solution: Replicate https://github.com/prometheus/common/blob/master/version/info.go (or move it to another module).

@puellanivis
Copy link
Author

So, the chief problem that I’ve seen is that just by importing client_golang the go mod graph becomes so awash in noise that it is basically unusable without extensive replication of logic from go mod to resolve which version of a package actually gets brought in, and then ignoring the others. Sure the resolution logic is not SAT, but it’s still not really trivial.

That the go mod dependency solver handles this reasonably well so far, does not mean that it could not still be seen as pathological behavior, whether now or in the future. Often in CIs and Dockerfiles, people are building in environments without persistent caches and end up having to repeatedly resolve the dependency chain, which now when it’s only the 3 version is not so bad, but imagine when multiple interdependent packages are stairstepping down say 10 or 30 different versions each in the future? And then with enough of those versions pointing to different versions of even non-cyclic packages, you can end up with an extraordinary number of versions for even those packages, perhaps even some that will have never have been actually depended upon for years, but keep getting dragged in as part of the eternal dependency chain. (Example, imagine that 10 years from now, people still need to resolve github.com/pkg/[email protected].)

So, sure, go mod uses a polynomial algorithm over the NP-complete SAT algorithm, but 🤷‍♀️ one should probably not treat it like it cannot be overloaded. Perhaps like Kessler Syndrome, it might not ever actually be a problem, until it is an insurmountable problem?

But of course moving the version package to client_golang would require removing the package from common, which would be a breaking change, which by semver, technically should be behind a major version bump. This is not an insurmountable issue, but it is not really trivial. So… 😢 I don’t know if there really is any good solution here.

@brian-brazil
Copy link
Contributor

which would be a breaking change, which by semver

This is internal-only code, and we're not 1.0 so no need to worry about semver. The issue is more that version does belong in this repo as it's not a public API. Appropriate fixes to this have been discussed in the issue I linked, but none are trivial.

@puellanivis
Copy link
Author

The issue is more that version does belong in this repo as it’s not a public API.

Unfortunately, while that might be the intent, it is being used publicly: https://github.com/search?l=Go&q=%22version.NewCollector%22&type=Code

@brian-brazil
Copy link
Contributor

If developers have chosen to use an internal API that's their choice, it's not much use without our (regularly changing) build infrastructure though.

@ilius
Copy link
Contributor

ilius commented Jul 14, 2024

I removed the dependency in Go code in #662.
But go mod tidy still adds it back to go.mod
I think it should be possible to remove it after the next tag.

@SuperQ
Copy link
Member

SuperQ commented Jul 14, 2024

The issue is a recursive dependency between common and client_golang. We broke the loop but now need to release client_golang with those changes.

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

No branches or pull requests

5 participants