GH-945 Fix relocation updates. Support BOM dependency. Improve resolving dependencies #946
Conversation
WalkthroughThis update improves how the project handles dependencies and simplifies some code. It updates and removes certain version constants, changes how some libraries are included by using shadowing and relocation, and refactors the dependency loader to work in clearer steps with parallel processing. It adds support for BOM dependencies and enhances property handling in Maven files. Some classes were simplified or rewritten, and new ones were added for managing local repositories and caching relocations. Overall, these changes make dependency management and loading smoother and more efficient. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/resource/ResourceLocator.java (1)
36-43:⚠️ Potential issueAdd error handling for invalid URI format
The
URI.create()method can throwIllegalArgumentExceptionfor invalid URI strings. Consider catching and wrapping it in a more descriptive exception.public static ResourceLocator from(String uri) { try { return new ResourceLocator(URI.create(uri).toURL()); + } catch (IllegalArgumentException exception) { + throw new RuntimeException("Invalid URI format: " + uri, exception); } catch (MalformedURLException exception) { throw new RuntimeException(exception); } }eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/Dependency.java (1)
147-158:⚠️ Potential issueMissing isBom in equals/hashCode
The
isBomfield isn't included in equals() and hashCode(). This means BOM and non-BOM dependencies with the same coordinates are considered equal, which might cause unexpected behavior.@Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; Dependency that = (Dependency) o; - return Objects.equals(this.groupId, that.groupId) && Objects.equals(this.artifactId, that.artifactId) && Objects.equals(this.version, that.version); + return this.isBom == that.isBom && Objects.equals(this.groupId, that.groupId) && Objects.equals(this.artifactId, that.artifactId) && Objects.equals(this.version, that.version); } @Override public int hashCode() { - return Objects.hash(this.groupId, this.artifactId, this.version); + return Objects.hash(this.groupId, this.artifactId, this.version, this.isBom); }
🧹 Nitpick comments (4)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/repository/Repository.java (1)
7-8: Constructor visibility opened for subclassing.Making the constructor
protectedenablesLocalRepositorywhile still preventing external instantiation. Works fine.
Remember to keep the factory methodof()as the preferred entry for generic URLs.eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/RelocationCacheResolver.java (2)
5-6: Remove unused importsThe Gson imports aren't used in this class.
-import com.google.gson.Gson; -import com.google.gson.reflect.TypeToken;
45-55: Consider logging exceptionsThe exception handling here silently swallows errors. Consider at least logging them for debugging purposes.
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyLoaderImpl.java (1)
111-119: Consider making timeout configurableThe 60-minute timeout is quite long. Consider making it configurable or reducing it for better user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
buildSrc/src/main/kotlin/Versions.kt(1 hunks)buildSrc/src/main/kotlin/eternalcode-java-test.gradle.kts(1 hunks)eternalcore-core/build.gradle.kts(3 hunks)eternalcore-plugin/build.gradle.kts(0 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/EternalCoreLoader.java(1 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/EternalCoreLoaderConstants.java(1 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/Dependency.java(3 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyCollector.java(2 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyLoader.java(0 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyLoaderImpl.java(2 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/pom/PomXmlProperties.java(1 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/pom/PomXmlScanner.java(4 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/Relocation.java(1 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/RelocationCacheResolver.java(1 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/RelocationHandler.java(5 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/repository/LocalRepository.java(1 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/repository/Repository.java(1 hunks)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/resource/ResourceLocator.java(1 hunks)
💤 Files with no reviewable changes (2)
- eternalcore-plugin/build.gradle.kts
- eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyLoader.java
🧰 Additional context used
🧬 Code Graph Analysis (6)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/EternalCoreLoader.java (1)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/EternalCoreLoaderConstants.java (1)
EternalCoreLoaderConstants(11-63)
eternalcore-core/build.gradle.kts (1)
buildSrc/src/main/kotlin/EternalShadowExtension.kt (2)
library(7-10)libraryRelocate(12-14)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/RelocationCacheResolver.java (2)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/Dependency.java (1)
Dependency(10-179)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/repository/LocalRepository.java (1)
LocalRepository(6-21)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyLoaderImpl.java (4)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/RelocationCacheResolver.java (1)
RelocationCacheResolver(14-57)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/RelocationHandler.java (1)
RelocationHandler(49-131)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/repository/LocalRepository.java (1)
LocalRepository(6-21)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/repository/Repository.java (1)
Repository(3-28)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/RelocationHandler.java (1)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/Dependency.java (1)
Dependency(10-179)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/Dependency.java (1)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/resource/ResourceLocator.java (1)
ResourceLocator(10-45)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (17)
🔇 Additional comments (21)
buildSrc/src/main/kotlin/eternalcode-java-test.gradle.kts (1)
17-19: Looks good – constant aligned.Switching both
adventure-platformartifacts to use the unifiedVersions.ADVENTURE_PLATFORMkeeps things tidy.
Just double-check thatVersions.ADVENTURE_PLATFORMnow points at the right version for both JARs.eternalcore-plugin/src/main/java/com/eternalcode/core/loader/EternalCoreLoader.java (1)
18-23: Pure formatting – no action needed.Indentation was the only change; behaviour stays the same. 👍
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/pom/PomXmlProperties.java (1)
56-58: Nice touch – clearer null contract.Adding
@Nullableon thevalueparameter matches the return type and helps callers. No further issues spotted.eternalcore-plugin/src/main/java/com/eternalcode/core/loader/EternalCoreLoaderConstants.java (1)
56-59: Simpler relocation creation.Swapping the builder for the new record constructor keeps the code short and readable. All good.
eternalcore-core/build.gradle.kts (2)
31-32: Nice work on the dependency management!Moving the Adventure libraries to the shadow configuration is a good practice. This ensures they're properly isolated and relocated to avoid conflicts with other plugins.
55-58:❓ Verification inconclusive
Double-check the H2 relocation path
The gitcheck addition looks good. However, the H2 relocation path changed from
com.h2databasetocom.h2. Please verify this matches the actual package structure of the H2 library to avoid relocation issues.Also applies to: 73-73
🏁 Script executed:
#!/bin/bash # Check the actual package structure of H2 library fd -e java . | xargs rg -l "^package com\.h2" | head -10Length of output: 58
Double-check the H2 relocation path
The gitcheck changes look good. For the H2 relocation (in
build.gradle.ktsaround lines 55–58 and line 73), please confirm that relocating fromcom.h2databasetocom.h2actually matches the H2 library’s package structure. You can inspect the H2 JAR (e.g., viajar tf) or check the source to ensure the new relocation path is correct.buildSrc/src/main/kotlin/Versions.kt (1)
15-16: Version updates look good!The Adventure library versions have been bumped appropriately. These updates align with the dependency changes in the build files.
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/repository/LocalRepository.java (1)
8-19: Clean implementation!Good job extracting the local repository logic into its own class. The error handling is appropriate for this use case.
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/RelocationCacheResolver.java (1)
24-28: Clever caching approach!The simple text-based serialization for relocations is a smart choice - it's human-readable and avoids complexity. The cache invalidation logic based on content changes is solid.
Also applies to: 30-37
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/Relocation.java (1)
30-35: Excellent use of records!Great refactoring to a record - much cleaner and simpler. The pattern replacement logic is preserved nicely in the compact constructor.
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyCollector.java (1)
10-51: Good thread safety implementation!The synchronized methods ensure safe concurrent access during parallel dependency scanning. The BOM handling logic is well thought out.
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/pom/PomXmlScanner.java (2)
54-70: Nice stream refactoring!The functional approach makes the dependency scanning logic cleaner and easier to follow.
211-213: Good consolidation of scope filtering!Moving the scope check to a helper method improves readability.
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyLoaderImpl.java (1)
65-70: Excellent modular design!Breaking down the loading process into clear phases improves maintainability.
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/Dependency.java (2)
22-37: Clean BOM support implementation!The boolean flag and conversion methods provide a simple way to handle BOM dependencies.
Also applies to: 171-178
51-61: Smart refactoring of URL construction!Extracting the common URL building logic reduces duplication nicely.
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/RelocationHandler.java (5)
52-54: Dependency updates look good!The ASM libraries have been updated to version 9.7.1, which is great for keeping dependencies current.
63-69: Nice addition of caching support!The RelocationCacheResolver integration is properly implemented with the field and constructor updates.
79-83: Smart caching logic!The code efficiently checks for existing relocated JARs and only relocates when necessary. This should improve performance significantly.
95-101: Good file handling improvements!Deleting existing files before creating new ones prevents potential conflicts, and marking the cache after successful relocation keeps everything in sync.
107-108: Factory method properly updated!The static create method correctly accepts and passes the cache resolver to support the new caching feature.
Also applies to: 120-120
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/pom/PomXmlScanner.java
Show resolved
Hide resolved
...alcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyLoaderImpl.java
Show resolved
Hide resolved
Rollczi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyLoaderImpl.java (2)
93-97: Dead branch:relocationHandlercan’t be null
relocationHandleris always created in the constructor, so this check will never be true. Removing the branch simplifies the code.- if (this.relocationHandler == null) { - return new DependencyLoadEntry(dependency, downloadedDependencyPath); - }
54-56: Thread-pool size could use tuning
repositories.size() * 5is arbitrary; on large repo lists you may spawn far more threads than CPU cores. ConsiderMath.min(availableProcessors, repositories.size()*5)or make it configurable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyLoaderImpl.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyLoaderImpl.java (4)
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/RelocationCacheResolver.java (1)
RelocationCacheResolver(14-57)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/relocation/RelocationHandler.java (1)
RelocationHandler(49-131)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/repository/LocalRepository.java (1)
LocalRepository(6-21)eternalcore-plugin/src/main/java/com/eternalcode/core/loader/repository/Repository.java (1)
Repository(3-28)
...alcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyLoaderImpl.java
Show resolved
Hide resolved
| try { | ||
| this.executor.shutdown(); | ||
| this.relocationHandler.close(); | ||
| } | ||
| catch (Exception exception) { | ||
| } catch (Exception exception) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gracefully wait for the pool to finish before exiting
shutdown() only triggers an orderly shutdown; threads may still be alive when relocationHandler.close() runs or when the JVM exits. Add an awaitTermination (with a sensible timeout) to make sure all tasks are done and resources released.
this.executor.shutdown();
+try {
+ if (!this.executor.awaitTermination(30, TimeUnit.SECONDS)) {
+ this.executor.shutdownNow();
+ }
+} catch (InterruptedException ex) {
+ this.executor.shutdownNow();
+ Thread.currentThread().interrupt();
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| this.executor.shutdown(); | |
| this.relocationHandler.close(); | |
| } | |
| catch (Exception exception) { | |
| } catch (Exception exception) { | |
| @Override | |
| public void close() { | |
| try { | |
| this.executor.shutdown(); | |
| try { | |
| if (!this.executor.awaitTermination(30, TimeUnit.SECONDS)) { | |
| this.executor.shutdownNow(); | |
| } | |
| } catch (InterruptedException ex) { | |
| this.executor.shutdownNow(); | |
| Thread.currentThread().interrupt(); | |
| } | |
| this.relocationHandler.close(); | |
| } catch (Exception exception) { | |
| throw new DependencyException("Failed to close relocation handler", exception); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
eternalcore-plugin/src/main/java/com/eternalcode/core/loader/dependency/DependencyLoaderImpl.java
around lines 127 to 130, after calling this.executor.shutdown(), add a call to
this.executor.awaitTermination() with a reasonable timeout to wait for all tasks
to complete before proceeding. This ensures the thread pool finishes gracefully
before calling this.relocationHandler.close() and before JVM exit. Handle
InterruptedException properly during awaitTermination.
| if (thisMajor != dependencyMajor) { | ||
| return thisMajor > dependencyMajor; |
There was a problem hiding this comment.
I would change thisMajor to currentMinor, same for the variables below. Up for @Rollczi consideration.
| DependencyCollector collector = new DependencyCollector(); | ||
| this.logger.info("Resolving dependencies..."); |
There was a problem hiding this comment.
Would be good to find a way to disable this logger, so it can be enabled only for example in debug mode. LuckPerms, for instance, does not have such logging. It's not like I dislike this, but if we are planning to implement on-the-fly dependency loading to all of our plugins, it would be good to avoid duplicated mess in console.
| List<DependencyLoadEntry> dependencyLoadEntries = CompletableFutures.allAsList(futures) | ||
| .orTimeout(60, TimeUnit.MINUTES) | ||
| .join(); | ||
| return new DependencyLoadEntry(dependency, relocatedDependency); |
There was a problem hiding this comment.
Shouldn't we also log messages to console after dependency downloading? Downloading and loading from disk is not the same, so the numbers could sometimes differ
| collector.addScannedDependency(firstChild); | ||
| } | ||
|
|
||
| subDependencies.get().stream() |
There was a problem hiding this comment.
Have you profiled this method to check for performance loss to using simple for loop?
| Dependency.of("org.ow2.asm", "asm", "9.7.1"), | ||
| Dependency.of("org.ow2.asm", "asm-commons", "9.7.1"), |
There was a problem hiding this comment.
| Dependency.of("org.ow2.asm", "asm", "9.7.1"), | |
| Dependency.of("org.ow2.asm", "asm-commons", "9.7.1"), | |
| Dependency.of("org.ow2.asm", "asm", "9.8"), | |
| Dependency.of("org.ow2.asm", "asm-commons", "9.8"), |
|
Due to the priority of this task, I was forced to merge it without review. |
Naprawiłem błąd z tą relokacją
Naprawiłem błąd z tym, że adventure nie mogło być normalnie pobierane
Dodałem wsparcie mavenowego bom
Przyspieszyłem resolwanie zależności z 16 sek do 3 czy coś tam
https://cdn.discordapp.com/attachments/1025826406707507302/1382121165492981922/20250610-2211-11.9157632.mp4?ex=684a0024&is=6848aea4&hm=f2076859e8d79e66203b187cfa3a589a993854040ca1e0e349dfa2fde1f96a3c&
Ta mała przerwa 5 sekund pod koniec nie jest związana z pobieraniem dependency ^