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

Update bytebuddy dependency for Mockito #6364

Closed
wants to merge 1 commit into from

Conversation

matthias-ronge
Copy link
Collaborator

Mockito needs a newer version of ByteBuddy if there is a recent JVM.

See mockito/mockito#3328

Mockito needs a newer version of ByteBuddy if there is a recent JVM.

See mockito/mockito#3328
@henning-gerhardt
Copy link
Collaborator

We are still on Java 11 and want to switch to Java 17. The mentioned issue is for Java 21+ which will be used in a further future. Why should we now introduce a "bugfix" which ist needed in the future and may only relevant for now? If we are using a most recent version of Java then this would be a current real solution but as we are miles away from this I would not recommend this change.

@solth
Copy link
Member

solth commented Jan 9, 2025

I agree with @henning-gerhardt. Kitodo currently only supports Java 11, so that is the Java version that should be used for development environments as well. Introducing a new dependency for an as of yet unsupported newer version of Java might have unwanted side effects for the current version.

@stweil
Copy link
Member

stweil commented Jan 9, 2025

Citing the link mentioned above: "The code was successfully run on JDK 11,17".

Isn't Mockito only relevant for the unit tests, so unwanted side effects would be directly visible? That's why I think the risk is low (and reverting this change if required would be easy). Sooner or later the change will be needed, so why not now?

@henning-gerhardt
Copy link
Collaborator

Isn't Mockito only relevant for the unit tests,

Mockito can be used in all kind of tests.

so unwanted side effects would be directly visible?

Depending on how good or a bad a test is written this could be visible

That's why I think the risk is low (and reverting this change if required would be easy). Sooner or later the change will be needed, so why not now?

If Mockito have some issues with Java 21+ which would be solved in the Mockito project (maybe by rising the dependency versions) and we still miles away from Java 21+ why we should add a bugfix which is not needed now?

@matthias-ronge
Copy link
Collaborator Author

I am setting up a new computer and installed the latest software of everything, Java, Maven, Eclipse, Firefox, MariaDB, …

I am unhappy to run any of my software on old Java version only because of a fixable bug. And I only have one JAVA_HOME (at least I don’t know otherwise) to set the Java location for everything on the PC. And Java should be backward compatible.

@solth
Copy link
Member

solth commented Jan 9, 2025

@matthias-ronge any modern IDE allows you to select project specific JDKs if you have more than one installed on your computer. In IntelliJ this is done via the "Project settings", I am sure there is a similar configuration in Eclipse. Since Kitodo only supports Java 11 at this time that is what you have to work with. We cannot change the code if it may be counterproductive just to accomodate specific setups of individual developers.

@matthias-ronge
Copy link
Collaborator Author

I must build the project on the command line before I can import it to the IDE, so no way.

@henning-gerhardt
Copy link
Collaborator

henning-gerhardt commented Jan 9, 2025

@matthias-ronge : can you try to run JAVA_HOME=/path/to/java-11-jdk mvn install or JAVA_HOME=/path/to/java-11-jdk mvn package to create the application? I'm not familiar with Windows console anymore so maybe the command must be possible adjusted. At least on Linux I can run in such way the build process on a total Java 17 system.

@matthias-ronge
Copy link
Collaborator Author

Solution (for documentation):

Create file %userprofile%\mavenrc_pre.bat with the line:
SET JAVA_HOME=...

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

Successfully merging this pull request may close these issues.

4 participants