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

Upgrade v4.10.1 => v4.11.0 makes dependency graph bloated #977

Open
inliquid opened this issue Mar 26, 2021 · 7 comments
Open

Upgrade v4.10.1 => v4.11.0 makes dependency graph bloated #977

inliquid opened this issue Mar 26, 2021 · 7 comments

Comments

@inliquid
Copy link

Hi,

have no idea what was in latest update (I'm using GORM with transitive dependency through gorm.io/driver/postgres), but when I updated to latest version (v4.10.1 => v4.11.0), my dependency tree becomes over bloated:

Before

go mod graph | wc -l
553

After

go mod graph | wc -l
1256

Just with this single line:

github.com/jackc/pgx/v4 v4.11.0 // indirect
@pjediny
Copy link
Contributor

pjediny commented Mar 27, 2021

I think the problem is the go-kit, not sure why should a db driver use a micro-service framework, in my case it broke build because of the thrift dependency that is incompatible with cadence. Looking at the code, I see it is because of the logger adapter implementation. In my opinion logger adapters should go out of the repo, or to some other (sub)module, maybe with the minimal adapter (to standard logger) and interface definition staying in this repo.

IIRC, the go-kit logger itself is an abstraction to other (structured) loggers, like zip and logrus, so we have direct adapters implemented here, indirect adapters too and too many of hard dependencies. Maybe author should consider to remove them completely, as they are trivial to implement and the provided implementations are not flexible enough.

@Juneezee
Copy link

Juneezee commented Mar 28, 2021

Could it be caused by this commit 9b59dd0?

EDIT: I agree with @pjediny. We can make pgx lighter by moving logger implementations to another repository. Furthermore, it is also very likely that someone using the logger implementation will only use one specified logger, for example zerolog. So there is no reason to include zap, go-kit, logrus if they are not used. By doing this, we can remove 5 logger dependencies from pgx.

@jackc
Copy link
Owner

jackc commented Apr 3, 2021

Including log adapters and other optional code in the main repo but in separate packages predates Go modules.

It seems really unfortunate that optional and test dependencies are triggering this behavior (pgtype also has this issue jackc/pgtype#97).

Is there no way around this outside of splitting the repositories? That would technically require a major version release... Not thrilled about inflicting that on everyone.

Also, pgx is already split into several modules (pgx, pgconn, pgtype, pgproto3, puddle, etc.). This already makes testing, changelogs, and releases more awkward than it would be as a monorepo. Not excited about making that even worse either.

@Juneezee
Copy link

Juneezee commented Apr 3, 2021

@jackc I think it's all about trade-offs. In my opinion, the benefit of slimming down the dependency tree outweights the drawbacks. Just like what @pjediny has suggested, the base Logger interface definition can stay in this repo.

Also just a small suggestion, you could consider creating an organization on GitHub and move the pgx family into it just to simplify the maintenance a little bit.

@inliquid
Copy link
Author

inliquid commented Apr 7, 2021

The workaround at this moment would be to get rid of go-kit adapter and leave others. This "logger" package is just bad, so if anyone wanted to use it he can do it by himself without affecting everyone else.

@james-lawrence
Copy link
Contributor

james-lawrence commented Apr 10, 2021

if its really just the logger causing the issue, then I'd just drop the adapters and let people roll their own repos for that. just have the interface defined here.

honestly most of those logger frameworks are overengineered anyways.

@connyay
Copy link

connyay commented Apr 12, 2021

Is there no way around this outside of splitting the repositories? That would technically require a major version release... Not thrilled about inflicting that on everyone.

Rather than splitting the repo the various adapters could be their own modules. Adding go modules adds a bit of complications for maintainers (ensuring the adapter pkgs reference the correct version of jackc/pgx, ensuring no cyclic deps, releases are complicated, etc) but reduces bloat on consumers.

IDEs historically have handled multi go module repos poorly, but have had recent improvements where things are tolerable.

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

6 participants