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

fix: attempt to fix hiero dependency runtime to compile time only. #635

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlfredoG87
Copy link
Contributor

Reviewer Notes

See the following issue: hiero-ledger/tsc#85

Related Issue(s)

@AlfredoG87 AlfredoG87 added this to the 0.4.0 milestone Feb 11, 2025
@AlfredoG87 AlfredoG87 added Block Node Issues/PR related to the Block Node. dependencies Pull requests that update a dependency file Simulator Issue related to Block Stream Simulator labels Feb 11, 2025
@AlfredoG87 AlfredoG87 self-assigned this Feb 11, 2025
@AlfredoG87 AlfredoG87 marked this pull request as ready for review February 11, 2025 19:46
@AlfredoG87 AlfredoG87 requested a review from a team as a code owner February 11, 2025 19:46
@@ -31,7 +32,7 @@ testModuleInfo {
requires("org.mockito")
requires("org.mockito.junit.jupiter")
requires("org.assertj.core")
requiresStatic("com.github.spotbugs.annotations")
Copy link
Member

Choose a reason for hiding this comment

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

This change is wrong. Having com.github.spotbugs.annotations as requiresStatic is correct in general. My assumption is that any dependency brings it in as a non-static transitive dependency. I will have a look again to give more information

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just did a force push with try number 2 😅

Let me know if this fixes it, also, it would be nice to know if the analysis is something I can do locally on my branch.

Copy link
Collaborator

@jjohannes jjohannes Feb 13, 2025

Choose a reason for hiding this comment

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

This change should be irrelevant for the runtime classpath. I expect that it would not change anything on the result.

As I explained here, there is nothing bringing it in transitive:
hiero-ledger/tsc#85 (comment)

You can check the build scans yourself if you have doubts.

Copy link
Member

Choose a reason for hiding this comment

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

@jjohannes what is the mainRuntimeClasspath?

Bildschirmfoto 2025-02-13 um 09 13 55

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay I see the confusion. The name is not great for how we use it now. I should probably check if we can change it to something else that does not contain the term "RuntimeClasspath". It is used internally in the Gradle setup to create a "global scope" across all Modules using a few Gradle tricks. The goal is to have the same dependencies and versions available everywhere which we need for the Jar module-info patching. In the past it was only based on the runtime classpath (hence the name). But we noticed that sometimes the Jar patching was missing information about versions, which was because of "require static" (compileOnly) dependencies like this one that also needed patching.

Bottom line: This is something used internally for version consistency across all module of a project.

For the license check, we should only look at the standard runtimeClasspath.

Copy link
Contributor Author

@AlfredoG87 AlfredoG87 Feb 13, 2025

Choose a reason for hiding this comment

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

@jjohannes Thank you for the help and clarification! 🙏

@hendrikebbers, so we should be good then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Block Node Issues/PR related to the Block Node. dependencies Pull requests that update a dependency file Simulator Issue related to Block Stream Simulator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants