Go: add bazel build #16317
Go: add bazel build #16317
Conversation
It's not required, and it can't work from the internal repository.
redsun82
force-pushed
the
redsun82/go
branch
2 times, most recently
from
7165768
to
375660c
Compare
redsun82
added
the
depends on internal PR
There was a problem hiding this comment.
Thanks for all your work on this 🙇🏻
I have reviewed this from the perspective of the Go team, but a disclaimer that I am neither a Bazel nor a Python expert. So it should definitely be reviewed by someone who has more experience in those areas as well.
Mostly this looks good and it certainly seemed to work fine for me locally when running our tests. Other than some minor points and questions, the biggest question is probably about how we choose which version of Go to install and use in the GHA workflow -- see my detailed comments there.
MODULE.bazel
Outdated
| @@ -52,6 +54,9 @@ node.toolchain( | |||
| ) | |||
| use_repo(node, "nodejs", "nodejs_toolchains") | |||
|
|
|||
| go_sdk = use_extension("@rules_go//go:extensions.bzl", "go_sdk") | |||
| go_sdk.download(version = "1.22.2") # default | |||
There was a problem hiding this comment.
Note: I assume this now replaces the GO_VERSION environment variables in the workflows, so we should update the playbook accordingly.
There was a problem hiding this comment.
indeed, I will open a PR for that!
This reverts commit abcd916.
go/extractor/BUILD.bazel
Outdated
| srcs = [":tokenizer-jar"], | ||
| prefix = "tools", | ||
| renames = { | ||
| ":tokenizer-jar": "tokenizer.jar", |
There was a problem hiding this comment.
Did you check that this JAR has all the class files? I'd expect you need to build a deploy jar here, as otherwise the tokenizer-deps library class files will not make it into tokenizer.jar. If our tests haven't uncovered this yet, we need to make sure this JAR is functional at all (see discussion above about JVM versions), and ideally add a test for this, so we don't accidentally break this functionality during bazel upgrades.
There was a problem hiding this comment.
yep, I checked that exactly the same files end up in the extractor pack. By the way, I'm no java expert so I don't know what this all entails, but the split between tokenizer-deps and tokenizer-jar is precisely intended to reproduce what was being done on the pack previously in the Makefile:
Lines 101 to 106 in 880262d
You can see we are actively rming the files in tokenizer-deps, so it seems we actually didn't want a deploy jar here? Maybe @mbg has more details here?
There was a problem hiding this comment.
Oo okay, in that case this needs to be documented - the way it's currently written in the BUILD.bazel file it looks like a bug, not intended behavior. It is a correct reproduction of the behavior from the Makefile though.
There was a problem hiding this comment.
I'll add a comment then!
Co-authored-by: Cornelius Riemenschneider <cornelius@github.com>
It turns out everything that is needed for the installer to work on windows is enabling runfiles. This also requires symlinks to avoid excessive copying of files.
go/extractor/BUILD.bazel
Outdated
| srcs = [":tokenizer-jar"], | ||
| prefix = "tools", | ||
| renames = { | ||
| ":tokenizer-jar": "tokenizer.jar", |
There was a problem hiding this comment.
Oo okay, in that case this needs to be documented - the way it's currently written in the BUILD.bazel file it looks like a bug, not intended behavior. It is a correct reproduction of the behavior from the Makefile though.
|
I think we're close to getting this in <3 |
The build files are mostly generated using gazelle. All file generation (vendor directory, BUILD files and dbscheme) can be triggered with
bazel run //go:gen. The extractor pack can be built withbazel run //go:create-extractor-pack, which will put it in the same location that was used before (go/build/codeql-extractor-go).The
Makefileand.githubworkflows have not been updated, so CI will still use the old build here and in the internal repo. After the internal repo has been updated to use the new bazel definitions, we can clean up here.