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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Go: add bazel build #16317
Go: add bazel build #16317
Changes from 28 commits
393f6b7
4ca8faa
7d9a68b
3ad9c02
925a2cc
19b2e56
bfa189e
146d84b
c8b0224
d98ccdf
0f387ee
86d6b8e
d66494d
b0758fd
0dfd336
6ec223c
2f6dd2a
f0f6c22
1f78882
15bb846
e7886d0
cb85a75
2590d8a
608791f
94212d1
a8d3226
12b9b80
520a2c9
9055d95
ca2d94b
318d954
4ae82ac
0bc6934
abcd916
1aafc37
76067cb
81dea9f
00baccb
31c427e
8f0b884
2132c7b
471303b
17990da
77128de
cba4ba0
5b184c1
73df4fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, I will open a PR for that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andtokenizer-jar
is precisely intended to reproduce what was being done on the pack previously in theMakefile
:codeql/go/Makefile
Lines 101 to 106 in 880262d
You can see we are actively
rm
ing the files intokenizer-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I haven't dealt with this part of the codebase before. @smowton or @owen-mc may know more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a comment then!
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.