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 to the project and its dependencies #17

Open
Adito5393 opened this issue May 5, 2022 · 6 comments
Open

Add module-info.java to the project and its dependencies #17

Adito5393 opened this issue May 5, 2022 · 6 comments

Comments

@Adito5393
Copy link
Contributor

Thanks a lot for sharing this library.

Would you be open to a PR for adding module-info.java to the project and its dependencies (lti-13-core & lti-13-jwt)?

The most important changes would be:

  1. Minimum Java version would change from 8 to 9+
  2. Some dependencies might need to be upgraded to fix issue Test errors seem to prevent successful build #16 (PR Made the build to be compatible with JDK13 environments by bumping some versions. Also just some import cleanup #5 shows the fix)

I would be willing to also configure Github actions to help you accept PR like #5 faster and easier.

@xaviaracil
Copy link
Contributor

Hi!

We have some apps in JDK 8. So, for making your proposed changes, we will have to branch JDK 8 version. I'll do it (and your PR's) next week.

Thanks!

@Adito5393
Copy link
Contributor Author

Great to hear that you are actively maintaining the library.

Indeed, the option of maintaining 2 branches for the library is easy to implement and it will fix the problem in the short run.
I think that it will add extra manual work in the long run (unless the branch synchronization is automated).

I would propose looking into the Multi Release feature via the maven-compiler-plugin. There should also be a Multi-Release JAR Check.

I, personally, never implemented it, however, I am willing to look into it and if it's not too much work, I could even supply a working version of the library for you to test out within your applications (JDK 8) and we can work towards a stable release deployment.

PS: I am not the author of PR #5. After checking the changes within that branch, there are many aesthetic changes, like replacing spaces with tab. Review it carefully and run your format IDE over the whole project to keep it consistent.
Alternatively, you could automate the format process via a maven plugin as part of the maven lifecycle on each developer machine, or, on the Github repo source code via the Github Actions for every PR/push event. (Check out the google-java-format repo or the formatter-maven-plugin).

Did you already know about the multi-release? What do you think about it? Do you think it is worth the extra hassle to keep the library in sync via JDK + maven?

@xaviaracil
Copy link
Contributor

I'm not aware of the multi-release. I know supporting JDK 8 nowadays may seem a loss of time and effort, but still there are some applications using it. However, I'd rather suggest to move to JDK 9 (or 11) and don't mess the library with some compatibility hacks that won't last for too long. So, our plans is to upgrade our applications to, at least, JDK 11, so that multi-release version won't be necessary.

Said that, It's only my opinion. If you find a working version compatible with JDK 8 that we can use would be very helpful for us, giving us extra time to upgrade our applications.

For this repo, I suggest keeping a JDK 8 branch, and move main branch to supported JDK versions. What do you thing?

@Adito5393
Copy link
Contributor Author

Adito5393 commented May 17, 2022

Awesome, raising the java minimum requirement to JDK11 is fine with me. I don't have any good argument to keep JDK 8 support alive.

Ping me here, when you have the JDK11 branch ready.
I can help out with 2 tasks:

  1. The module-info.java creation, testing, debugging, etc...
  2. Github actions CI/CD to automate the process for maven compile & testing for each Pull Request

EDIT: I would recommend the following repo management changes:

  1. Bumping the version number to 2.x.x for the JDK 11 branch and keeping the same artifactId lti-13-core. This way, the JDK8 branch can continue to be updated under version 1.x.x.
  2. Keep the master/main/default branch using the artifactId name ending with -SNAPSHOT & only remove it during a release phase (when a git tag is created). (it is following the gitflow-workflow where the master/main and develop are the same branch).

What do you think?

Adito5393 pushed a commit to Adito5393/java-lti-1.3-core that referenced this issue May 17, 2022
Run: ./mvnw clean verify -P sources,javadoc
Changes:
- Upgrade lombok for [JDK11 support](https://projectlombok.org/setup/maven)
- Add pluginManagement to force the maven plugin versions
- Add sources & javadoc profiles for the release phase
@Adito5393
Copy link
Contributor Author

Check out my changes and let me know to which branch would you like the PR

@xaviaracil
Copy link
Contributor

Great! Since the main branch must contain jdk11+, in my opinion, I'd suggest to create a JDk1.8 branch.

Agree with the management changes.

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

No branches or pull requests

2 participants