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 handling jars without manifest #2

Merged
merged 2 commits into from
Oct 15, 2020

Conversation

iherasymenko
Copy link
Contributor

An example of such a jar: https://repo1.maven.org/maven2/javax/inject/javax.inject/1/javax.inject-1.jar

P.S. I know that there's a properly modularized Jakarta EE replacement, but I believe the plugin should still support the subject and do not throw an NPE if it encounters a jar like that.

Copy link
Member

@jjohannes jjohannes left a comment

Choose a reason for hiding this comment

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

@iherasymenko Thanks for this fix! Could you please not bump the Gradle version. I want to stay on 6.4 to make sure the plugin works with 6.4 as minimal version.

And would you mind adding a test that reproduces the NPE with "javax.inject-1.jar"? You can add a test with a minimal build doing that to ExtraJavaModuleInfoTest.

@iherasymenko iherasymenko force-pushed the jars-without-manifest branch from cd7a5c4 to 29f32d5 Compare October 14, 2020 18:18
@iherasymenko
Copy link
Contributor Author

iherasymenko commented Oct 14, 2020

@jjohannes thanks for the review. I applied the changes that you requested.

Btw, in order to make sure the plugin is compatible with 6.4, we could amend the test runner declaration with the "withGradleVersion" option. I.e. make it looks as follows.

    GradleRunner gradleRunnerFor(List<String> args) {
        GradleRunner.create()
                .withGradleVersion('6.4')
                .forwardOutput()
                .withPluginClasspath()
                .withProjectDir(testFolder.root)
                .withArguments(args)
    }

I can create a separate PR with the version bump and this change.

@iherasymenko iherasymenko force-pushed the jars-without-manifest branch from 29f32d5 to 07dd135 Compare October 14, 2020 18:30
Copy link
Member

@jjohannes jjohannes 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 adding the tests @iherasymenko. This looks good now.

Thanks for offering to do another PR to bump the Gradle version, but I would like to keep the it as it is to compile against the right gradleApi() as long as there is not very good reason to move to a newer version.

I'll merge this and do a new release of the plugin soon.

@jjohannes jjohannes merged commit 7e715cb into gradlex-org:master Oct 15, 2020
@iherasymenko iherasymenko deleted the jars-without-manifest branch October 15, 2020 14:27
@iherasymenko iherasymenko restored the jars-without-manifest branch October 15, 2020 14:27
@iherasymenko iherasymenko deleted the jars-without-manifest branch October 15, 2020 14:27
@msgilligan
Copy link

I just stumbled upon this issue (also with javax.inject) trying to migrate a module from Java Modularity Plugin to to this plugin. Will you be releasing 0.2 soon?

@iherasymenko
Copy link
Contributor Author

@msgilligan there is a properly modularized version of java.inject: jakarta.inject:jakarta.inject-api:1.0.1

I solved the issue as follows by substituting one with the other.

configurations.all { Configuration configuration ->
    configuration.resolutionStrategy.dependencySubstitution {
        substitute module('javax.inject:javax.inject') because 'it is not properly modularized' with module('jakarta.inject:jakarta.inject-api:1.0.1')
    }
}

@jjohannes
Copy link
Member

@iherasymenko @msgilligan I just published 0.2 which includes this fix. Let me know if you encounter any issues.

@msgilligan
Copy link

@iherasymenko Thanks! I didn't know you could do that. That may come in handy in the future.

@jjohannes 0.2 seems to be working for me. Thanks!

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.

3 participants