-
Notifications
You must be signed in to change notification settings - Fork 40.2k
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
Include licence and notice files in shipped jars #20058
Conversation
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 the PR.
I wonder if we should also include a notice.txt
. And maybe add a test for this?
@@ -127,6 +127,7 @@ private void applyJavaConventions(Project project) { | |||
}); | |||
project.getTasks().withType(Jar.class, (jar) -> { | |||
project.afterEvaluate((evaluated) -> { | |||
jar.metaInf((metaInf) -> metaInf.from(project.getRootProject().file("LICENSE.txt"))); |
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 am not sure if we want to ship our license or something a bit more complete as the one Spring Framework use.
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.
Let me know if I can help with something. Seems to be outside of my control at the moment, though!?
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 the Framework one matches our needs as it includes information about CGLib and ASM which Framework jarjars. The file in the root of our repo looks like a better fit to me for most of our modules at least. We may need to do something different for spring-boot-configuration-processor
as it shades some JSON stuff.
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 the Framework one matches our needs as it includes information about CGLib and ASM which Framework jarjars.
I didn't literally mean that file but more something with more details than just our license. I was actually referring to the bottom of the file. Not feeling strongly about it though.
I've added a test, @snicoll . Deciding what LICENSE.txt and/or if a notice.txt should be added, seems to be outside of my hands at the moment, but I'm happy to add anything after you made a decision. |
Thanks Christoph. I've added this one for team attention so that we figure it out during our next meeting. |
We discussed this today and concluded that:
@dreis2211 If the offer still stands, would you like to update this PR to reflect the above? |
Sure, @wilkinsona . Give me a minute or two. |
Okay it was an hour, but it should be done now. Let me know if this is what you had in mind. BTW: I have a bit of trouble to run the tests in |
Sorry, I'm not sure how IDEA handle's |
No problem here either but I am on |
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 very much, @dreis2211. This is pretty much exactly what I had in mind. I've left one comment/suggestion about how the LICENSE.txt and NOTICE.txt text resources are dealt with.
String noticeContent = FileCopyUtils.copyToString(new InputStreamReader(notice, StandardCharsets.UTF_8)) | ||
.replace("${version}", project.getVersion().toString()) | ||
.replace("${currentYear}", Integer.toString(Year.now().getValue())); | ||
metaInf.from(project.getResources().getText().fromString(noticeContent), |
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 think this'll break up-to-date checking for the jar. Creating a text resource from a string results in the creation of a temporary file with a random name. As that random name varies from build-to-build, the jar tasks will never be considered up-to-date as it'll look like its inputs have changed.
We had this problem when adding the classpath index support to BootJar
. The solution was to get the text resource's file and rename it before adding it to the copy spec rather than as part of the spec.
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 will take a look how it was done there and report back
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. I should have linked to the relevant code earlier. It is the following:
Lines 113 to 119 in 16111f1
private File createClasspathIndex(List<String> dependencies) { | |
String content = dependencies.stream().collect(Collectors.joining("\n", "", "\n")); | |
File source = getProject().getResources().getText().fromString(content).asFile(); | |
File indexFile = new File(source.getParentFile(), "classpath.idx"); | |
source.renameTo(indexFile); | |
return indexFile; | |
} |
This will result in a NOTICE.txt
and LICENSE.txt
file beneath each project's build
directory. An alternative could be to access the files in buildSrc/src/main/resources
directly from the filesystem.
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 tried that and that didn't work. Will take a look again.
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.
After the latest changes I get the following:
./gradlew :spring-boot-project:spring-boot-properties-migrator:jar
BUILD SUCCESSFUL in 1s
9 actionable tasks: 9 up-to-date
Previously, I got the following:
./gradlew :spring-boot-project:spring-boot-properties-migrator:jar
BUILD SUCCESSFUL in 1s
9 actionable tasks: 2 executed, 7 up-to-date
Thanks for the good catch, @wilkinsona
It's just occurred to me that the suggestion to use ${currentYear} was a bad one as it'll break the repeatability of the build. We should probably just hardcode it and update as needed. We can make that amendment as part of merging. |
Thanks very much, @dreis2211. |
Thanks for the proper review, @wilkinsona |
Hi,
this PR includes the
LICENSE.txt
file in theMETA-INF
directory when a jar is built. This should fix #15643 .Let me know what you think.
Cheers,
Christoph