Blockers for using here at TM

Question

Blockers for using here at TM

Closed this issue 3 years ago · 6 comments

Hey guys, first off I'm eternally grateful for this tool otherwise I would be in a very sorry place for this v16 migration. That being said there are a few niggles that need fixing before we can use the tool here at Thought Machine. I'm very happy to help out once I get v16 out the door. With that being said, here they are:

Wollemi doesn't support GOPATH based projects very well. Ideally it shouldn't need to know about the go path. I've developed a helper tool to resolve go imports to build rules so you should be able to use that instead of guessing based on the go path.
We put tests in their own directory i.e. foo/foo.go and foo/test/foo_test.go however Wollemi will generate extra go_library() build rules for us.
We have a service_acceptance_test() build rule that wraps go_test() but injects some test main stuff to help with our integration tests. Wollemi doesn't understand this so generates us go_test() targets (and doesn't update it's deps). I wonder if we can give it a list of rules to work on, and default this to go_test(), go_library(), and go_binary() but we can add service_acceptance_test().
We sometimes have multiple go_test() targets in a package. In this case, wollemi adds a go_test(name = "test", ...) to our package instead of updating those tests. Ideally it should be able to look at the srcs to each rule.

To make it easier for you guys, I plan on labelling go rules with the packages they generate. There's then this code I wrote that consumers the output of plz query graph:

https://gist.github.com/Tatskaari/2820811a0de25b6a9a86d2de636015a2

I'll clean this up and get this out in a Please release so you guys can use that to resolve imports to build targets.

Other than that, it works pretty great and has helped massively. If we can work through these niggles, I will have a hoard of happy developers here at Thought Machine.

Answer 1 · 2021-03-09T05:43:52.000Z

That's great. Thanks for the feedback @Tatskaari! I would be more than happy to fix these.

It would help me to have a bit more clarification on these. If you could provide a simplified example that would be even better to help me understand what needs to be fixed.

1) I know that wollemi currently naively assumes the GOPATH will not include multiple paths. That definitely needs to be fixed. Were there other specific issues you had here that also need to be addressed other than that one?

2) I don't think I understand. Trying to reproduce locally I get the following. Is that not the desired behavior for your specific case or are you seeing something different?

[~]$ tree foo
foo
├── BUILD.plz
├── foo.go
└── test
    ├── BUILD.plz
    └── foo_test.go

[~]$ cat foo/BUILD.plz
go_library(
    name = "foo",
    srcs = glob(["*.go"], exclude = ["*_test.go"]),
    visibility = ["//foo/..."],
)

[~]$ cat foo/test/BUILD.plz
go_test(
    name = "test",
    srcs = glob(["*_test.go"]),
    external = True,
    deps = ["//foo"],
)

3) Yeah, I definitely think we can do that. Maybe we could just allow wollemi to be configured with a set of managed rule kinds. Or possibly just have wollemi manage deps, in a safe way, of any rule which includes go source files and appears to be like go_binary, go_library or go_test.

4) This one should already be supported. This was something which existed in our code base as well so I added support for it. However, I did not add a unit test specifically for this and support for it appears to have become broken. When a go_test does not exist and the package contains _test.go files wollemi will generate one with the name = "test" like you said. However, when go_test rules already do exist wollemi should just manage the deps of those rules based on their specific source files and not generate any new ones. The bug I mentioned looks like it caused each go_test rule to just use the imports of the entire go test package instead.

I pushed up a fix for the multiple go_test rule issue. It should work correctly now. Let me know if that doesn't fix the issue for you or you are experiencing some other issue there. Also btw I pushed up another change which improves general dependency resolution. I don't know if you had to configure any known_dependency targets on your side but if you did those may resolve for you normally now without the extra configuration. I also added configuration to make wollemi capable of generating and managing rules using explicitly listed source files instead of glob(["*.go"]). These recent changes have yet to be tagged so you would need to target master if you want to try them out.

Answer 2 · 2021-03-09T10:35:22.000Z

I know that wollemi currently naively assumes the GOPATH will not include multiple paths. That definitely needs to be fixed. Were there other specific issues you had here that also need to be addressed other than that one?

So it wasn't clear to me what we need to set gopkg to when we're using a GOPATH. I set it to "" and then ignored it, using the import path -> target map I generated from Please. Ultimately I don't think the go path actually needs to be relevant here if we use Please to resolve imports.

I don't think I understand. Trying to reproduce locally I get the following. Is that not the desired behavior for your specific case or are you seeing something different?

Ah I think we have some sources that look like normal sources but are only included in the test target:

foo
├── BUILD.plz
├── foo.go
└── test
    ├── BUILD.plz # contains go_test( srcs = ["test_util.go", "foo_test.go"], ... ) 
    ├── test_util.go
    └── foo_test.go

I'll see if I can get a simplified example together.

Yeah, I definitely think we can do that. Maybe we could just allow wollemi to be configured with a set of managed rule kinds. Or possibly just have wollemi manage deps, in a safe way, of any rule which includes go source files and appears to be like go_binary, go_library or go_test.

Need to be careful because there are some rules in our repo that depend on go srcs but don't need their deps e.g. for mock generation.

.....

No worries, appreciate the quick response on this one. :)

Answer 3 · 2021-03-09T17:24:02.000Z

So it wasn't clear to me what we need to set gopkg to when we're using a GOPATH. I set it to "" and then ignored it, using the import path -> target map I generated from Please. Ultimately I don't think the go path actually needs to be relevant here if we use Please to resolve imports.

Ah. I think I understand now. Wollemi requires the project to either exist in the GOPATH or define a go.mod. It uses one of those two options to determine what the go package name of the current working project is. i.e. in the context of wollemi, this repository, it would determine the gopkg to be github.com/tcncloud/wollemi. The reason it needs to know this is so it can distinguish internal imports from external imports. For example, continuing in the context of this repository, if we have a package which imports github.com/tcncloud/wollemi/adapters/bazel we know, since this import is prefixed with the gopkg, that it is an internal import and therefore use the target //adapters/bazel. Whereas if we have a package which imports github.com/spf13/viper we know, since this import is not prefixed with gopkg, that it is an external import and must instead be found somewhere in //third_party/go. Does that make sense? Unless I am remembering incorrectly this is the only way in which wollemi uses the GOPATH in regards to the gofmt command.

Forcing the gopkg variable to be "" would probably make wollemi assume every package is an internal import since every package would have "" as a prefix.

gopkg := ""

strings.HasPrefix("github.com/spf13/viper", gopkg) // => true
strings.HasPrefix("github.com/tcncloud/wollemi/adapters/bazel", gopkg) // => true
strings.HasPrefix("database/sql", gopkg) // => true

We could also just make this variable configurable, or alternatively try to get it from the .plzconfig.

Answer 4 · 2021-03-09T17:53:24.000Z

yeah that makes sense but with the go path stuff, that doesn't work. What should gopkg be if the project exists in the root of the go path? For example, we have import "common/go/kafka" which needs to resolve to //common/go/kafka.

If you look at my gist above, you can see that it no longer needs to do that. It can just ask the import resolver (which asks Please) to resolve the import. If it can't, it's either part of the go SDK or it's a missing import. This works for imports in the current package or externally. It also doesn't need a go.mod or for the project to be in the go path.

Answer 5 · 2021-03-22T22:50:20.000Z

@Tatskaari I think most if not all of your issues should be resolved now.

If you want wollemi to not create new rules you can now do the following from the command line.

wollemi gofmt --create=off

Alternatively you can enable or disable this at the package level with .wollemi.json config files.

{
  "gofmt": {
    "create": []
  }
}

It can also be more fine grained if desired.

{
  "gofmt": {
    "create": ["go_binary", "go_library"]
  }
}

In regards to 2) you have a couple new options. You can define managed rule kinds through .wollemi.json config files. A managed rule will have its deps automatically managed.

{
  "gofmt": {
    "manage": ["default", "custom_rule"]
  }
}

Or alternatively you can define a mapping between a standard go rule kind and a custom kind. The mapping will make wollemi treat custom_rule as if it was go_test. With the mapping you get rule creation using custom_rule instead of go_test as well as source and dependency management.

{
  "gofmt": {
    "mapped": {
      "go_test": "custom_rule"
    }
  }
}

Even though you can now disable rule creation you really shouldn't need to disable it everywhere. Wollemi should do a much better about not creating extra rules. If you do still get extra rules that's a bug and if I know about it I can fix it.

The one case I know of which will definitely still cause the extra build files is the following.

go_binary(
    name = "app",
    srcs = ["app/cmd/main.go"]
    visibility = ["PUBLIC"],
)

Each build file is currently inspected in isolation. When it parses and rewrites app/cmd it has no knowledge another package is consuming it's source files. As such you will get extra build rules created in app/cmd where the source files actually exist. I plan to fix this but until I do you should be able to manually disable rule creation in those problem areas through configuration.

There is also a lot more detail about the new configuraiton in the docs https://github.com/tcncloud/wollemi#configuration. The wollemi gofmt command also now includes --create, --manage and --mapped flags which, when explicitly set, override configuration on disk.

I think this covers everything you mentioned above but let me know if you have any other issues or if I have missed something.

Answer 6 · 2021-03-30T14:45:16.000Z

I think I'm 90% of the way there! Will raise some more specific issues/PRs if things come up but otherwise thanks so much for the hard work. I think you'll have 200 happy new users soon. :)