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 type metadata to jars' manifests #22203

Closed
wants to merge 4 commits into from

Conversation

dreis2211
Copy link
Contributor

@dreis2211 dreis2211 commented Jul 2, 2020

Hi,

this PR fixes #22036 by marking the starter jars with an additional attribute in their manifest which is later checked to exclude the starter JAR during packaging.

I opened the PR now to discuss the taken approach better.

Let me know what you think.
Cheers,
Christoph

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 2, 2020
@snicoll
Copy link
Member

snicoll commented Jul 16, 2020

I wonder if generalizing this slightly could help us fix #22025 as well.

@dreis2211
Copy link
Contributor Author

@snicoll You mean marking spring-boot-autoconfigure-processor & spring-boot-configuration-processor with a similar entry in its manifest (or with one that suits all)?

@snicoll
Copy link
Member

snicoll commented Jul 16, 2020

Yes. I should mention this was targeted to the team at large to see if we could come up with a manifest entry that makes sense for both this use case and the issue I've referenced.

@wilkinsona
Copy link
Member

I like that idea, @snicoll. A more general purpose manifest header that can be used to influence what's included sounds good to me. I'm struggling to think of a good name for it though. In the interests of consistency, I think it makes sense for the exclusion to apply more broadly than just building fat jars and wars. It should probably also affect the classpath used for spring-boot:run, bootRun, when using Gradle's application plugin, etc.

@dreis2211
Copy link
Contributor Author

What about Exclude-From-Runtime as a name for the manifest entry?

@wilkinsona I would need a pointer which classes are involved when this should be applied on a broader level beyond packaging. At the moment I'm looking at AbstractRunMojo for Maven and BootRun for Gradle. But especially for Gradle I find it a bit difficult to apply the chosen mechanism to be honest. (I only have looked for a couple of minutes, though)

@wilkinsona
Copy link
Member

For Gradle, I'd try to do it by filtering the classpath that the BootRun, BootJar, and BootWar plugins are configured with. I'd do that in JavaPluginAction and WarPluginAction when those tasks are being defined.

I've just reminded myself that the application distribution that Boot configures uses the fat jar or war so we don't need to worry about that one.

@dreis2211
Copy link
Contributor Author

For Gradle, this seems to conflict with AdditionalMetadataLocationsConfigurer in JavaPluginAction that seems to check for spring-boot-configuration-processor being on the classpath.

@dreis2211
Copy link
Contributor Author

@snicoll I just tried adding the following to a test project's pom.xml:

		<dependency>
			<groupId>org.springframework.boot</groupId>
			<artifactId>spring-boot-configuration-processor</artifactId>
			<optional>true</optional>
		</dependency>

When packaging this up I see no jar of the processor included in the build.

@wilkinsona
Copy link
Member

For Gradle, this seems to conflict with AdditionalMetadataLocationsConfigurer

That's strange. That configurer checks the classpath of the JavaCompile classpath which should be unaffected by any filtering that's being done to the classpath of BootJar etc.

@dreis2211
Copy link
Contributor Author

When adding the following to a build.gradle

annotationProcessor "org.springframework.boot:spring-boot-autoconfigure-processor"

and packaging this up via bootJar I see no jar included. So both in Maven & Gradle the processors don't seem to end up already in the fat JARs. Am I missing something here?

@wilkinsona
Copy link
Member

wilkinsona commented Jul 16, 2020

That's the behaviour that I would expect with Gradle. With Maven, my understanding was that the annotation processor would be included in the fat jar and #22025 was opened to improve that.

@dreis2211
Copy link
Contributor Author

Would that also render the classpath discussion obsolete? If it doesn't I would need help there, I'm afraid...

@dreis2211
Copy link
Contributor Author

Edit: I tried Maven again with a clean build again and it included the processors. Sorry for the noise. For Gradle, it seems to be not included as I wrote earlier.

@wilkinsona
Copy link
Member

Would that also render the classpath discussion obsolete?

No, I don't think so. I'd like to try to use a classpath-filtering-based mechanism for the starters.

If it doesn't I would need help there, I'm afraid...

Sure thing. Let us know when you're ready to pass the baton and we can take it from there.

@dreis2211
Copy link
Contributor Author

I've pushed a more general solution for the packaging aspect with a new PackageExcludeFilter. Let me know what you think of that.
Other than that, this would be the point were you probably need to take over to cover the classpath filtering.

@dreis2211
Copy link
Contributor Author

I just renamed PackageExcludeFilter to FileExcludeFilter as this is a more fitting name imho.

@wilkinsona
Copy link
Member

Thanks, @dreis2211.

FWIW, I think Exclude-From-Runtime that you mentioned above is preferable to Exclude-From-Packaging that is currently proposed. That said, I've been wondering if we should go with something that's more declarative, describing what the dependency is rather than how it should be treated. Something like Spring-Boot-Dependency-Scope or Spring-Boot-Dependency-Nature with values of compileOnly or dependencyMetadataOnly respectively. I'm not sure yet though. Let's see what you and the rest of the team think.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Jul 17, 2020
@dreis2211
Copy link
Contributor Author

I wasn't really happy with either Exclude-From-Runtime or Exclude-From-Packaging, so I like the idea of introducing Spring-Boot-Dependency-Scope, but I'm not sure about the values yet. The configuration processors would be marked with compileOnly and the starters with dependencyMetadataOnly, right? What about the other modules - maybe a simple default value? I'm also thinking how the Gradle plugin structure would look like for that and how we would like to apply this to all modules.

@philwebb
Copy link
Member

We had a bit of a brainstorm today and we quite likeSpring-Boot-Jar-Type for the key with values dependencies-starter or annotation-processor.

@philwebb philwebb added type: enhancement A general enhancement and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Jul 17, 2020
@philwebb philwebb added this to the 2.4.x milestone Jul 17, 2020
@philwebb philwebb added the for: merge-with-amendments Needs some changes when we merge label Jul 17, 2020
@dreis2211
Copy link
Contributor Author

And for the others? Leave it empty? I could at least do that before you can take it over...

@wilkinsona
Copy link
Member

Yeah, leave it empty for the others please.

@dreis2211
Copy link
Contributor Author

Okay, I will craft a new plugin then and let you know when I'm finished.

@dreis2211
Copy link
Contributor Author

I added a new AnnotationProcessorPlugin similar to the starter plugin that applies the new jar type to the manifest. That seemed more pragmatic than introducing an additional DSL extension for the new jar type.
I finished the work that I wanted to do. I'm not happy here and there with some duplication (more specifically with the magic strings Spring-Boot-Jar-Type and its values). Maybe you can find a good place for those constants somewhere.
Let me know if I should do something on this PR still.
Cheers

@wilkinsona wilkinsona self-assigned this Jul 20, 2020
@wilkinsona wilkinsona changed the title Do not include empty starter jars in fat jars Add type metadata to jars' manifests Sep 10, 2020
@wilkinsona wilkinsona added type: task A general task and removed type: enhancement A general enhancement labels Sep 10, 2020
@wilkinsona
Copy link
Member

Thanks for the PR, @dreis2211. In reviewing this I came to the conclusion that we should take a different approach to filtering out the jars in the Maven and Gradle plugins so I reworked the changes here to add the Spring-Boot-Jar-Type manifest entry to Boot's starters and annotation processors. The changes to the filtering were made in 143d197 (Gradle) and e743d5f (Maven) and apply to both packaging and running an app.

@wilkinsona wilkinsona modified the milestones: 2.4.x, 2.4.0-M3 Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exclude empty starters jars and annotation processors when running or packaging with Maven and Gradle
5 participants