The buildbuddy repo is incompatible with OSS tooling #16
Comments
|
We're just a couple of ex-Googlers who went with what we're used to 😂 I think we'd be open to this at some point in the future - but right now we're moving quickly and think the migration would slow us down a bit more than we'd like. The go_default_library naming is probably our biggest annoyance with the OSS standard at the moment. Where exactly do you see the breakage? With gazelle, or some other tool? I wonder if there is some workaround we can implement in the meantime. |
This looks like a huge change, but I promise it's quite simple. In fact it's so simple that I was able to sync it with several days of commit from your mainline repo and only had one merge conflict (`server/BUILD`, which I replaced entirely) + updating the new imports you had made to point at their new path. I promise this won't negatively impact your productivity. If a file previously was declaring a unique package, put it under the equivalent path. For example: `server/backends/cachedb.go` was creating a package `github.com/buildbuddy-io/buildbuddy/cachedb`, so I simply moved it to the directory that would be associated with it. Additionally, I manually mapped the `go_proto_library`s in `/proto` to have an `importpath` that was relative to the repo root (`github.com/buildbuddy-io/buildbuddy`). This means that Gazelle knows about the existance of these proto files and can automatically add dependencies on them. Why is this worth merging? * You will now be able to get the beautiful godoc.org docs when you're working on your project. * I made Gazelle work, so now you'll no longer have to manually update your build files. * You will also be able to use more of the go ecosystem around linting, code fixes (eg), and more! * Having code that uses the standard tools decreases the burden on external contributors to your project (like me) who would be delighted to pitch in and fix issues here and there. After all, this is the whole reason to do open source, right? But isn't `go_default_library` ugly? * Yes, yes it is. It's also a hack that has been in place in `rules_go` for a while that is not beloved by the maintainer. In the olden days of rules_go, before there was even a Gazelle, the package name was inferred from the path to the file. You would declare your repo root when you loaded rules_go in your WORKSPACE. This, over time, was deemed to be insufficiently flexible for open-source where you can't make modifications in a `third_party` context. They have a different, now deprecated system called `vendors` that behaves similarly, but differently enough that it caused problems. In order to address this, `importpath` was added as a stopgap to allow the file path and the `importpath` to be inconsistent. Then there was an issue around creating crosscompilation targets for linux from osx. This issue has since been resolved in the master for rules_go using transitions, and as of the next release of rules_go. Fixing `go_default_library` is now just a matter of someone going into gazelle and updating it to have a flag to select naming things name things `go_default_library` or the directory that contains them. I think that is probably something I'm going to do in the near future. Testing strategy: I've ensured that the entire repo is able to build/test completely. Note that there are no tests checked into this repo, so the `bazel test` returns failure. The `bazel build` was successful, however. ``` $ bazelisk test //... INFO: Invocation ID: e4f2626b-88ef-4e32-a52c-1c0bdbe10780 INFO: Streaming build results to: https://app.buildbuddy.io/invocation/e4f2626b-88ef-4e32-a52c-1c0bdbe10780 WARNING: Streamed test output requested. All tests will be run locally, without sharding, one at a time INFO: Analyzed 117 targets (0 packages loaded, 0 targets configured). INFO: Found 117 targets and 0 test targets... INFO: Elapsed time: 0.262s, Critical Path: 0.00s INFO: 0 processes. ERROR: No test targets were found, yet testing was requested INFO: Streaming build results to: https://app.buildbuddy.io/invocation/e4f2626b-88ef-4e32-a52c-1c0bdbe10780 INFO: Build completed successfully, 1 total action $ bazelisk build //... INFO: Invocation ID: d1abc034-6186-4f9c-9b0a-0763c3725dc3 INFO: Streaming build results to: https://app.buildbuddy.io/invocation/d1abc034-6186-4f9c-9b0a-0763c3725dc3 INFO: Build option --test_sharding_strategy has changed, discarding analysis cache. INFO: Analyzed 117 targets (0 packages loaded, 16507 targets configured). INFO: Found 117 targets... INFO: Elapsed time: 1.096s, Critical Path: 0.07s INFO: 0 processes. INFO: Streaming build results to: https://app.buildbuddy.io/invocation/d1abc034-6186-4f9c-9b0a-0763c3725dc3 INFO: Build completed successfully, 1 total action ``` Fixed: buildbuddy-io#16
Oh yeah, I know that feeling 😉
For starters, none of the godocs work which makes it much harder as an outsider to delve into the project. In addition I get a lot of complaining from Gazelle when I run it. It sounds to me like the big blocker is finding dev resources to do this, so I threw some 20% time with some bash and a couple of for loops change to make you conformant with the expectations of the OSS world. I have prepared #17 to demonstrate how it could be done. |
The BuildBuddy repo uses the Google convention of having a
go_librarynamed for each package you would like declared. Unfortunately this is incompatible with the opensource golang ecosystem which makes importing it to a standard golang tree impossible.Would you be open to a PR that moves to a more standard OSS golang import path?
The text was updated successfully, but these errors were encountered: