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 java module-info (#314 / #326) #326

Merged
merged 29 commits into from
Sep 26, 2020

Conversation

Bukama
Copy link
Member

@Bukama Bukama commented Aug 29, 2020

Add java module-info (#314 / #326)

This PR adds the java module-info file to the extension pack to
support modular builds.

The PR does not turn the extension pack into modules, which would
require Java9+ - the project still builds and runs on Java 8!

The module-info is placed into an own directory (called module)
inside `src/main` and is then compiles and placed into the final
JAR using the tool `moditect` (see: https://github.com/moditect/moditect ).
The tool was uses, because Gradle does not support a build on
Java 8 and adding a predefined module-info file. Gradle only supports
real modular-info files placed inside `src/main`, which results in the 
requirement of Java 9+ to build the project.

As a real module-info is now provided, the name of the automatic module
inside the MANIFEST.MF was removed.

This PR also removes JUnit internal classes from some extensions (e.g. the class
`Preconditions`) or replaces internal classes with their public counterpart
(e.g. `AnnotationUtils`). This ensures that a modular project can use
the extension pack without compile errors.

closes #314 
PR: #326

build.gradle.kts Outdated Show resolved Hide resolved
@Bukama
Copy link
Member Author

Bukama commented Sep 5, 2020

Local build successful. What do you think @sormuras ?

@Bukama Bukama requested a review from sormuras September 5, 2020 15:54
@Bukama Bukama changed the title Add module file (#314) Add java module-info and update Gradle to 6.4 (#314 / #326) Sep 5, 2020
@Bukama Bukama marked this pull request as ready for review September 5, 2020 16:30
# Conflicts:
#	gradle/wrapper/gradle-wrapper.properties
@Bukama Bukama changed the title Add java module-info and update Gradle to 6.4 (#314 / #326) Add java module-info (#314 / #326) Sep 6, 2020
Copy link
Member

@Michael1993 Michael1993 left a comment

Choose a reason for hiding this comment

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

LGTM

@Michael1993
Copy link
Member

Not 100% sure about the module name, since JUnit Pioneer has different projects as well as Pioneer itself.

Maybe it should be org.junitpioneer.junitpioneer? I'm not actually sure.

@Bukama
Copy link
Member Author

Bukama commented Sep 8, 2020

Not 100% sure about the module name, since JUnit Pioneer has different projects as well as Pioneer itself.

Maybe it should be org.junitpioneer.junitpioneer? I'm not actually sure.

I understand what you mean and I'm more one your side. Cause that I wrote it the current way is Nicolais version of the file (which I nearly did not modify). I have in mind that he chose the name by purpose, but I can't remember exactly.

@Bukama
Copy link
Member Author

Bukama commented Sep 8, 2020

Talked with Nicolai about the name and we most probably go for the double name to divide groupid / artefactid (or organization / reponame). We don't want to introduce "xp / expension pack" as this name is not common around. So we have to live with decisions from the past aka the double name in the repo.

edit: done

@sormuras
Copy link
Member

sormuras commented Sep 9, 2020

Would be great to have a "0.9.1" release with this PR merged ... to double-check the double name and modular resolution via Bach (aka Mozart).

@Bukama Bukama marked this pull request as draft September 9, 2020 14:53
@Bukama
Copy link
Member Author

Bukama commented Sep 9, 2020

Note; module-info gets not into JAR

We use internal classes of JUnit5, that must be changed.

@Bukama
Copy link
Member Author

Bukama commented Sep 9, 2020

@sormuras After adding moditect the whole gradle files is red, IntellJ removes main and test from sources (and you have to restore it) and when trying to execute it it fails that it does not find the plugin

An exception occurred applying plugin request [id: 'org.moditect.gradleplugin', version: '1.0.0-rc1']
> Failed to apply plugin 'org.moditect.gradleplugin'.
   > Could not create an instance of type org.moditect.gradleplugin.ModitectExtension.
      > java.lang.NullPointerException (no error message)

Are you sure this plugin is ready to use?

Edit: Adding just the dependency for moditect turns into the NPE

@Bukama Bukama requested a review from sormuras September 9, 2020 17:50
@sormuras
Copy link
Member

sormuras commented Sep 9, 2020

Seems like the org.moditect.gradleplugin plugin doesn't work well with Gradle 6.6.1 - a known issue @siordache @gunnarmorling?

@Bukama
Copy link
Member Author

Bukama commented Sep 16, 2020

Is it possible to create a Github Action which takes the repo, copies the module-info into src/main and runs the build on Java 11 and of course for Java11-code so we can check that?

Upon reading this, I realized that we can use the existing Java 11+ builds for that. No reason to create a new one, "just" use the existing ones and prepend cp src/main/module/module-info-java src/main/java.

But doesn't the source compatability need to be changed to 11? 🤷‍♂️

@Bukama
Copy link
Member Author

Bukama commented Sep 16, 2020

Is it possible to create a Github Action which takes the repo, copies the module-info into src/main and runs the build on Java 11 and of course for Java11-code so we can check that?

Upon reading this, I realized that we can use the existing Java 11+ builds for that. No reason to create a new one, "just" use the existing ones and prepend cp src/main/module/module-info-java src/main/java.

But doesn't the source compatability need to be changed to 11? 🤷‍♂️

Forget what I've said. I tried to update the build.

@beatngu13 & @aepfli Please have a look if I did it right. Never changed a Github Action before.

Copy link
Member

@aepfli aepfli left a comment

Choose a reason for hiding this comment

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

Lgtm - the gha changes, hard to read the rest on mobile

.github/workflows/main-build.yml Outdated Show resolved Hide resolved
@Bukama
Copy link
Member Author

Bukama commented Sep 17, 2020

As fiddeling in the main was too much for me I added the "new" module-build which was created based on the main build. Build first failed because module-info is not for Java 8 and I should add -source 9 but gradles doesn't either acceppts the said -source 9 neither a double dashed --source 9

How do I pass the argument correctly to Gradle?

edit: Fixed, thanks @Michael1993

@Bukama Bukama marked this pull request as draft September 20, 2020 08:53
@Bukama Bukama marked this pull request as ready for review September 20, 2020 09:29
@Bukama
Copy link
Member Author

Bukama commented Sep 20, 2020

Job for building with modules on Java 11 and 14 added.

Copy link
Member

@sormuras sormuras left a comment

Choose a reason for hiding this comment

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

org.junitpioneer.junitpioneer@0.9.1 jar:file:///.../junit-pioneer-bishue314_modules-v0.3.3-gdbbd460-133.jar/!module-info.class
exports org.junitpioneer.jupiter
exports org.junitpioneer.jupiter.params
exports org.junitpioneer.vintage
requires java.base mandated
requires org.junit.jupiter.api
requires org.junit.jupiter.params
requires org.junit.platform.commons

@sonarcloud
Copy link

sonarcloud bot commented Sep 26, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

97.4% 97.4% Coverage
0.0% 0.0% Duplication

@nipafx nipafx merged commit 3b67f16 into junit-pioneer:master Sep 26, 2020
@Bukama Bukama deleted the bishue314/modules branch September 26, 2020 09:32
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.

None yet

6 participants