Javascript extractor: Bazel-based build #14552
Conversation
|
@redsun82 will this interfere with the in-repo bazel build defined elsewhere? |
| "@//resources/lib/java:jericho-html", | ||
| "@//resources/lib/java:slf4j-api", | ||
| "@//resources/lib/java:snakeyaml", | ||
| "@//resources/lib/java/DO_NOT_DISTRIBUTE:junit", |
There was a problem hiding this comment.
Do we actually need this in deps?
There was a problem hiding this comment.
Surprisingly, yes. I think the project could use a better setup, to not compile (and ship) the unit tests (see https://github.com/github/codeql/tree/main/javascript/extractor/src/com/semmle/js/extractor/test).
I don't know how or where those tests are run either.
There was a problem hiding this comment.
I don't know how or where those tests are run either.
That seems important to find out, if only to ensure that we don't break anything by accident.
There was a problem hiding this comment.
I found out how it's working - there's a separate build target that runs these tests, and it's not broken by either this PR or the internal PR.
it shouldn't... it will break On the other hand, it is a bit weird to have open source code that only builds with an internal closed source build configuration. But I guess it was the case already before this change, right? |
|
Yes, exactly, this is not a regression. As we depend on a bunch of internal projects here it's not so easy to make this buildable standalone, and that's not the goal of my PR either. |
|
Yeah, I can confirm this has no effect on other in-repo bazel builds 👍 |
|
Can you explain what the |
|
Sure - bazel now compiles everything with errorprone, and that discovered that the two ifs I removed were never actually taken (char is unsigned in Java, therefore it can never be less than zero). |
Can you instead change the |
|
Sure, I replaced -1 with |
This introduces a bazel-based build for the Java components of the JS extractor.