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 manifest files generated from ksp into final jar #1094

Merged
merged 11 commits into from
Jan 26, 2024

Conversation

raghulvelusamy
Copy link
Contributor

@raghulvelusamy raghulvelusamy commented Dec 18, 2023

Fix for issue #991

  • Ensures to copy all generated files from KSP jar's Manifest folder to the final jar.
  • Adds tests to verify the same

@@ -42,7 +43,7 @@ open class JarExtractor protected constructor(
Files.createDirectories(target)
else -> jar.getInputStream(entry).use {
Files.createDirectories(target.parent)
Files.copy(it, target)
Files.copy(it, target, StandardCopyOption.REPLACE_EXISTING)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happened here to need the REPLACE_EXISTING attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we are using the same generated classes directory for all the jar creation processes during KotlinCompile when its trying to copy there is a java.nio.file.FileAlreadyExistsException. This is to avoid that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think one common file that would be a duplicate is META-INF/MANIFEST.MF.

val relativePath = srcJarsPath.relativize(path)
val destPath = Paths.get(directories.generatedClasses).resolve(relativePath)
destPath.parent.toFile().mkdirs()
Files.copy(path, destPath, StandardCopyOption.REPLACE_EXISTING)
Copy link
Collaborator

Choose a reason for hiding this comment

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

And same here, is this actually needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as this #1094

@raghulvelusamy raghulvelusamy force-pushed the fix_manifest branch 3 times, most recently from a9d29cc to 1ae9b7c Compare December 18, 2023 23:40
MODULE.bazel Outdated Show resolved Hide resolved
Comment on lines 452 to +455
expandWithSources(
SourceJarExtractor(
destDir = Paths.get(directories.temp).resolve("_srcjars"),
fileMatcher = IS_JVM_SOURCE_FILE,
destDir = Paths.get(directories.temp).resolve(SOURCE_JARS_DIR),
fileMatcher = { str: String -> IS_JVM_SOURCE_FILE.test(str) || "/$MANIFEST_DIR" in str },
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not fully aware of the whole compilation process but if we add this logic here, it will also run for jars generated by kapt (as it's based on whole input.sourceJarsList). Since the issue for missing manifest files seems to be related to ksp only, could we possibly put this expand logic in a place where it wouldn't affect kapt generated jars? 💭

Copy link
Collaborator

Choose a reason for hiding this comment

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

@zalewskise I think we actually want this for Kapt as wellsince the meta directory could contain files needed in the final jar. One example that comes to mind is google-auto-service where it spits out information that's needed in the final jar.

@restingbull restingbull merged commit 2a67a50 into bazelbuild:master Jan 26, 2024
1 of 2 checks passed
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.

6 participants