Repository navigation
perf(deps): let a build omit the datasource drivers and GraphQL engine it does not use - #4167
Conversation
7989966 to
a969749
Compare
|
A downstream data point for We run a GoFr service that uses no pub/sub at all. Because Last week that last one broke a deploy for us. Google moved the $ curl -s https://proxy.golang.org/github.com/googleapis/enterprise-certificate-proxy/@v/v0.3.19.mod | grep toolchain
toolchain go1.25.8
$ curl -s https://raw.githubusercontent.com/googleapis/enterprise-certificate-proxy/v0.3.19/go.mod | grep toolchain
toolchain go1.26.5Our None of that is GoFr's doing. The point is only where we happened to be standing when it landed: we absorbed it for a module we never import, reached solely through a pub/sub client we don't use, feeding a client-certificate feature we have never enabled — nothing in our tree, infra or workflows sets So the 213 packages For what it's worth, this also reads as a continuation of existing practice rather than a new idea: Happy to build our service against the tag and report back if that would be useful. |
|
Thanks, this is a useful data point, and yes please: building your service against this branch and reporting back would help a lot. The branch head is
One limit to be upfront about, since it bears directly on the failure you hit. I reproduced it against a minimal service on this branch:
The tag removes the code from the build graph, so it is no longer linked into or compiled into your binary. It does not remove it from the module graph. Removing it from the module graph as well would mean moving the pub/sub clients into their own modules, the way |
PiyushSingh-ZS
left a comment
PiyushSingh-ZS
left a comment
There was a problem hiding this comment.
The engineering here is careful and the measurements are worth having. Before line-level review, though, I think there is a mechanism question that maintainers should rule on, because it decides whether most of this diff is the right shape.
What is clearly right
The warning that the interface alone does nothing is the most valuable thing in the PR:
Extracting
graphQLRunnerleft all the graphql packages linked untilgraphql.goitself carried the tag -- the concrete file still compiles the library in.
That is the failure mode people ship by accident and then wonder why the binary did not shrink. Catching it and stating it plainly is a real contribution regardless of what happens to the rest.
The modernc.org/libc finding is also specific and verifiable: netdb init parsing embedded /etc/protocols and /etc/services into permanent Go structs, retained for the life of the process, unreachable from the HTTP path.
The mechanism question: build tags, or the pattern the repo already has?
GoFr already ships 25 nested go.mod modules under pkg/gofr/datasource/ — mongo, cassandra, clickhouse, scylladb, elasticsearch, oracle, surrealdb, dgraph, file/s3, file/gcs, kv-store/badger, and directly relevant here: pubsub/nats, pubsub/sqs, pubsub/eventhub.
Three pub/sub backends are already separate modules. Kafka, Google and MQTT are the outliers that stayed in the root module. So for gofr_nopubsub — by some distance the largest win in this PR, 213 packages and 8.9 MB — there is an existing, proven, zero-build-tag answer: extract pubsub/kafka, pubsub/google and pubsub/mqtt into their own modules, exactly like pubsub/nats.
| separate module | build tag | |
|---|---|---|
| Default build | unchanged | unchanged |
| Opt-in | go get + wire it |
remember -tags at every build site |
| Wrong config | compile error at the wiring site | runtime ERROR, service starts degraded |
| CI | ordinary go build ./... |
N-way tag matrix |
| Coverage gate | ordinary | tag-only files invisible to the default run |
go doc |
one surface | differs per tag |
| Precedent in this repo | 25 | 0 |
There is also a distribution problem tags have and modules do not: a tag is set by whoever runs go build, not by the dependency graph. A user building through Docker, ko, goreleaser, Bazel or an inherited Makefile has to thread -tags gofr_nopubsub,gofr_nosqldrivers,gofr_nographql through every one of those. Miss it in a single place and the binary silently reverts to 57 MB, with nothing to indicate it. A module boundary cannot be forgotten.
I do not think that argument is fatal to the whole PR — GraphQL lives in package gofr itself and the SQL drivers are blank imports for database/sql registration, so neither moves to a module easily, and a tag may genuinely be the only option for those two. But those are also the two smallest wins (1.4 MB and 6.1 MB against pub/sub's 8.9 MB). It would be worth splitting the pub/sub half out and asking whether it should be modules instead.
If tags are the agreed mechanism, notes on the implementation
1. The tagged build never exercises the tagged behavior for SQL.
drivers_testdeps_test.go blank-imports lib/pq and modernc.org/sqlite for tests, so under -tags gofr_nosqldrivers the test binary still has both drivers registered. The comment explains why and the reasoning is fair. But the consequence is that the documented behavior —
DB_DIALECT=postgresor=sqlitethen fails at startup: NewSQL'sregisterOtelcall reportsdatabase/sql's own "unknown driver" error naming the dialect, and returns a nil DB.
— is never tested in any configuration. One test that opens an unregistered dialect directly would cover it without disturbing the rest of the suite.
2. Coverage. Roughly half the added code (*_disabled.go) compiles only under a tag, so the code-climate gate CONTRIBUTING relies on will never see it. The Slim Build Tags job is the right mitigation — worth saying so in the PR description so the coverage delta is not read as a regression by a reviewer who has not spotted the job.
3. enabled() as an interface method. disabledGraphQL.buildSchema() returns nil and GetHandler() returns nil purely because enabled() guards them, which leaves two methods whose contract is "never call me". A const graphQLLinked = false — mirroring pubsubBackendsLinked, which this PR already introduces for pub/sub — would be consistent with the PR's own vocabulary and would let the compiler drop the dead branches outright.
4. The noopResponder relocation rationale is correct and non-obvious (responder.go cannot carry the tag because it holds the exported Responder interface, and leaving noopResponder there makes it unused under the tag and fails the lint gate). Worth keeping that comment exactly as written — it will save someone an afternoon.
5. createMqttPubSub returning an untyped nil in the disabled stub is clean, and composes correctly with #4164, which switches the container's pub/sub guard to isNil. Might be worth cross-linking the two.
Verifying identical go doc -all across 6 packages and an identical linked-package list for the default build is exactly the assertion this class of change needs.
Umang01-hash
left a comment
Umang01-hash
left a comment
There was a problem hiding this comment.
Verified end-to-end at the head SHA in a clean worktree: each tag drops its tree completely (graphql→0, sqldrivers→0, pubsub→0 syms), the full set takes a real binary 56.3MB→39.6MB (−16.7MB/~30%), no API break, no typed-nil, fail-loud confirmed by actually running tagged binaries (PUBSUB_BACKEND/DB_DIALECT → ERROR naming the tag, nil datasource, service still boots). Clean, effective work — the interface seams and verbatim moves are well done, and CI building the all-3 combination is the right call.
Requesting changes on disabled-path test coverage + one overstated CI claim — same pattern flagged on #4168, plus two gaps unique here:
- The "Default build links the same packages as before" step doesn't assert that — it only compiles + tests. A
go list -depsdiff (or dep-count check) vs base would make the step earn its name and guard the whole PR's value. - The SQL disabled fail-loud path (unknown-driver → nil DB) has no test, and
drivers_testdeps_test.gore-registers pq+sqlite untagged, so CI actively masks the disabled state. GraphQL got a dedicated disabled test; SQL should have parity. - The pubsub disabled fail-loud (
pubsubDisabledMsg) is untested — a regression dropping the Errorf (silent no-publish) wouldn't fail CI.
Minor (non-blocking): the graphql_runner.go doc says the tag drops "1.4 MB"; the real measured drop is ~0.83 MB.
… it claims Review follow-ups on #4167. The CI step named "Default build links the same packages as before" only ran go build and go test, so it asserted nothing of the sort. It now diffs `go list -deps ./... | sort` against the merge base and fails on any change, which is the claim the tags rest on: 861 packages, identical, measured locally. The tagged build is also linted now. It was not before, and the first run of the new step found a real noctx failure in graphql_disabled_test.go that no job would ever have reported -- the same class of problem that prompted moving noopResponder and configTrue in the first place. Two fail-loud paths had no test. - The disabled pub/sub stubs log an error naming the tag, but container_test.go only skips under the tag, and a skip proves nothing about the stub. An edit dropping one Errorf would leave every job green while producing the exact failure the tag exists to avoid: a publisher that silently never publishes. pubsub_backends_disabled_test.go asserts all three, and fails when the Errorf is removed. - registerOtel's "unknown driver" error is what makes a misconfigured tagged build legible. It is asserted against a dialect that is registered in NEITHER build, not against postgres under the tag: drivers_testdeps_test.go blank-imports pq and sqlite so the suite behaves identically either way, which means a tagged assertion about postgres would test the fixture rather than the code. graphQLRunner.enabled() becomes `const graphQLLinked`, matching the pubsubBackendsLinked the pub/sub side already uses. Whether the engine is linked is a property of the build, not of an instance, and the compiler can fold the branch away. drivers_disabled.go named only postgres and sqlite; supabase and cockroachdb register under the postgres driver (sql.go:268), so the tag affects them too. Adds the user-facing documentation the tags had none of, with the numbers measured rather than asserted (829 packages by default, 617/785/809 per tag, 553 with all three), and a CONTRIBUTING note that these tags are a closed exception for subsystems already in core -- a new integration ships as its own module.
…rect the grpc claim Review follow-ups on #4168, plus the #4167 follow-ups merged in. Each tag was only ever built in isolation, so a symbol that resolves under one and breaks under two would ship green and fail only for the user who set both. CI now builds, vets and tests all six together -- verified locally: builds clean, and 412 packages against the default 829. The nil-guard on grpcSrv had no test. newGRPCRunner fails on an out-of-range GRPC_PORT and factory.go logs and continues, so App runs on with no gRPC server, and these four setters used to dereference the field blind -- a config typo turning into a nil-pointer panic in the user's own setup code. All four are asserted, since a later edit is as likely to reintroduce it in one of the others as in the one that was reported. Neutralising the guard makes the test panic. The claim that gofr_nootlp "is the tag that actually releases google.golang.org/grpc" was wrong, and measurably so. Against gofr.dev/pkg/gofr: gofr_nogrpc alone leaves 82 grpc packages, adding gofr_nootlp leaves 81, adding gofr_nodgraph still leaves 81, and only adding gofr_nopubsub reaches 0 -- the Google Pub/Sub client pins grpc through cloud.google.com/go. A shared dependency goes when its last importer does, which is a property of these tags worth documenting rather than a detail of this one. The slim-builds documentation added in #4167 covers all six tags accordingly, including the table of that measurement, and notes that gofr_nogrpc is the one tag that changes the API surface.
|
@PiyushSingh-ZS @Umang01-hash — everything addressed at Tags vs modules: these three are a closed exceptionAgreed that modules are the right pattern, and that's the rule from here on. These three tags are deliberately limited to subsystems already compiled into core, where moving them out breaks existing users:
A new integration has no such users, so there is nothing to break — it goes in its own module, which is the existing pattern. I've written that into @Umang01-hash's three threads — all real, all fixed1. The CI step didn't check what its name claimed. Correct, and it was the worst kind of comment: it asserted the PR's central premise and verified nothing. It now diffs 2. The unknown-driver path had no test, and your point about the scaffolding masking it is exactly right. That's why the assertion is against a dialect registered in neither build rather than 3. The disabled pub/sub stubs had no assertion. Also right — skips prove nothing about what the stub does, and a dropped Also found while fixing those
@PiyushSingh-ZS's implementation notes
DocumentationThere was none for any of these tags — not in the PR, |
- TestGraphQL_RequestErrors now calls graphqlManager.GetHandler().ServeHTTP instead of Handle, so it keeps compiling once App.graphqlManager becomes the graphQLRunner interface (gofr-dev#4167), which has GetHandler but not Handle. - Reword the errWriter comment: it is a synthetic write failure used to exercise the logging path, not a reproduction of a client disconnect, since net/http buffers these small bodies.
…4268) * fix(graphql): log JSON encode failure in respondWithErrors (#4256) respondWithErrors discarded the error returned by json.Encoder.Encode, so a failure to write the GraphQL error body (e.g. client disconnected) left no trace. Log it via the container logger, matching the existing success-path log in the same file. Also add the missing blank line before GetHandler. Tests cover the three real error responses (500/415/400), the write-failure log, and the previously untested 415 and 400 paths through Handle. * test(graphql): route new test through GetHandler and clarify errWriter - TestGraphQL_RequestErrors now calls graphqlManager.GetHandler().ServeHTTP instead of Handle, so it keeps compiling once App.graphqlManager becomes the graphQLRunner interface (#4167), which has GetHandler but not Handle. - Reword the errWriter comment: it is a synthetic write failure used to exercise the logging path, not a reproduction of a client disconnect, since net/http buffers these small bodies. --------- Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com>
akshat-kumar-singhal
left a comment
akshat-kumar-singhal
left a comment
There was a problem hiding this comment.
Approve with nits (one should-fix in CI). Default build unchanged: go list -deps ./... is identical to development (840 packages), and it merges cleanly. No exported API removed; GraphQLLog is the only exported type moved behind a tag and nothing outside graphql.go uses it. go build/go vet/go test pass for default, each tag alone, and all three. Ran a binary with all three tags: PUBSUB_BACKEND=KAFKA|GOOGLE|MQTT, DB_DIALECT=postgres|sqlite|supabase and a GraphQL resolver each log the documented message, no /graphql route is registered, and the service still starts.
Should-fix: "Default build links the same packages as before" will fail unrelated PRs — go.yml:693
It diffs go list -deps ./... of HEAD against the merge base across the whole root module, so any future PR that adds a package or a dependency turns slim_build_tags red. Drop it (the "Each tag removes the packages it claims to" step already covers the direction that matters), or assert only that the default go list -deps gofr.dev/pkg/gofr still contains each tagged-out pattern.
Nit: the counts in slim-builds/page.md are platform/toolchain dependent. Go 1.26.0 linux/amd64, gofr.dev/pkg/gofr: none 808 (doc 828), nopubsub 595 (616), nosqldrivers 788 (784), nographql 788 (808), all three 555 (552). Plausible std/libc differences; state the GOOS/GOARCH and Go version measured with.
Note: this and #4168 conflict in docs/advanced-guide/slim-builds/page.md and disagree on the default count (828 vs 829); #4168 lacks this PR's last three commits (incl. the CI tag check, 8803000).
A GoFr binary links every datasource driver whether or not the service
opens one. Measured on a plain HTTP service: 827 packages, 57.8 MB of
binary and 31.6 MB of resident memory at rest, against 19.6-20.9 MB for
every other Go HTTP framework at the same observability.
Two things account for it, and neither is reachable from the HTTP path.
modernc.org/sqlite is blank-imported for driver registration and brings
modernc.org/libc, whose netdb init parses embedded copies of /etc/protocols
and /etc/services into permanent Go structs -- 1.69 MB of retained heap,
roughly half the process's fixed heap floor. The three concrete pub/sub
clients bring 212 packages between them, most of it Google Pub/Sub's grpc
and auth stack.
Both are now behind build tags. The default build is unchanged: it links
the same 827 packages as before, byte for byte, so a user who does nothing
sees nothing. A service that uses neither can build with
-tags 'gofr_nosqldrivers gofr_nopubsub'
and gets 42.7 MB of binary (-26%), 22.8 MB idle RSS (-28%) and a 1.5 MB
heap floor (-68%) -- within about 2 MB of gin and gorilla.
Nothing is pinned by a public type, which is what makes this possible
without an API change: Container.PubSub is the pubsub.Client interface, and
the drivers are blank imports whose only effect is init(). Container.SQL and
Container.Redis keep their concrete types and are untouched.
A tagged build that configures an omitted backend still starts and serves.
It logs one ERROR naming the tag for pub/sub, or database/sql's own
'unknown driver' for a dialect, and leaves the client nil -- the same state
an unconfigured datasource already produces. The stubs return an untyped
nil, so both Close and Health take their nil branches. (Health guards SQL
and Redis with isNil but PubSub with a plain != nil, which a typed-nil
would defeat; nothing here produces one, but see the follow-up.) Verified by running a tagged binary
under each of PUBSUB_BACKEND=KAFKA, GOOGLE and MQTT and DB_DIALECT=sqlite:
all four serve correctly and complain loudly.
Tests run in both configurations. The sql suite imports its own drivers, and
the two container tests that assert on a concrete client skip when the
backends are not linked.
Every GoFr binary links graphql-go and gqlparser whether or not the
service registers a resolver: 20 packages and 0.78 MB of binary that a
service with no GraphQL never executes.
-tags gofr_nographql now leaves them out. Measured on a minimal GoFr
service (gofr.New, one route, Run):
default 57.10 MB 831 packages
-tags gofr_nographql 56.32 MB 811 packages
Idle RSS is unchanged, and that is expected: the engine allocates
nothing until a resolver is registered, so what the tag saves is text,
not heap. The saving compounds with the datasource tags -- both of those
plus this one take the same service to 41.26 MB and 554 packages.
The mechanism is the one the pub/sub backends already use, with one
addition. App's field becomes an interface, graphQLRunner, naming the
five methods App actually calls; graphql.go, which holds the concrete
manager, carries the tag. Both halves are needed -- the interface alone
changes nothing, because the concrete file still compiles the library
in. That is the general rule for making any subsystem optional.
Nothing changes for a user who does not set the tag: GraphQLQuery and
GraphQLMutation name only GoFr's own Handler, so the exported surface is
byte-identical in both builds, and the default build links the same
packages it always did.
A build that does set the tag still compiles a user's GraphQL calls.
Registering a resolver logs an error naming the tag rather than failing
silently, and enabled() keeps App from routing /graphql and the
playground against a handler this build cannot provide. Without that
guard setupGraphQL registers a nil handler and the first POST /graphql
panics; TestGraphQLDisabled_SetupRegistersNoRoute fails if the guard is
removed.
noopResponder moves from responder.go into graphql.go, its only user. It
cannot stay: responder.go also holds the exported Responder interface,
so it cannot carry the tag, and leaving the type there makes it dead
code in a tagged build -- which golangci-lint reports as unused, failing
the lint gate for anyone who sets the tag.
The Slim Build Tags CI job now covers gofr_nographql alongside the
datasource tags, and runs the stub tests, which exist only under it.
… it claims Review follow-ups on #4167. The CI step named "Default build links the same packages as before" only ran go build and go test, so it asserted nothing of the sort. It now diffs `go list -deps ./... | sort` against the merge base and fails on any change, which is the claim the tags rest on: 861 packages, identical, measured locally. The tagged build is also linted now. It was not before, and the first run of the new step found a real noctx failure in graphql_disabled_test.go that no job would ever have reported -- the same class of problem that prompted moving noopResponder and configTrue in the first place. Two fail-loud paths had no test. - The disabled pub/sub stubs log an error naming the tag, but container_test.go only skips under the tag, and a skip proves nothing about the stub. An edit dropping one Errorf would leave every job green while producing the exact failure the tag exists to avoid: a publisher that silently never publishes. pubsub_backends_disabled_test.go asserts all three, and fails when the Errorf is removed. - registerOtel's "unknown driver" error is what makes a misconfigured tagged build legible. It is asserted against a dialect that is registered in NEITHER build, not against postgres under the tag: drivers_testdeps_test.go blank-imports pq and sqlite so the suite behaves identically either way, which means a tagged assertion about postgres would test the fixture rather than the code. graphQLRunner.enabled() becomes `const graphQLLinked`, matching the pubsubBackendsLinked the pub/sub side already uses. Whether the engine is linked is a property of the build, not of an instance, and the compiler can fold the branch away. drivers_disabled.go named only postgres and sqlite; supabase and cockroachdb register under the postgres driver (sql.go:268), so the tag affects them too. Adds the user-facing documentation the tags had none of, with the numbers measured rather than asserted (829 packages by default, 617/785/809 per tag, 553 with all three), and a CONTRIBUTING note that these tags are a closed exception for subsystems already in core -- a new integration ships as its own module.
The tagged lint step ran unscoped, so its first CI run reported every pre-existing goconst and exhaustive finding in the tree rather than anything this branch introduced. only-new-issues matches what the code_quality job already does. The typos check wants US spelling in the new docs page.
Building and testing under a tag proved the tagged code compiled; nothing asserted it dropped anything, which is the only reason the tags exist. Dropping the build constraint from drivers.go, or an import elsewhere pulling lib/pq back in by another path, left the Slim Build Tags job green. The new step diffs `go list -deps gofr.dev/pkg/gofr` per tag against the default build and fails if lib/pq, modernc.org/sqlite, kafka-go, cloud.google.com/go/pubsub, paho.mqtt.golang or graphql-go is still linked. It uses the package path, not ./..., because a pattern lists its own matches whether or not anything links them. It also fails when a pattern is absent from the DEFAULT build, so a renamed module cannot make it pass vacuously. This is the assertion the sql package cannot make from a test: drivers_testdeps_test.go blank-imports pq and modernc.org/sqlite because TestNewSQL_GetDBDialect opens a postgres DSN, so the test binary has both drivers registered under the tag too. What the library links is only observable from outside that binary. Renames drivers_disabled_test.go to drivers_registration_test.go: it has no build tag on purpose, and a _disabled_test.go name next to a tagged drivers_disabled.go reads like one that was forgotten. Measured on this branch: gofr.dev/pkg/gofr links 829 packages by default, 785 with gofr_nosqldrivers, 617 with gofr_nopubsub, 809 with gofr_nographql and 553 with all three.
…pment The figures were measured before this branch merged development, which changed the default set by one package, so every number in the section was off by one. Re-measured at this commit with `go list -deps gofr.dev/pkg/gofr`: 828 by default, 616 / 784 / 808 per tag and 552 with all three. Adds the binary-size figure the section was missing -- examples/http-server goes from 60,028,914 to 43,357,218 bytes, 27.8% -- and says plainly that these move with dependencies, so nobody reads them as a contract. The contract is the Slim Build Tags job, which fails if a tag stops removing the packages it names.
…tests compiling Rebased onto development, which brought in two untagged tests that do not hold under a tag, plus akshat-kumar-singhal's review of the CI step. **The "Default build links the same packages as before" step is removed.** It diffed `go list -deps ./...` against the merge base across the whole root module, and this job runs on every pull request -- so it meant "no PR may add a package or a dependency, ever". That is not a rule this repo has, and it would have gone red on changes with nothing to do with build tags. What it was protecting is already covered, and better, by the step above it: `grep -Eq "$pattern" /tmp/deps.default` fails when a tagged-out package is missing from the DEFAULT build, so the per-tag checks cannot pass vacuously and an untagged build cannot quietly stop linking them. A comment says so, and asks that the broad diff not be re-added. **Two tests from development needed a home under the tags.** TestApp_setupGraphQL_MissingSchema (#4274) names errSchemaMissing, which graphql.go compiles out; it moves from the untagged gofr_test.go into graphql_test.go, which already carries !gofr_nographql. TestContainer_createKafkaPubSub_InvalidConfigs asserts what kafka.New's own validation logs, and under gofr_nopubsub there is no kafka.New -- it takes the pubsubBackendsLinked skip the file already uses for three other tests. Both failed `go vet`/`go test` under their tag while the non-test build stayed green, which is the tagged-build blind spot this PR's CI job exists for: it caught them. **The doc now states where its counts were measured** -- darwin/arm64, Go 1.26.3 -- and that they are platform-dependent, so a linux/amd64 reader seeing roughly twenty fewer knows why the absolute numbers differ and the deltas do not. Verified across all five tag configurations: build, vet and test clean. typos clean, gofmt clean, go.yml parses. golangci-lint reports 5 issues against 7 on development, none new.
2139269 to
f1634f2
Compare
|
Rebased onto @akshat-kumar-singhal — your CI should-fix is taken, and I went with your first option rather than the second, because the second turned out to be already implemented. The whole-module dep diff is goneYou were right about the failure mode: Your suggested replacement — assert the default build still contains each tagged-out pattern — is what the "Each tag removes the packages it claims to" step above it already does: # Also fails when the packages are absent from the DEFAULT build, so a
# renamed or dropped module cannot make this check pass vacuously.
if ! grep -Eq "$pattern" /tmp/deps.default; thenSo adding it separately would have duplicated it. I dropped the broad diff and left a comment in its place explaining why, and asking that it not be re-added — @Umang01-hash originally asked for this step, and without the note the next reader has one reviewer's request in the history and no record of why it went away. @Umang01-hash — flagging that directly since it reverses part of what you asked for. The half of your comment that matters is intact: the step genuinely did not assert what its name claimed, and the fix for that is the per-tag check, which does the comparison against the default build. What is removed is only the "nothing may ever change" part. Happy to restore it scoped to The rebase caught two tests that do not survive a tagBoth came in from
This is the ongoing cost @PiyushSingh-ZS predicted in the coverage note, and worth saying plainly: every new untagged test that touches a tagged symbol lands as a red tagged build until someone moves it. Doc countsNow labelled darwin/arm64, Go 1.26.3, with a note that the counts are platform-dependent and that the differences between rows are the point — so your linux/amd64 figures being ~20 lower is explained rather than looking like an error. VerificationAll five configurations — default, each tag alone, and all three together —
|
…rect the grpc claim Review follow-ups on #4168, plus the #4167 follow-ups merged in. Each tag was only ever built in isolation, so a symbol that resolves under one and breaks under two would ship green and fail only for the user who set both. CI now builds, vets and tests all six together -- verified locally: builds clean, and 412 packages against the default 829. The nil-guard on grpcSrv had no test. newGRPCRunner fails on an out-of-range GRPC_PORT and factory.go logs and continues, so App runs on with no gRPC server, and these four setters used to dereference the field blind -- a config typo turning into a nil-pointer panic in the user's own setup code. All four are asserted, since a later edit is as likely to reintroduce it in one of the others as in the one that was reported. Neutralising the guard makes the test panic. The claim that gofr_nootlp "is the tag that actually releases google.golang.org/grpc" was wrong, and measurably so. Against gofr.dev/pkg/gofr: gofr_nogrpc alone leaves 82 grpc packages, adding gofr_nootlp leaves 81, adding gofr_nodgraph still leaves 81, and only adding gofr_nopubsub reaches 0 -- the Google Pub/Sub client pins grpc through cloud.google.com/go. A shared dependency goes when its last importer does, which is a property of these tags worth documenting rather than a detail of this one. The slim-builds documentation added in #4167 covers all six tags accordingly, including the table of that measurement, and notes that gofr_nogrpc is the one tag that changes the API surface.
Umang01-hash
left a comment
Umang01-hash
left a comment
There was a problem hiding this comment.
Re-verified locally at the head SHA against the three changes requested on 2026-09-18 — all three are addressed:
- Default-build dep parity is now asserted, not just compiled: the
Each tag removes the packages it claims tostep runsgo list -deps gofr.dev/pkg/gofrper tag and fails if a tagged-out package is missing from the default build (so it can't pass vacuously). Reproduced: default 828 pkgs -> nosqldrivers 784, nopubsub 616, nographql 808; 0 of the removed deps remain linked under their tag; default still links all of them (13 pq/sqlite, 60 kafka/google/mqtt, 11 graphql). - SQL fail-loud is tested:
TestRegisterOtel_UnregisteredDialectFailsLoudlyasserts the unknown-driver error via a dialect registered in neither build (dodging the drivers_testdeps fixture), plusTestRegisterOtel_AliasedDialectsUsePostgresfor supabase/cockroach. Passes. - Pub/sub fail-loud is tested:
pubsub_backends_disabled_test.go(tagged) asserts each stub logs the tag + client + rebuild hint and leaves PubSub nil. Passes.
Also verified: all four tag combos go build/go test clean; default exported API of pkg/gofr, container, sql is byte-identical to development (no break; GraphQLLog present in the default build); golangci-lint 0 new issues both default and tagged.
Still open:
- Minor, unaddressed:
graphql_runner.gostill says the tag drops1.4 MB(measured ~0.83 MB). - The mechanism question @PiyushSingh-ZS raised — build tags vs extracting the pub/sub backends into modules like the repo's existing 25 — is a maintainer design call, not a code issue. It's the one thing that isn't just verifiable, and it's what decides whether the pub/sub half of this stays tag-based.
The implementation itself is correct and does what it claims.
Conflict in pkg/gofr/container/container.go: development (#4164) changed createKafkaPubSub and createGooglePubSub to assign c.PubSub only when the constructor returns a real client, so a rejected config no longer leaves a typed nil in the exported field. This branch had moved both functions to pubsub_backends.go behind the gofr_nopubsub tag, so the conflict is resolved by keeping them there and carrying #4164's guard (and its comments) over to the new location. Re-measured the slim-builds figures at this merge, darwin/arm64, Go 1.26.3: 827 packages by default, 614 / 783 / 807 per tag, 550 with all three; examples/http-server 60,008,402 -> 43,384,642 bytes, 27.7%.
…ed 0.8 MB Measured on examples/http-server, darwin/arm64, Go 1.26.3: 60,008,402 B default vs 59,176,994 B with -tags gofr_nographql (-831,408 B), 827 -> 807 packages. Matches docs/advanced-guide/slim-builds.
…ropped 194a60f added a step building, vetting and testing all six tags together and widened the tagged lint to all six. Both were lost when the branch was rebased onto the updated #4167, whose own lint step carries only its three tags, so no CI step built nogrpc/nodgraph/nootlp in combination and the tagged files this PR adds were never linted. Restoring the lint surfaced two wsl_v5 findings in otlp_disabled_test.go that no job would have reported; the test now reads the registry through the package's lookup helper.
@Umang01-hashThanks for re-verifying. The one open minor from your 2026-09-18 review is now fixed in
That matches your ~0.83 MB and the slim-builds doc. All three of your blocking items are still in place at HEAD:
Your review is still recorded as CHANGES_REQUESTED, so it needs a re-review to clear. |
Umang01-hash
left a comment
Umang01-hash
left a comment
There was a problem hiding this comment.
Reviewed at head 8521d4b1, every claim verified locally.
- Compiles in all 5 configs (default + each tag + combined);
go list -depsconfirms the tags actually strip the packages (827 → 614 undergofr_nopubsub, pq/sqlite/graphql-go gone under theirs). - Default build is behaviorally identical — exported API unchanged (
graphqlManagerfield is unexported; interface swap is invisible to users).examples/using-graphqllive test passes; user code still compiles undergofr_nographql. - Disabled paths are gated (
graphQLActive()), no ungated nil-handler route. Disabled-path tests are behavioral (assert the loud error names the tag + "rebuild",/graphqlnot routed, SQL test uses a never-registered dialect to avoid fixture pollution). - gofmt/vet clean, golangci-lint 0 new issues (default + tagged). The
slim_build_tagsCI job genuinely enforces removal and is non-vacuous. Docs/nav/CONTRIBUTING consistent.
Clean, tightly scoped, no findings. LGTM. (Stacked before #4168.)
4 participants
Description:
Three opt-in build tags. A build that sets none of them is byte-for-byte what it is today.
gofr_nosqldriversgofr_nopubsubgofr_nographqlMeasured on a minimal GoFr service (
gofr.New, one route,Run):gofr_nosqldriversgofr_nopubsubgofr_nographql−15.84 MB (−27.7%), −277 packages. Idle RSS drops too, driven by the drivers.
Why the drivers cost so much.
modernc.org/sqliteis blank-imported for driver registration and bringsmodernc.org/libc, whosenetdbinit parses embedded copies of/etc/protocolsand/etc/servicesinto permanent Go structs — about 1.7 MB of retained heap, roughly half the process's fixed heap floor. None of it is reachable from the HTTP path. The three pub/sub clients bring 213 packages between them, most of it Google Pub/Sub's gRPC and auth stack.The mechanism, and the part that is easy to get wrong. Making a subsystem optional takes two things: an interface on the consumer naming only the methods it calls, and a build tag on the concrete file.
graphQLRunnerleft all the graphql packages linked untilgraphql.goitself carried the tag — the concrete file still compiles the library in.Breaking Changes (if applicable):
None. Every tag is opt-in and the default build is unchanged.
GraphQLQueryandGraphQLMutationname only GoFr's ownHandler, so the exported surface is identical in both builds.Verified against
development: identicalgo doc -allacross 6 packages, and an identical linked-package list.A build that does set
gofr_nographqlstill compiles a user's GraphQL calls. Registering a resolver logs an error naming the tag rather than failing silently, andenabled()keeps App from routing/graphqland the playground against a handler this build cannot provide.setupGraphQLregisters a nil handler and the firstPOST /graphqlpanics.TestGraphQLDisabled_SetupRegistersNoRoutefails if the guard is removed.Additional Information:
noopRespondermoves fromresponder.gointographql.go, its only user. It cannot stay:responder.goalso holds the exportedResponderinterface so it cannot carry the tag, and leaving it there makes it dead code in a tagged build — whichgolangci-lintreports asunused, failing the lint gate for anyone who sets the tag.configTrueis declared insidepubsub_backends.gofor the same reason: an untagged home would leave it unused undergofr_nopubsub.Checklist:
goimportandgolangci-lint.