-
Notifications
You must be signed in to change notification settings - Fork 10
versionsProvidingConfiguration change and ExportAllExcludes #207
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
base: main
Are you sure you want to change the base?
versionsProvidingConfiguration change and ExportAllExcludes #207
Conversation
…ther than a string identifying the configuration. Added the ability to export all packages with the exception of named ones.
jjohannes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank your for the contribution @timhamoni!
I think the exportAllPackagesExcept is a great addition to this plugin.
I would rather not touch the versionsProvidingConfiguration thing as suggested here (see comment below). I am happy to discuss alternative improvements in this area as a separate issue.
| */ | ||
| private static String pathToPackage(String path) { | ||
| return path.replace('/', '.'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Thanks for introducing these methods!
| // or simply export all packages | ||
| // exportAllPackages() | ||
| // or export all packages except specific named ones | ||
| // exportAllPackagesExcept("org.mycompany.notgood1", "org.mycompany.notgood2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| file("src/main/java/module-info.java") << """ | ||
| module org.gradle.sample.app { | ||
| exports org.gradle.sample.app; | ||
| requires org.glassfish.java.json; | ||
| requires java.json; | ||
| } | ||
| """ | ||
| buildFile << """ | ||
| dependencies { | ||
| implementation("org.glassfish:jakarta.json:1.1.6") | ||
| implementation("jakarta.json:jakarta.json-api:1.1.6") | ||
| } | ||
| extraJavaModuleInfo { | ||
| module("org.glassfish:jakarta.json", "org.glassfish.java.json") { | ||
| exportAllPackagesExcept("javax.json", "javax.json.spi", "javax.json.stream") | ||
| overrideModuleName() | ||
| } | ||
| knownModule("jakarta.json:jakarta.json-api", "java.json") | ||
| } | ||
| """ | ||
|
|
||
| expect: | ||
| def result = failRun() | ||
| result.output.matches(/(?s).*Package javax\.json[.a-z]* in both.*/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding a test. Right now it does not seem to test the new feature, though. The split-package error will always occur no matter if the packages are exported or not.
I suggest the following adjustment
| file("src/main/java/module-info.java") << """ | |
| module org.gradle.sample.app { | |
| exports org.gradle.sample.app; | |
| requires org.glassfish.java.json; | |
| requires java.json; | |
| } | |
| """ | |
| buildFile << """ | |
| dependencies { | |
| implementation("org.glassfish:jakarta.json:1.1.6") | |
| implementation("jakarta.json:jakarta.json-api:1.1.6") | |
| } | |
| extraJavaModuleInfo { | |
| module("org.glassfish:jakarta.json", "org.glassfish.java.json") { | |
| exportAllPackagesExcept("javax.json", "javax.json.spi", "javax.json.stream") | |
| overrideModuleName() | |
| } | |
| knownModule("jakarta.json:jakarta.json-api", "java.json") | |
| } | |
| """ | |
| expect: | |
| def result = failRun() | |
| result.output.matches(/(?s).*Package javax\.json[.a-z]* in both.*/) | |
| file("src/main/java/module-info.java") << """ | |
| module org.gradle.sample.app { | |
| exports org.gradle.sample.app; | |
| requires java.json; | |
| } | |
| """ | |
| buildFile << """ | |
| dependencies { | |
| implementation("org.glassfish:jakarta.json:1.1.6") | |
| implementation("jakarta.json:jakarta.json-api:1.1.6") | |
| } | |
| extraJavaModuleInfo { | |
| module("org.glassfish:jakarta.json", "java.json") { | |
| exportAllPackagesExcept("javax.json", "javax.json.spi", "javax.json.stream") | |
| } | |
| } | |
| """ | |
| expect: | |
| def result = failRun() | |
| result.output.contains("error: package javax.json is not visible") |
| public abstract Property<Boolean> getDeriveAutomaticModuleNamesFromFileNames(); | ||
|
|
||
| public abstract Property<String> getVersionsProvidingConfiguration(); | ||
| public abstract Property<Configuration> getVersionsProvidingConfiguration(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should change this. If I understand your reasoning correctly, this actually makes the situation worse. The code at the moment always takes the Configuration out of the current project scope – versionsSource = project.getConfigurations()... in PublishedMetadata.java. This ensures that the user cannot put in a Configuration object obtain from a different context.
To solve the warning in you project, you should never do something like this:
configurations.add(project("platform").configurations.named("allDependencies"))
Note: if you turn on org.gradle.unsafe.isolated-projects=true it will find such things in your setup and give you warnings/errors for it.
Instead, you should make the allDependencies a consumable (not resolvable) in platform.
Then in all other projects, you define a dependency to that. Similar to what is documented here. Something like:
// main/platform/build.gradle.kts
val consistentResolutionAttribute = Attribute.of("consistent-resolution", String::class.java)
configurations.create("allDependencies") {
isCanBeResolved = false
...
attributes { attribute(consistentResolutionAttribute, "global") }
}// main/project/build.gradle.kts
val consistentResolutionAttribute = Attribute.of("consistent-resolution", String::class.java)
val allDependenciesPath = configurations.create("allDependenciesPath")) {
isCanBeConsumed = false
attributes { attribute(consistentResolutionAttribute, "global") }
}
dependencies {
allDependenciesPath(project(":platform"))
}
extraJavaModuleInfo {
versionsProvidingConfiguration = "allDependenciesPath"
}This is a bit tricky, but I don't have a more compact solution. And because it is quite individual in different project setups, and cross-cutting subprojects, I don't have a good idea for features in this plugin to make it easier atm.
Hi,
I've submitted two changes in this. The first is probably the most major.
versionsProvidingConfiguration change
I have changed the versionsProvidingConfiguration from a String to a Configuration. I realise this is a breaking change, but I think it is needed. Gradle 9 introduces extra restrictions around Configurations and their use. A configuration from another project cannot be imported into the current project. The reason I was doing this is as follows.
I have a multi-project build, which do not share a common parent (instead they have a platform project). To obtain a consistent Configuration for resolution, I was adding all dependencies onto a configuration in a holder project, then adding that to each project that used so that the string lookup would work.
For example,
This caused Gradle to issue a warning about the feature being depreciated, and caused the dependencies task to fail.
The updated approach should allow more flexibility, but I realise it will break existing usage.
exportAllPackages enhancement
The second change is to add an exception to exportAllPackages(). I found that a number of packages are "mostly good", and just needed one package not-exporting. In the current version I had to specify all packages to export, using this feature, I can specify which packages not to export.
extraJavaModuleInfo { module("org.example:package", "org.example.package") { exportAllPackagesExcept("org.example.package.hidden") } }