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

Add module-info.java, fixes #2 #3

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

Conversation

zimmi
Copy link

@zimmi zimmi commented Apr 14, 2021

I also took the liberty to do the following things, please tell me if I should revert some of them:

  • Bump test dependency + plugin versions
  • Set project.build.sourceEncoding to fix build warning
  • Fix FileListTest when run as a module
  • Bump Travis JDK version to oraclejdk9 (I assume that's the reason the build fails there right now)
  • Set scm.url without property reference so it works on search.maven.org. You can see the current sad display here: https://search.maven.org/artifact/com.github.rzymek/opczip/1.2.0/jar

The --add-opens args for surefire are needed because JUnit reflectively calls the package private test methods, and that's not allowed when we are a module. An alternative would be to make every test class / method public, but that will probably just annoy future developers while providing little gain.

pom.xml Outdated Show resolved Hide resolved
Copy link
Author

@zimmi zimmi left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look! The current code (Version 1.2.0) is already building with and using Java 9 features (namely Set::of, InputStream::transferTo, InputStream::readNBytes, InputStream::readAllBytes and Arrays::equals), so I'm not sure how to proceed. I see two possibilities:

  1. Remove the Java 9 usage from the current code.
  2. Make another PR against the last released version that was Java 8 (Version 1.0.2, that's also the version used by fastexcel). Then do a point release (1.0.3?), then merge the changes back into master.

@rzymek
Copy link
Owner

rzymek commented Apr 18, 2021

Oh, damm! I haven't pushed my commit for Java 8 compatibility I've made a year ago 🤦
Pushed them now.

@zimmi
Copy link
Author

zimmi commented Apr 18, 2021

Haha, always good to be prepared! I'll take a look.

- Bump test dependency + plugin versions
- Set project.build.sourceEncoding to fix build warning
- Fix FileListTest when run as a module
- Bump Travis JDK version to oraclejdk9
- Set scm.url without property reference so it works on search.maven.org
@zimmi zimmi force-pushed the add-module-info branch from 8a0d80d to 5b27563 Compare April 19, 2021 03:43
@zimmi
Copy link
Author

zimmi commented Apr 19, 2021

@rzymek I rebased onto master and switched back to target Java 8. Please take another look.

Note: The project still has to be compiled with Java 9+ so the compiler understands the module-info. If building with Java 8 is needed, we could use https://github.com/moditect/moditect (generates module-info.class by ASM magic).

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.

2 participants