Rework in Kotlin by Malinskiy · Pull Request #123 · Tinder/bazel-diff

Malinskiy · 2022-05-13T06:16:53Z

Here is a non-exhaustive list of stuff done in the branch:

Rewrite in Kotlin
New heuristic for picking up source targets during hash calculation even when the target was not returned by bazel query
Moved integration test into the main codebase and just in general more tests. New coverage is 84%
Further parallelisation of source with hashing + target hashing (~20% faster for a large monorepo). Concurrency races are now solved
Reworked commands since get-impacted-targets doesn’t even need bazel in the first place, so less arguments. This changes the way you access the CLI though

There is an edge-case where the rules are depending on each other's digests, but they're hashed in parallel, so threads will not see the result of a previous hashing iteration. To overcome this we synchronise access using ConcurrentHashMap for the ruleHashes

…e error

* separate concerns * parallelise as much as possible * rework CLI interface (generate-hashes is not a subcommand of root command really) * remove redundant interfaces (there is only 1 implementation for each, no need for abstractions) * preprocess arguments

Malinskiy · 2022-05-13T14:11:45Z

I tried to setup the codecoverage for the project, but JRE8 is incompatible with the coverage code in the bazel apparently, so this in theory would only work for JRE11. Problem is that the output of code coverage $(bazel info output_path)/_coverage/_coverage_report.dat is always empty on the Github Runner and I can't reproduce this locally (where it passes without issues). Will try to tackle it next week

Malinskiy · 2022-05-17T09:50:56Z

Turns out that using latest version of bazel, i.e. Using unreleased version at commit d46269fe2397b4612a654543ba6fed45c2cdbdea doesn't produce the coverage data. I've replaced the ci config for now with the latest stable @tinder-maxwellelliott . What do you want to do? We can leave it at stable or wait until it's fixed upstream. Don't know if bazel is aware of this though, haven't checked.

As for the coverage - I tried using the commenting action, but it doesn't work with forked repositories since GITHUB_TOKEN has read-only access to the main repo from an action running for a fork. A better way may be to use something like coveralls, I believe they support lcov format but I'm not sure if this would make any difference to the permissions problem

* separate matrix jobs into jre8 and jre11. JRE8 doesn't support coverage with the latest bazel (coverage reporter class is compiled for JRE11) * latest upstream bazel breaks code coverage even for JRE11. use latest stable version

…E9+ code on JRE8

Malinskiy · 2022-05-17T09:56:21Z

Cleaned up the history a bit: there was a lot of trial and error involved in making this work

tinder-maxwellelliott · 2022-05-17T13:32:43Z

I tried to setup the codecoverage for the project, but JRE8 is incompatible with the coverage code in the bazel apparently, so this in theory would only work for JRE11. Problem is that the output of code coverage $(bazel info output_path)/_coverage/_coverage_report.dat is always empty on the Github Runner and I can't reproduce this locally (where it passes without issues). Will try to tackle it next week

Can we create a separate workflow for JRE11 and then get coverage data on that at least? I think its good to have a test run on idk8, but we don't need coverage from both

Malinskiy · 2022-05-17T13:35:28Z

@tinder-maxwellelliott that is exactly what’s done in this PR https://github.com/Tinder/bazel-diff/actions/runs/2337802484
coverage archive is available only from the jre11

tinder-maxwellelliott

Looking great!

tinder-maxwellelliott · 2022-05-17T13:34:03Z

-    name = "bazel-diff",
-    actual = "//src/main/java/com/bazel_diff:bazel-diff"
-)
+#alias(


Feel free to delete, I do not use these aliases

sure, will create another PR tomorrow

tinder-maxwellelliott · 2022-05-17T13:34:30Z

    "info.picocli:picocli:jar:4.3.2",
    "com.google.code.gson:gson:jar:2.8.6",
    "com.google.guava:guava:29.0-jre",
    "org.apache.commons:commons-pool2:2.11.1",


Should we go through these and upgrade them as well?

Yeah, will raise another PR then

tinder-maxwellelliott · 2022-05-17T13:39:19Z

@tinder-maxwellelliott that is exactly what’s done in this PR https://github.com/Tinder/bazel-diff/actions/runs/2337802484
coverage archive is available only from the jre11

🤦🏻 saw that as I was reviewing

Malinskiy · 2022-05-17T13:49:48Z

We should also update the readme with the new command bazel-diff generate-hashes. Both have less options, e.g. get-impacted-targets doesn't need bazel info anymore since it's not even used for the diff logic

tinder-maxwellelliott · 2022-05-17T13:59:43Z

Will do, testing in our repo now and will update accordingly

Malinskiy added 11 commits May 2, 2022 15:04

fix(generate-hashes): attempt to fix the nested generatino deadlock

b668cc8

fix(generate-hashes): fix recursing into already calculated hashes

e59794e

fix(generate-hashes): rework code a bit

8e6b4cc

fix(generate-hashes): warn and fail on problems instead of masking th…

44260cd

…e error

fix(test): bring back the tests

3fdd2ba

feat(test): reimplement integration test

c085e84

fix(test): use just bazel instead of hardcoded path to bazel in e2e test

3609182

fix(all): fix warnings

4653365

feat(build): remove deprecated jcenter repository

c9f9c16

Malinskiy changed the title ~~Feature/kotlin~~ Rework May 13, 2022

Malinskiy changed the title ~~Rework~~ Rework in Kotlin May 13, 2022

Malinskiy force-pushed the feature/kotlin branch 2 times, most recently from 4e3e798 to 75030c0 Compare May 13, 2022 13:36

Malinskiy force-pushed the feature/kotlin branch from 40473e4 to 8226bc7 Compare May 17, 2022 08:06

Malinskiy added 3 commits May 17, 2022 19:54

fix(ci): test logs and coverage report as an artifact

f609b03

* separate matrix jobs into jre8 and jre11. JRE8 doesn't support coverage with the latest bazel (coverage reporter class is compiled for JRE11) * latest upstream bazel breaks code coverage even for JRE11. use latest stable version

fix(hashing): fix for NoSuchMethodError on ByteBuffer when running JR…

ffd6315

…E9+ code on JRE8

fix(ci): fix for bytebuddy mock macker

68672fc

Malinskiy force-pushed the feature/kotlin branch from 7feca62 to 68672fc Compare May 17, 2022 09:55

tinder-maxwellelliott approved these changes May 17, 2022

View reviewed changes

tinder-maxwellelliott merged commit 06a75b8 into Tinder:master May 17, 2022

Malinskiy deleted the feature/kotlin branch May 18, 2022 03:19

tinder-maxwellelliott mentioned this pull request Aug 15, 2026

ci: turn the orphaned deploy job into a release-artifacts check #463

Merged

4 tasks

Uh oh!

Conversation

Malinskiy commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Malinskiy commented May 13, 2022

Uh oh!

Malinskiy commented May 17, 2022

Uh oh!

Malinskiy commented May 17, 2022

Uh oh!

tinder-maxwellelliott commented May 17, 2022

Uh oh!

Malinskiy commented May 17, 2022

Uh oh!

tinder-maxwellelliott left a comment

Choose a reason for hiding this comment

Uh oh!

tinder-maxwellelliott May 17, 2022

Choose a reason for hiding this comment

Uh oh!

Malinskiy May 17, 2022

Choose a reason for hiding this comment

Uh oh!

Malinskiy May 18, 2022

Choose a reason for hiding this comment

Uh oh!

tinder-maxwellelliott May 17, 2022

Choose a reason for hiding this comment

Uh oh!

Malinskiy May 17, 2022

Choose a reason for hiding this comment

Uh oh!

Malinskiy May 18, 2022

Choose a reason for hiding this comment

Uh oh!

tinder-maxwellelliott commented May 17, 2022

Uh oh!

Malinskiy commented May 17, 2022

Uh oh!

tinder-maxwellelliott commented May 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Malinskiy commented May 13, 2022 •

edited

Loading