Skip to content
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

Create blert v0.2.0 #6039

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Create blert v0.2.0 #6039

wants to merge 20 commits into from

Conversation

frolv
Copy link

@frolv frolv commented May 19, 2024

Blert is a PvM tracking and data analysis system hosted at https://blert.io.

This plugin collects data about players' raids and transmits it to the Blert servers to be processed and visualized.

@runelite-github-app
Copy link

runelite-github-app bot commented May 19, 2024

Includes non-plugin changes

New plugin blert: https://github.com/blert-io/plugin/tree/a0b45a8a1cf06be8d08b7ae7f29b951d12ee8973

@frolv
Copy link
Author

frolv commented May 19, 2024

This is a pretty large and complex project, so I'd be happy to discuss it in more detail as it is reviewed.

@frolv
Copy link
Author

frolv commented Jun 21, 2024

This should be ready to review now. Happy to chat about anything, and please reach out before merging.

@raiyni
Copy link
Member

raiyni commented Jun 21, 2024

The chances this gets reviewed in a timely manor is very low based on the number of dependencies added

@frolv
Copy link
Author

frolv commented Jun 21, 2024

No worries, I don't expect a quick review. Was just pointing out that I don't intend to make any further commits.

thirdParty("com.google.gradle:osdetector-gradle-plugin:1.7.3") {
because "blert"
}
thirdParty("kr.motd.maven:os-maven-plugin:1.7.1") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this used for? Both your plugin and the plugin hub are gradle projects, not maven projects.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried looking into it but couldn't find exactly where this is pulled in -- I assume it's a transitive of one of the protobuf libraries.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was looking the in wrong place. It is indeed coming from protobuf:

+--- com.google.protobuf:protobuf-gradle-plugin:0.9.4
|    \--- com.google.gradle:osdetector-gradle-plugin:1.7.3
|         \--- kr.motd.maven:os-maven-plugin:1.7.1
|              \--- com.google.code.findbugs:jsr305:3.0.2

@Nightfirecat
Copy link
Member

This would make the build non-reproduceable, please don't include it.

@frolv frolv requested a review from a team as a code owner July 27, 2024 19:29
@frolv
Copy link
Author

frolv commented Jul 27, 2024

This would make the build non-reproduceable, please don't include it.

Removed the remote check, it wasn't actually being used.

@frolv frolv changed the title Create blert v0.1.0 Create blert v0.2.0 Oct 18, 2024
@frolv
Copy link
Author

frolv commented Nov 23, 2024

On the dependency issue:

The plugin pulls in a single direct dependency (the protobuf compiler), which is quite large and has a bunch of transitives. However, this dependency is only required at build time to generate Java code from the plugin's .proto files. At runtime, the plugin reuses Runelite's existing version of the protobuf libraries and has no additional dependencies.

It is possible to remove the protoc dependency by manually running the compiler and checking in the generated Java protobuf code. This currently amounts to ~14.5K LOC (26K lines total) for serializing/deserializing protobuf messages to Java objects.

Does Runelite have a policy around checking in/reviewing generated code of this nature, and would it be preferable to eliminate the protoc dependency by doing so?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants