Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

Add ability to override the source-package in mockgen #531

Closed
choo-stripe opened this issue Feb 24, 2021 · 6 comments
Closed

Add ability to override the source-package in mockgen #531

choo-stripe opened this issue Feb 24, 2021 · 6 comments

Comments

@choo-stripe
Copy link

Requested feature A clear description of the desired feature and an example of
how it would be used.

There should be a way to specify the source-package such that mockgens requiring structs from the source file can succeed outside of a standard go environment.

For example, I should be able to pull a mockgen binary, and in a fresh docker container run:

$ workdir=$(mktemp)
$ cd $workdir
$ wget my_src.go
$ env - mockgen -source=my_src.go

Today, that'd fail.

Why the feature is needed A clear description of how this feature is not
served by existing functionality in gomock.

My end goal is to write a bazel rule that can mockgen for grpc clients. The great part about this is - as the proto schemas change, bazel will notice the dependency changed and re-run mockgen, ensuring the mock and source stay in-sync.

Currently mockgen uses standard go tooling to find the package of the source file submitted - go modules or poking around in GOPATH. This makes it incompatible with build systems like bazel, which run their own go toolchain to compile go files. Bazel executes builds in isolated build environments, which don't necessarily have access to go modules or GOPATH.

Now, mockgen is a tool. While it does go-like things i.e. codegen, the binary should be portable and able to execute outside of a go environment. The problem, obviously, is that mockgen can't really know the full package path for some arbitrary source file you give it, hence it relies on the go env.

So! If only we could override the package where a source file comes from, it wouldn't require a full-blown go env setup (also the behaviour is a bit too magical right now), and we'd be able to portably call mockgen -source=foo.go -source_package=my/foo, such that the generated mock would properly import from my/foo.

(Optional) Proposed solution A clear description of a proposed method for
adding this feature to gomock.

I wrote up a PR #529

@codyoss
Copy link
Member

codyoss commented Feb 24, 2021

Hey @choo-stripe,

I would like to have this issue sit a while for discussion from the community. I know there are other bazel users today and mockgen works for them from what I have heard. I assume this is because their build environment has Go in it. In general I don't think having Go in the environment is a bad thing. mockgen is meant to be a tool to generate mocks so you can write tests against. In order to run those test you will also need Go.

@jmillikin-stripe
Copy link

Hi @codyoss,

Thanks for the response! For context, the issue is not with the go tool itself, but with the dependency on a $GOPATH:

  • The standard way to use Go with Bazel is via rules_go, which makes the Go toolchain part of the Bazel build dependencies. This allows Go itself to be safely and hermetically upgraded.
  • Some auxiliary Go tooling assumes the presence of a $GOPATH-style directory, containing the full transitive dependencies of a library. This directory is expensive to create as part of a Bazel build action.
  • I don't have full context on this, but it looks like mockgen is using a library to detect the package name to generate code for, and that library depends on $GOPATH.
  • It would be nice, as a matter of build performance and debuggability, if mockgen allowed the package name to be specified directly instead of detected via the $GOPATH mechanism.

@codyoss
Copy link
Member

codyoss commented Mar 25, 2021

Why can you not specify with GOPATH where your sources are? I feel like this could work?

@linzhp I know you have talked about using bazel and mockgen in the past. Do you have thoughts here?

@linzhp
Copy link
Contributor

linzhp commented Mar 26, 2021

@codyoss Thanks for tagging me. Yes! I have some thoughts.

  • Some auxiliary Go tooling assumes the presence of a $GOPATH-style directory, containing the full transitive dependencies of a library. This directory is expensive to create as part of a Bazel build action.
  • I don't have full context on this, but it looks like mockgen is using a library to detect the package name to generate code for, and that library depends on $GOPATH.

I can give some context here. Before #420, mockgen used golang.org/x/tools/go/packages.Load, which calls go list, to determine the the package path given a source file. go list requires a GOPATH with a full transitive closure of dependencies, which is very I/O intensive to populate from scratch. When running outside Bazel, there is already either a full GOPATH or mod cache available, so mockgen doesn't have to populate it every time. However, they are not available in Bazel's sandbox for hermeticity, so they had to be populated for every target that calls mockgen in source mode, making them among the slowest targets to build in Uber's Go monorepo.

#420 removes the need for full GOPATH by guessing import paths from the source file's relative path in GOPATH. So Bazel only needs to populate a small GOPATH with the source file and aux files in it, instead of the full transitive closure. This greatly improves the build performance. Although we have been benefiting from such improvement internally at Uber, our pull request got stuck due to backward compatibility concerns and inactivity of the upstream project too.

I don't think this proposal can significantly improve the Bazel build performance over the combination of #420 and jmhodges/bazel_gomock#44. However, it would make mockgen more flexible, further breaking the tie to GOPATH, which is going away in Go 1.17. It also makes build systems like Bazel easier to call mockgen, because they no longer need to put the source file in a structure to resemble GOPATH. Such structure is often not readily available in their runtime environment, requiring extra copying.

So I see this as a usability improvement, with the cost of supporting an extra flag in mockgen.

@linzhp
Copy link
Contributor

linzhp commented Mar 26, 2021

To the point of

My end goal is to write a bazel rule that can mockgen for grpc clients.

You can do this without writing a Bazel rule on your own. For example:

load("@rules_proto//proto:defs.bzl", "proto_library")
load("@com_github_jmhodges_bazel_gomock//:gomock.bzl", "gomock")
load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
load("@io_bazel_rules_go//go:def.bzl", "go_library")

proto_library(
    name = "paxoshpb_proto",
    srcs = ["paxosh.proto"],
    visibility = ["//visibility:public"],
)

go_proto_library(
    name = "paxoshpb_go_proto",
    compilers = [
        "@io_bazel_rules_go//proto:gogoslick_grpc",
    ],
    importpath = "proto.example.com/paxosh",
    proto = ":paxoshpb_proto",
    visibility = ["//visibility:public"],
)

go_library(
    name = "go_default_library",
    embed = [":paxoshpb_go_proto"],
    importpath = "proto.example.com/paxosh",
    visibility = ["//visibility:public"],
)

gomock(
    name = "mocks",
    out = "mocks.go",
    interfaces = [
        "PaxosHClient",
        "PaxosHServer",
    ],
    library = ":go_default_library",
    package = "paxosh",
)

go_library(
    name = "go_mocks_library",
    srcs = ["mocks.go"],
    importpath = "mock.proto.example.com/paxosh",
    visibility = ["//visibility:public"],
    deps = [
        ":go_default_library",
        "@com_github_golang_mock//gomock:go_default_library",
        "@org_golang_google_grpc//:go_default_library",
    ],
)

Note that the example use reflective mode of mockgen, because the generated source file in go_proto_library is not visible

@codyoss
Copy link
Member

codyoss commented Apr 16, 2021

#547 should help with this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants