Separating tools dependencies from main dependencies #4895
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adding gowrap in uber#4879 pulled in an additional *30MB* of dependencies, which is pretty ridiculous. It's happening because, while gowrap uses a tools-tagged `tools.go`, it's in the same module as everything else, so its dependencies are pulled in too, because Go has no way to tell that it's not needed by callers (it needs to consider ALL tags). We're doing the same thing. Granted, we're an application and not a library, but it still seems kinda unnecessary. And it forces upgrades we may not want to force (yet). So this moves tools.go to a new package, sets up a new go.mod file just for it, and adds a bit of automation to prevent it from drifting too far from the main module. With this, `go mod tidy` in the top level does not consider tools-dependencies any more, so we actually *remove* some, and do not add tons more for gowrap/etc in the future. --- internal/tools is just because it needs to be in a separate package, and "tools" was taken. plus nobody should ever import it, so it should be a clear "dependency dead-end".
Pull Request Test Coverage Report for Build 0181d202-4a5b-44cd-9939-9c2e378c1da3
💛 - Coveralls |
Makefile
Outdated
| @@ -110,7 +111,19 @@ LINT_SRC := $(filter-out %_test.go ./.gen/%, $(ALL_SRC)) | |||
| # downloads and builds a go-gettable tool, versioned by go.mod, and installs | |||
| # it into the build folder, named the same as the last portion of the URL. | |||
| define go_build_tool | |||
| @echo "building $(or $(2), $(notdir $(1))) from $(1)..." | |||
| @echo "building $(or $(2), $(notdir $(1))) from internal/tools/go.mod..." | |||
| @go build -mod=readonly -modfile=internal/tools/go.mod -o $(BIN)/$(or $(2), $(notdir $(1))) $(1) | |||
There was a problem hiding this comment.
won't this fail your @ rule?
There was a problem hiding this comment.
if merged without updating it, yes.
stacked PRs are not a thing github supports though, so I have to either include its changes in this PR, or wait for it to merge first.
Groxx
commented
Jul 6, 2022
Groxx
commented
Jul 6, 2022
| import ( | ||
| "reflect" | ||
|
|
||
| "github.com/apache/thrift/lib/go/thrift" |
There was a problem hiding this comment.
surprisingly, this is the only import of apache/thrift in the server. the rest of the use comes from importing the client.
might be worth exploring that, to see if we can get rid of it entirely.... but perhaps not. either way, this file was totally unused, and it moved a problematic dependency to only-indirect, so I figured I'd include it.
fixed conflict in makefile around @ -> $Q
davidporter-id-au
approved these changes
Jul 6, 2022
Groxx
added a commit
to Groxx/cadence
that referenced
this pull request
Jul 21, 2022
Adding gowrap in uber#4879 pulled in an additional *30MB* of dependencies, which is pretty ridiculous. It's happening because, while gowrap uses a tools-tagged `tools.go`, it's in the same module as everything else, so its dependencies are pulled in too, because Go has no way to tell that it's not needed by callers (it needs to consider ALL tags). We're doing the same thing. Granted, we're an application and not a library, but it still seems kinda unnecessary. And it forces upgrades we may not want to force (yet). So this moves tools.go to a new package, sets up a new go.mod file just for it, and adds a bit of automation to prevent it from drifting too far from the main module. With this, `go mod tidy` in the top level does not consider tools-dependencies any more, so we actually *remove* some, and do not add tons more for gowrap/etc in the future. --- internal/tools is just because it needs to be in a separate package, and "tools" was taken. plus nobody should ever import it, so it should be a clear "dependency dead-end" for tools.
Groxx
added a commit
to Groxx/cadence
that referenced
this pull request
Jul 21, 2022
Adding gowrap in uber#4879 pulled in an additional *30MB* of dependencies, which is pretty ridiculous. It's happening because, while gowrap uses a tools-tagged `tools.go`, it's in the same module as everything else, so its dependencies are pulled in too, because Go has no way to tell that it's not needed by callers (it needs to consider ALL tags). We're doing the same thing. Granted, we're an application and not a library, but it still seems kinda unnecessary. And it forces upgrades we may not want to force (yet). So this moves tools.go to a new package, sets up a new go.mod file just for it, and adds a bit of automation to prevent it from drifting too far from the main module. With this, `go mod tidy` in the top level does not consider tools-dependencies any more, so we actually *remove* some, and do not add tons more for gowrap/etc in the future. --- internal/tools is just because it needs to be in a separate package, and "tools" was taken. plus nobody should ever import it, so it should be a clear "dependency dead-end" for tools.
Groxx
added a commit
that referenced
this pull request
Jul 21, 2022
Adding gowrap in #4879 pulled in an additional *30MB* of dependencies, which is pretty ridiculous. It's happening because, while gowrap uses a tools-tagged `tools.go`, it's in the same module as everything else, so its dependencies are pulled in too, because Go has no way to tell that it's not needed by callers (it needs to consider ALL tags). We're doing the same thing. Granted, we're an application and not a library, but it still seems kinda unnecessary. And it forces upgrades we may not want to force (yet). So this moves tools.go to a new package, sets up a new go.mod file just for it, and adds a bit of automation to prevent it from drifting too far from the main module. With this, `go mod tidy` in the top level does not consider tools-dependencies any more, so we actually *remove* some, and do not add tons more for gowrap/etc in the future. --- internal/tools is just because it needs to be in a separate package, and "tools" was taken. plus nobody should ever import it, so it should be a clear "dependency dead-end" for tools.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding gowrap in #4879 pulled in an additional 30MB of dependency source files, which is
pretty ridiculous. It's happening because, while gowrap uses a tools-tagged
tools.go,it's in the same module as everything else, so its dependencies are pulled in too, because
Go has no way to tell that it's not needed by callers (it needs to consider ALL tags).
We're doing the same thing. Granted, we're an application and not a library, but it still
seems kinda unnecessary. And it forces quite a large number of upgrades we may not want to
force (yet).
So this moves
tools.goto a new package, sets up a newgo.{mod,sum}just for it, and addsa bit of automation to prevent it from drifting too far from the main module.
With this,
go mod tidyin the top level does not consider tools-dependencies any more,so we actually remove some, and do not add tons more or force upgrades to pull in gowrap/etc
in the future.
internal/tools is just because it needs to be in a separate package, and "tools" was taken.
plus nobody should ever import it, so it should be a clear "dependency dead-end".