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

Polish outdated configs #831

Merged
merged 5 commits into from Mar 20, 2023
Merged

Polish outdated configs #831

merged 5 commits into from Mar 20, 2023

Conversation

Goooler
Copy link
Contributor

@Goooler Goooler commented Feb 27, 2023

Follow up 1a57c16.

Comment on lines -56 to -68
tasks.withType(JavaCompile).configureEach {
// This will be the default in Gradle 5.0
if (!options.compilerArgs.contains("-processor")) {
options.compilerArgs << '-proc:none'
}
}

tasks.withType(GroovyCompile).configureEach {
// This will be the default in Gradle 5.0
if (!options.compilerArgs.contains("-processor")) {
options.compilerArgs << '-proc:none'
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems we can remove these from buildSrc too?

Comment on lines -14 to -17
// Remove the gradleApi so it isn't merged into the jar file.
configurations.named(JavaPlugin.API_CONFIGURATION_NAME) {
dependencies.remove(project.dependencies.gradleApi())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

compileOnly should be enough

./gradlew assemble

diffuse diff --jar shadow-8.1.0-SNAPSHOT-before.jar shadow-8.1.0-SNAPSHOT-after.jar

OLD: shadow-8.1.0-SNAPSHOT-before.jar
NEW: shadow-8.1.0-SNAPSHOT-after.jar

 JAR   │ old       │ new       │ diff 
───────┼───────────┼───────────┼──────
 class │ 700.9 KiB │ 700.9 KiB │  0 B 
 other │  10.6 KiB │  10.6 KiB │  0 B 
───────┼───────────┼───────────┼──────
 total │ 711.5 KiB │ 711.5 KiB │  0 B 


 CLASSES │ old  │ new  │ diff      
─────────┼──────┼──────┼───────────
 classes │  146 │  146 │ 0 (+0 -0) 
 methods │ 1813 │ 1813 │ 0 (+0 -0) 
  fields │  746 │  746 │ 0 (+0 -0) 

Copy link
Owner

Choose a reason for hiding this comment

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

This appears to have broken shadowJar itself in this project. I think you're only comparing the output of the jar command above. However, the shadowJar tasks has been running for 18m on my machine.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, so applying the Gradle Plugin plugin adds the gradleApi() to the api configuration...https://github.com/gradle/gradle/blob/master/subprojects/plugin-development/src/main/java/org/gradle/plugin/devel/plugins/JavaGradlePluginPlugin.java#L162

And this is what this block was doing before.
So the compileOnly change works, but this block needs to be kept to remove this behavior (as there is no way to exclude the dependency.

Copy link
Owner

Choose a reason for hiding this comment

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

I've read-ed this line in 8.1.1. I'm not sure if that affects the configuration caching in an way.

archiveClassifier.set('javadoc')
from 'build/docs/javadoc'
}

tasks.register('sourcesJar', Jar) {
def sourcesJar = tasks.register('sourcesJar', Jar) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can drop these tasks entirely and instead use

java {
    withJavadocJar()
    withSourcesJar()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!


plugins {
id 'groovy'
id 'project-report'
id 'idea'
id 'java-gradle-plugin'
// id 'signing'
Copy link
Owner

Choose a reason for hiding this comment

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

I think I will re-add signing as plugin-publish supports it natively. I had turned this off when troubleshooting errors with publishing the 8.1.0 plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll put it back, believe you just set signing.required = false will disable the sign tasks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +45 to +46
// See https://github.com/johnrengelman/shadow/pull/831#discussion_r1119012328
required = false && gradle.taskGraph.hasTask("artifactoryPublish")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disabled for now.

@Goooler Goooler changed the title Remove outdated configs Polish outdated configs Feb 27, 2023
@Goooler Goooler force-pushed the more branch 3 times, most recently from de0c61d to 65595ff Compare March 2, 2023 16:15
@johnrengelman johnrengelman added this to the 8.1.1 milestone Mar 20, 2023
@johnrengelman johnrengelman merged commit 7f7a26a into johnrengelman:master Mar 20, 2023
@Goooler Goooler deleted the more branch March 21, 2023 01:24
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

2 participants