feat: initial implementation of gradle plugin #73 #1005
Conversation
WalkthroughThis pull request introduces several classes and interfaces within the Changes
Possibly related PRs
Suggested labels
Poem
Tip New featuresWalkthrough comment now includes:
Notes:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Outside diff range, codebase verification and nitpick comments (3)
sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ResolveMainClassName.java (1)
49-203: Well-structured Gradle task class with clear responsibilities.The
ResolveMainClassNameclass is well-implemented with clear methods for setting and getting properties, and for resolving the main class name. The use of Gradle'sDefaultTaskand properties is appropriate, and the task-specific annotations are correctly applied.
- The method
resolveAndStoreMainClassNameeffectively handles file operations which are crucial for the task's functionality.- The use of
@DisableCachingByDefaultis justified as the task's output is not worth caching due to its dynamic nature based on the project's state.Consider adding error handling for file operations and classpath manipulations to enhance robustness.
sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkJar.java (1)
37-254: Well-structured Gradle task class for creating bootable JAR files.The
ArkJarclass is well-implemented with clear methods for configuring the JAR task and handling file operations. The use of Gradle'sJarclass and theBootArchiveinterface is appropriate, and the task-specific methods are correctly applied.
- The methods
copyandcreateCopyActioneffectively handle the JAR creation process, which is crucial for the task's functionality.- The use of properties like
mainClassandclasspathis well thought out, allowing for dynamic configuration based on the project's state.Consider adding more detailed error handling for file operations and JAR creation to enhance robustness.
sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkPluginAction.java (1)
47-221: Comprehensive Gradle task configuration class with robust functionality.The
ArkPluginActionclass is well-implemented with methods that comprehensively configure Gradle tasks and settings for the SofaArk framework. The use of theAction<Project>interface is appropriate, and the methods are well-organized and documented.
- The methods cover a broad range of configurations, from JAR task setup to compiler argument adjustments, which are crucial for the framework's functionality.
- The UTF-8 encoding setup and the handling of compiler arguments are particularly noteworthy for ensuring compatibility and performance.
Consider adding more detailed logging or debugging information to help trace the configuration steps during project builds.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkArchiveSupport.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkBizCopyAction.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkCopyAction.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkJar.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkPluginAction.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/BootArchive.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/DefaultTimeZoneOffset.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/JarTypeFileSpec.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/LoaderZipEntries.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ResolveMainClassName.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/SofaArkGradlePlugin.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/SofaArkGradlePluginExtension.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ZipCompression.java (1 hunks)
Files skipped from review due to trivial changes (1)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ZipCompression.java
Additional comments not posted (12)
sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/DefaultTimeZoneOffset.java (1)
23-51: Well-implemented utility class for time zone offset handling.The
DefaultTimeZoneOffsetclass is well-designed, using a singleton pattern for easy reuse across the project. The methods are clear and effectively documented, providing robust functionality for time manipulation relative to the default time zone.sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/BootArchive.java (4)
32-38: Well-implemented method for retrieving the main class.The use of
@InputandProperty<String>is appropriate and follows Gradle best practices for task inputs.
40-45: Flexible method for specifying unpack requirements.The use of varargs for specifying Ant-style patterns is flexible and user-friendly.
47-52: Enhanced flexibility with spec-based unpacking.Allowing the use of
Spec<FileTreeElement>provides advanced users more control over unpacking conditions.
55-61: Properly annotated method for classpath retrieval.The use of
@Optionaland@Classpathannotations is correct and aligns with Gradle's requirements for task classpaths.sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkArchiveSupport.java (3)
54-66: Well-implemented static initialization block.The static block correctly initializes the
DEFAULT_LAUNCHER_CLASSESwith an immutable set, which is a good practice for thread safety and to prevent accidental modifications.
86-96: Constructor initializes class fields effectively.The constructor of
ArkArchiveSupportis well-structured and initializes multiple class fields. It also sets up the manifest and parses Git information, which are crucial for the functionality of this class.Consider verifying the robustness of the
JGitParser.parse(gitDic)method in handling different edge cases or potential exceptions.
99-113: Effective use of Java Map methods in manifest configuration.The method
configureManifestusesputIfAbsentandcomputeIfAbsenteffectively to ensure that manifest entries are not inadvertently overwritten. This approach maintains the integrity of existing manifest data while allowing new data to be added conditionally.sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkCopyAction.java (4)
115-124: Review of execute MethodThe
executemethod inArkCopyActionclass handles the core functionality of copying actions. Here are some observations:
- Error Handling: The method catches
IOExceptionand wraps it in aGradleException, which is a good practice as it provides a clear message about the failure context.- WorkResult Usage: The method returns a
WorkResultobject, which is appropriate for tasks in Gradle that can either succeed or fail.This method is implemented correctly and follows good error handling practices.
126-149: Review of writeArchive MethodsThere are overloaded
writeArchivemethods designed to handle the writing of the archive with detailed error handling and resource management:
- Resource Management: The use of try-with-resources or finally blocks to ensure streams are closed is commendable.
- Separation of Concerns: The method that accepts an
OutputStreamas a parameter allows for better testing and reusability by decoupling the file writing logic from stream management.These methods are well-implemented with proper resource management and error handling.
151-165: Review of closeQuietly MethodThe
closeQuietlymethod is used to close streams without throwing exceptions. This is a utility method often used to simplify stream handling in finally blocks. However, consider the following:
- Silent Failure: The method catches and ignores
IOException. While this is typical for a closeQuietly method, it's essential to ensure that this silent failure is acceptable in all use cases wherecloseQuietlyis used.This method is generally well-implemented for its intended use, but ensure that ignoring exceptions is acceptable in all cases where this method is used.
183-209: Review of InputStreamEntryWriter ClassThe
InputStreamEntryWriterclass is an implementation of theEntryWriterinterface, designed to write data from anInputStreamto anOutputStream. Observations:
- Resource Management: The class takes care of closing the
InputStreamif thecloseflag is true, which is good practice.- Buffer Size: The buffer size is set to 32KB, which is a reasonable default for file I/O operations.
This class is correctly implemented and follows good practices for I/O operations.
| class JarTypeFileSpec implements Spec<File> { | ||
|
|
||
| private static final Set<String> EXCLUDED_JAR_TYPES = Collections.singleton("dependencies-starter"); | ||
|
|
||
| @Override | ||
| public boolean isSatisfiedBy(File file) { | ||
| try (JarFile jar = new JarFile(file)) { | ||
| String jarType = jar.getManifest().getMainAttributes().getValue("Spring-Boot-Jar-Type"); | ||
| if (jarType != null && EXCLUDED_JAR_TYPES.contains(jarType)) { | ||
| return false; | ||
| } | ||
| } | ||
| catch (Exception ex) { | ||
| // Continue | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Overall implementation looks good, but consider enhancing exception handling.
The class JarTypeFileSpec effectively filters files based on the jar type. However, the exception handling in the isSatisfiedBy method is minimal and could potentially suppress important errors that should be addressed or logged.
Consider logging the exception or rethrowing it as a specific type that could be handled upstream.
| public class SofaArkGradlePlugin implements Plugin<Project> { | ||
|
|
||
| public static final String ARK_VERSION = "2.2.14"; | ||
| public static final String ARK_BIZ_CONFIGURATION_NAME = "bizJar"; | ||
| public static final String ARK_CONFIGURATION_NAME = "arkJar"; | ||
| public static final String ARK_BIZ_TASK_NAME = "bizJar"; | ||
| public static final String ARK_TASK_NAME = "arkJar"; | ||
| public static final String DEVELOPMENT_ONLY_CONFIGURATION_NAME = "developmentOnly"; | ||
| public static final String PRODUCTION_RUNTIME_CLASSPATH_CONFIGURATION_NAME = "productionRuntimeClasspath"; | ||
|
|
||
| @Override | ||
| public void apply(Project project) { | ||
| verifyGradleVersion(); | ||
| createAndSetExtension(project); | ||
| registerPluginActions(project); | ||
| } | ||
|
|
||
| private void verifyGradleVersion() { | ||
| GradleVersion currentVersion = GradleVersion.current(); | ||
| if (currentVersion.compareTo(GradleVersion.version("6.8")) < 0) { | ||
| throw new GradleException("Spring Boot plugin requires Gradle 6.8.+ " | ||
| + "The current version is " + currentVersion); | ||
| } | ||
| } | ||
|
|
||
| private void createAndSetExtension(Project project) { | ||
| SofaArkGradlePluginExtension arkExtension = project.getExtensions().create("arkConfig", SofaArkGradlePluginExtension.class, project); | ||
|
|
||
| } | ||
|
|
||
| private void registerPluginActions(Project project) { | ||
| ArkPluginAction arkAction = new ArkPluginAction(); | ||
| arkAction.execute(project); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Solid implementation with a suggestion for enhanced error handling.
The SofaArkGradlePlugin class is well-structured and effectively sets up the necessary configurations and actions for the SofaArk framework within a Gradle environment. The enforcement of a minimum Gradle version is crucial and well-handled.
Consider enhancing the error message in the verifyGradleVersion method to provide more detailed guidance on how to resolve the version issue, possibly including a link to documentation or support resources.
| /** | ||
| * Sets the classpath to include in the archive. The given {@code classpath} is | ||
| * evaluated as per {@link Project#files(Object...)}. | ||
| * @param classpath the classpath | ||
| * @since 2.0.7 | ||
| */ | ||
| void setClasspath(Object classpath); | ||
|
|
||
| /** | ||
| * Sets the classpath to include in the archive. | ||
| * @param classpath the classpath | ||
| * @since 2.0.7 | ||
| */ | ||
| void setClasspath(FileCollection classpath); |
There was a problem hiding this comment.
Clarify usage of overloaded setClasspath methods.
While the flexibility of overloading the setClasspath method is beneficial, it could lead to confusion. Consider enhancing the documentation or providing usage examples to clarify when to use each variant.
| private void buildArkManifest(){ | ||
| this.arkManifest.getMainAttributes().putValue("Manifest-Version", "1.0"); | ||
| this.arkManifest.getMainAttributes().putValue("Main-Class", this.loaderMainClass); | ||
| this.arkManifest.getMainAttributes().putValue("Start-Class", this.loaderMainClass); | ||
| this.arkManifest.getMainAttributes().putValue("Sofa-Ark-Version",this.arkVersion); | ||
| this.arkManifest.getMainAttributes().putValue("Ark-Container-Root","SOFA-ARK/container/"); | ||
| this.arkManifest.getMainAttributes().putValue("build-time", | ||
| new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssZ").format(new Date())); | ||
|
|
||
| if (gitInfo != null) { | ||
| this.arkManifest.getMainAttributes().putValue("remote-origin-url", gitInfo.getRepository()); | ||
| this.arkManifest.getMainAttributes().putValue("commit-branch", gitInfo.getBranchName()); | ||
| this.arkManifest.getMainAttributes().putValue("commit-id", gitInfo.getLastCommitId()); | ||
| this.arkManifest.getMainAttributes().putValue("commit-user-name", gitInfo.getLastCommitUser()); | ||
| this.arkManifest.getMainAttributes() | ||
| .putValue("commit-user-email", gitInfo.getLastCommitEmail()); | ||
| this.arkManifest.getMainAttributes().putValue("COMMIT_TIME", gitInfo.getLastCommitDateTime()); | ||
| this.arkManifest.getMainAttributes().putValue("COMMIT_TIMESTAMP", | ||
| String.valueOf(gitInfo.getLastCommitTime())); | ||
| this.arkManifest.getMainAttributes().putValue("build-user", gitInfo.getBuildUser()); | ||
| this.arkManifest.getMainAttributes().putValue("build-email", gitInfo.getBuildEmail()); | ||
| } |
There was a problem hiding this comment.
Comprehensive manifest building with a suggestion for date handling.
The method buildArkManifest effectively includes detailed build and version information in the manifest. However, consider specifying a locale or timezone in SimpleDateFormat to avoid potential inconsistencies in time representation across different environments.
Consider using SimpleDateFormat with a specific timezone setting to ensure consistent date formatting:
new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssZ", Locale.US).setTimeZone(TimeZone.getTimeZone("UTC"));| public CopyAction createCopyAction(Jar jar) throws IOException { | ||
| return createCopyAction(jar, null); | ||
| } | ||
|
|
||
| public CopyAction createCopyAction(Jar jar, String layerToolsLocation) throws IOException { | ||
| File bizOutput = getTargetFile(jar, this.arkExtension.getBizClassifier().get()); | ||
| File arkOutput = getTargetFile(jar, this.arkExtension.getArkClassifier().get()); | ||
|
|
||
| System.out.println("bizOutput:"+bizOutput.getAbsolutePath()); | ||
| System.out.println("arkOutput:"+arkOutput.getAbsolutePath()); | ||
|
|
||
| Manifest manifest = jar.getManifest(); | ||
| boolean preserveFileTimestamps = jar.isPreserveFileTimestamps(); | ||
| Integer dirMode = getDirMode(jar); | ||
| Integer fileMode = getFileMode(jar); | ||
| boolean includeDefaultLoader = isUsingDefaultLoader(jar); | ||
| Spec<FileTreeElement> requiresUnpack = this.requiresUnpack.getAsSpec(); | ||
| Spec<FileTreeElement> exclusions = this.exclusions.getAsExcludeSpec(); | ||
| Spec<FileCopyDetails> librarySpec = this.librarySpec; | ||
| Function<FileCopyDetails, ZipCompression> compressionResolver = this.compressionResolver; | ||
| String encoding = jar.getMetadataCharset(); | ||
|
|
||
| CopyAction action = new ArkBizCopyAction(bizOutput,arkOutput, manifest, preserveFileTimestamps, dirMode, fileMode, | ||
| includeDefaultLoader, requiresUnpack, exclusions, librarySpec, | ||
| compressionResolver, encoding, this.arkManifest); | ||
|
|
||
|
|
||
| return jar.isReproducibleFileOrder() ? new ReproducibleOrderingCopyAction(action) : action; | ||
| } |
There was a problem hiding this comment.
Complex method handling file copy actions effectively.
The createCopyAction method is well-implemented, handling various configurations and conditions to set up the appropriate CopyAction. Due to its complexity, consider adding more inline comments to explain the logic behind each configuration step, especially around the conditions that affect the choice of CopyAction.
Add more inline comments to improve readability and maintainability of the method, especially explaining the conditions and their impact on the behavior of the method.
| class LoaderZipEntries { | ||
|
|
||
| private final Long entryTime; | ||
|
|
||
| private final int dirMode; | ||
|
|
||
| private final int fileMode; | ||
|
|
||
| LoaderZipEntries(Long entryTime, int dirMode, int fileMode) { | ||
| this.entryTime = entryTime; | ||
| this.dirMode = dirMode; | ||
| this.fileMode = fileMode; | ||
| } | ||
|
|
||
| WrittenEntries writeTo(ZipArchiveOutputStream out) throws IOException { | ||
| WrittenEntries written = new WrittenEntries(); | ||
| try (ZipInputStream loaderJar = new ZipInputStream( | ||
| getClass().getResourceAsStream("/META-INF/loader/spring-boot-loader.jar"))) { | ||
| ZipEntry entry = loaderJar.getNextEntry(); | ||
| while (entry != null) { | ||
| if (entry.isDirectory() && !entry.getName().equals("META-INF/")) { | ||
| writeDirectory(new ZipArchiveEntry(entry), out); | ||
| written.addDirectory(entry); | ||
| } | ||
| else if (entry.getName().endsWith(".class")) { | ||
| writeClass(new ZipArchiveEntry(entry), loaderJar, out); | ||
| written.addFile(entry); | ||
| } | ||
| entry = loaderJar.getNextEntry(); | ||
| } | ||
| } | ||
| return written; | ||
| } | ||
|
|
||
| private void writeDirectory(ZipArchiveEntry entry, ZipArchiveOutputStream out) throws IOException { | ||
| prepareEntry(entry, this.dirMode); | ||
| out.putArchiveEntry(entry); | ||
| out.closeArchiveEntry(); | ||
| } | ||
|
|
||
| private void writeClass(ZipArchiveEntry entry, ZipInputStream in, ZipArchiveOutputStream out) throws IOException { | ||
| prepareEntry(entry, this.fileMode); | ||
| out.putArchiveEntry(entry); | ||
| copy(in, out); | ||
| out.closeArchiveEntry(); | ||
| } | ||
|
|
||
| private void prepareEntry(ZipArchiveEntry entry, int unixMode) { | ||
| if (this.entryTime != null) { | ||
| entry.setTime(DefaultTimeZoneOffset.INSTANCE.removeFrom(this.entryTime)); | ||
| } | ||
| entry.setUnixMode(unixMode); | ||
| } | ||
|
|
||
| private void copy(InputStream in, OutputStream out) throws IOException { | ||
| System.out.println("55555555555555555555555555555"); | ||
| StreamUtils.copy(in, out); | ||
| } | ||
|
|
||
| /** | ||
| * Tracks entries that have been written. | ||
| */ | ||
| static class WrittenEntries { | ||
|
|
||
| private final Set<String> directories = new LinkedHashSet<>(); | ||
|
|
||
| private final Set<String> files = new LinkedHashSet<>(); | ||
|
|
||
| private void addDirectory(ZipEntry entry) { | ||
| this.directories.add(entry.getName()); | ||
| } | ||
|
|
||
| private void addFile(ZipEntry entry) { | ||
| this.files.add(entry.getName()); | ||
| } | ||
|
|
||
| boolean isWrittenDirectory(FileTreeElement element) { | ||
| String path = element.getRelativePath().getPathString(); | ||
| if (element.isDirectory() && !path.endsWith(("/"))) { | ||
| path += "/"; | ||
| } | ||
| return this.directories.contains(path); | ||
| } | ||
|
|
||
| Set<String> getFiles() { | ||
| return this.files; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Review of LoaderZipEntries class
The LoaderZipEntries class is generally well-implemented and uses Java's try-with-resources effectively. However, it contains debug print statements (e.g., line 93) that should be removed for production code. Additionally, the exception handling could be improved to provide more specific error messages or handle specific cases.
Remove debug print statements:
- System.out.println("55555555555555555555555555555");Improve exception handling by adding specific catch blocks or error messages where applicable.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class LoaderZipEntries { | |
| private final Long entryTime; | |
| private final int dirMode; | |
| private final int fileMode; | |
| LoaderZipEntries(Long entryTime, int dirMode, int fileMode) { | |
| this.entryTime = entryTime; | |
| this.dirMode = dirMode; | |
| this.fileMode = fileMode; | |
| } | |
| WrittenEntries writeTo(ZipArchiveOutputStream out) throws IOException { | |
| WrittenEntries written = new WrittenEntries(); | |
| try (ZipInputStream loaderJar = new ZipInputStream( | |
| getClass().getResourceAsStream("/META-INF/loader/spring-boot-loader.jar"))) { | |
| ZipEntry entry = loaderJar.getNextEntry(); | |
| while (entry != null) { | |
| if (entry.isDirectory() && !entry.getName().equals("META-INF/")) { | |
| writeDirectory(new ZipArchiveEntry(entry), out); | |
| written.addDirectory(entry); | |
| } | |
| else if (entry.getName().endsWith(".class")) { | |
| writeClass(new ZipArchiveEntry(entry), loaderJar, out); | |
| written.addFile(entry); | |
| } | |
| entry = loaderJar.getNextEntry(); | |
| } | |
| } | |
| return written; | |
| } | |
| private void writeDirectory(ZipArchiveEntry entry, ZipArchiveOutputStream out) throws IOException { | |
| prepareEntry(entry, this.dirMode); | |
| out.putArchiveEntry(entry); | |
| out.closeArchiveEntry(); | |
| } | |
| private void writeClass(ZipArchiveEntry entry, ZipInputStream in, ZipArchiveOutputStream out) throws IOException { | |
| prepareEntry(entry, this.fileMode); | |
| out.putArchiveEntry(entry); | |
| copy(in, out); | |
| out.closeArchiveEntry(); | |
| } | |
| private void prepareEntry(ZipArchiveEntry entry, int unixMode) { | |
| if (this.entryTime != null) { | |
| entry.setTime(DefaultTimeZoneOffset.INSTANCE.removeFrom(this.entryTime)); | |
| } | |
| entry.setUnixMode(unixMode); | |
| } | |
| private void copy(InputStream in, OutputStream out) throws IOException { | |
| System.out.println("55555555555555555555555555555"); | |
| StreamUtils.copy(in, out); | |
| } | |
| /** | |
| * Tracks entries that have been written. | |
| */ | |
| static class WrittenEntries { | |
| private final Set<String> directories = new LinkedHashSet<>(); | |
| private final Set<String> files = new LinkedHashSet<>(); | |
| private void addDirectory(ZipEntry entry) { | |
| this.directories.add(entry.getName()); | |
| } | |
| private void addFile(ZipEntry entry) { | |
| this.files.add(entry.getName()); | |
| } | |
| boolean isWrittenDirectory(FileTreeElement element) { | |
| String path = element.getRelativePath().getPathString(); | |
| if (element.isDirectory() && !path.endsWith(("/"))) { | |
| path += "/"; | |
| } | |
| return this.directories.contains(path); | |
| } | |
| Set<String> getFiles() { | |
| return this.files; | |
| } | |
| } | |
| } | |
| class LoaderZipEntries { | |
| private final Long entryTime; | |
| private final int dirMode; | |
| private final int fileMode; | |
| LoaderZipEntries(Long entryTime, int dirMode, int fileMode) { | |
| this.entryTime = entryTime; | |
| this.dirMode = dirMode; | |
| this.fileMode = fileMode; | |
| } | |
| WrittenEntries writeTo(ZipArchiveOutputStream out) throws IOException { | |
| WrittenEntries written = new WrittenEntries(); | |
| try (ZipInputStream loaderJar = new ZipInputStream( | |
| getClass().getResourceAsStream("/META-INF/loader/spring-boot-loader.jar"))) { | |
| ZipEntry entry = loaderJar.getNextEntry(); | |
| while (entry != null) { | |
| if (entry.isDirectory() && !entry.getName().equals("META-INF/")) { | |
| writeDirectory(new ZipArchiveEntry(entry), out); | |
| written.addDirectory(entry); | |
| } | |
| else if (entry.getName().endsWith(".class")) { | |
| writeClass(new ZipArchiveEntry(entry), loaderJar, out); | |
| written.addFile(entry); | |
| } | |
| entry = loaderJar.getNextEntry(); | |
| } | |
| } | |
| return written; | |
| } | |
| private void writeDirectory(ZipArchiveEntry entry, ZipArchiveOutputStream out) throws IOException { | |
| prepareEntry(entry, this.dirMode); | |
| out.putArchiveEntry(entry); | |
| out.closeArchiveEntry(); | |
| } | |
| private void writeClass(ZipArchiveEntry entry, ZipInputStream in, ZipArchiveOutputStream out) throws IOException { | |
| prepareEntry(entry, this.fileMode); | |
| out.putArchiveEntry(entry); | |
| copy(in, out); | |
| out.closeArchiveEntry(); | |
| } | |
| private void prepareEntry(ZipArchiveEntry entry, int unixMode) { | |
| if (this.entryTime != null) { | |
| entry.setTime(DefaultTimeZoneOffset.INSTANCE.removeFrom(this.entryTime)); | |
| } | |
| entry.setUnixMode(unixMode); | |
| } | |
| private void copy(InputStream in, OutputStream out) throws IOException { | |
| StreamUtils.copy(in, out); | |
| } | |
| /** | |
| * Tracks entries that have been written. | |
| */ | |
| static class WrittenEntries { | |
| private final Set<String> directories = new LinkedHashSet<>(); | |
| private final Set<String> files = new LinkedHashSet<>(); | |
| private void addDirectory(ZipEntry entry) { | |
| this.directories.add(entry.getName()); | |
| } | |
| private void addFile(ZipEntry entry) { | |
| this.files.add(entry.getName()); | |
| } | |
| boolean isWrittenDirectory(FileTreeElement element) { | |
| String path = element.getRelativePath().getPathString(); | |
| if (element.isDirectory() && !path.endsWith(("/"))) { | |
| path += "/"; | |
| } | |
| return this.directories.contains(path); | |
| } | |
| Set<String> getFiles() { | |
| return this.files; | |
| } | |
| } | |
| } |
| this.out.write(data, 0, data.length); | ||
| this.out.closeArchiveEntry(); | ||
| } | ||
|
|
||
| private void prepareStoredEntry(FileCopyDetails details, ZipArchiveEntry archiveEntry) throws IOException { | ||
| prepareStoredEntry(details.open(), archiveEntry); | ||
| if (ArkBizCopyAction.this.requiresUnpack.isSatisfiedBy(details)) { | ||
| archiveEntry.setComment("UNPACK:" + FileUtils.sha1Hash(details.getFile())); | ||
| } | ||
| } | ||
|
|
||
| private void prepareStoredEntry(InputStream input, ZipArchiveEntry archiveEntry) throws IOException { | ||
| new CrcAndSize(input).setUpStoredEntry(archiveEntry); | ||
| } | ||
|
|
||
| private Long getTime() { | ||
| return getTime(null); | ||
| } | ||
|
|
||
| private Long getTime(FileCopyDetails details) { | ||
| if (!ArkBizCopyAction.this.preserveFileTimestamps) { | ||
| return CONSTANT_TIME_FOR_ZIP_ENTRIES; | ||
| } | ||
| if (details != null) { | ||
| return details.getLastModified(); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private int getDirMode() { | ||
| return (ArkBizCopyAction.this.dirMode != null) ? ArkBizCopyAction.this.dirMode | ||
| : UnixStat.DIR_FLAG | UnixStat.DEFAULT_DIR_PERM; | ||
| } | ||
|
|
||
| private int getFileMode() { | ||
| return (ArkBizCopyAction.this.fileMode != null) ? ArkBizCopyAction.this.fileMode | ||
| : UnixStat.FILE_FLAG | UnixStat.DEFAULT_FILE_PERM; | ||
| } | ||
|
|
||
| private int getFileMode(FileCopyDetails details) { | ||
| return (ArkBizCopyAction.this.fileMode != null) ? ArkBizCopyAction.this.fileMode | ||
| : UnixStat.FILE_FLAG | getPermissions(details); | ||
| } | ||
|
|
||
| private int getPermissions(FileCopyDetails details) { | ||
| if (GradleVersion.current().compareTo(GradleVersion.version("8.3")) >= 0) { | ||
| try { | ||
| Object permissions = details.getClass().getMethod("getPermissions").invoke(details); | ||
| return (int) permissions.getClass().getMethod("toUnixNumeric").invoke(permissions); | ||
| } | ||
| catch (Exception ex) { | ||
| throw new GradleException("Failed to get permissions", ex); | ||
| } | ||
| } | ||
| return details.getMode(); | ||
| } | ||
|
|
||
|
|
||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Data holder for CRC and Size. | ||
| */ | ||
| private static class CrcAndSize { | ||
|
|
||
| private static final int BUFFER_SIZE = 32 * 1024; | ||
|
|
||
| private final CRC32 crc = new CRC32(); | ||
|
|
||
| private long size; | ||
|
|
||
| CrcAndSize(InputStream inputStream) throws IOException { | ||
| try { | ||
| load(inputStream); | ||
| } | ||
| finally { | ||
| inputStream.close(); | ||
| } | ||
| } | ||
|
|
||
| private void load(InputStream inputStream) throws IOException { | ||
| byte[] buffer = new byte[BUFFER_SIZE]; | ||
| int bytesRead; | ||
| while ((bytesRead = inputStream.read(buffer)) != -1) { | ||
| this.crc.update(buffer, 0, bytesRead); | ||
| this.size += bytesRead; | ||
| } | ||
| } | ||
|
|
||
| void setUpStoredEntry(ZipArchiveEntry entry) { | ||
| entry.setSize(this.size); | ||
| entry.setCompressedSize(this.size); | ||
| entry.setCrc(this.crc.getValue()); | ||
| entry.setMethod(ZipEntry.STORED); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Review of ArkBizCopyAction class
The ArkBizCopyAction class is complex and handles many aspects of file copying and ZIP handling effectively using Java's try-with-resources and functional interfaces. However, it contains debug print statements (e.g., lines 126, 148, 151) that should be removed for production code. The class could also benefit from additional comments or refactoring for better readability and maintainability.
Remove debug print statements:
- System.out.println("``````````````````````````````````````````````````````````````````````````````````");
- System.out.println("111111111111111111111111111111111");
- System.out.println("2222222222222222222222222222");Add more comments or refactor for clarity, especially around complex logic sections.
Committable suggestion was skipped due to low confidence.
| public class ArkCopyAction implements CopyAction { | ||
|
|
||
| static final long CONSTANT_TIME_FOR_ZIP_ENTRIES = OffsetDateTime.of(1980, 2, 1, 0, 0, 0, 0, ZoneOffset.UTC) | ||
| .toInstant() | ||
| .toEpochMilli(); | ||
|
|
||
| private final File output; | ||
|
|
||
| private final Manifest manifest; | ||
|
|
||
| private final boolean preserveFileTimestamps; | ||
|
|
||
| private final Integer dirMode; | ||
|
|
||
| private final Integer fileMode; | ||
|
|
||
| private final boolean includeDefaultLoader; | ||
|
|
||
| private final Spec<FileTreeElement> requiresUnpack; | ||
|
|
||
| private final Spec<FileTreeElement> exclusions; | ||
|
|
||
| private final Spec<FileCopyDetails> librarySpec; | ||
|
|
||
| private final Function<FileCopyDetails, ZipCompression> compressionResolver; | ||
|
|
||
| private final String encoding; | ||
|
|
||
| private final JarOutputStream jarOutput; | ||
|
|
||
| private final File arkOutput; | ||
|
|
||
|
|
||
|
|
||
| ArkCopyAction(File output,File arkOutput, Manifest manifest, boolean preserveFileTimestamps, Integer dirMode, Integer fileMode, | ||
| boolean includeDefaultLoader, Spec<FileTreeElement> requiresUnpack, | ||
| Spec<FileTreeElement> exclusions, Spec<FileCopyDetails> librarySpec, | ||
| Function<FileCopyDetails, ZipCompression> compressionResolver, String encoding | ||
| ) throws IOException { | ||
| this.output = output; | ||
| this.arkOutput = arkOutput; | ||
| this.manifest = manifest; | ||
| this.preserveFileTimestamps = preserveFileTimestamps; | ||
| this.dirMode = dirMode; | ||
| this.fileMode = fileMode; | ||
| this.includeDefaultLoader = includeDefaultLoader; | ||
| this.requiresUnpack = requiresUnpack; | ||
| this.exclusions = exclusions; | ||
| this.librarySpec = librarySpec; | ||
| this.compressionResolver = compressionResolver; | ||
| this.encoding = encoding; | ||
| FileOutputStream fileOutputStream = new FileOutputStream(this.output); | ||
| this.jarOutput = new JarOutputStream(fileOutputStream); | ||
| } |
There was a problem hiding this comment.
Review of ArkCopyAction Constructor
The constructor of ArkCopyAction is well-structured and initializes several fields based on the parameters it receives. However, there are a few points to consider:
- Exception Handling: The constructor throws
IOException, which is appropriate given that it deals with file operations. Ensure that callers of this constructor handle this exception properly. - Parameter Validation: There is no explicit validation of the parameters (e.g., checking if any are
null). Adding null checks could prevent potentialNullPointerExceptionsduring runtime. - Resource Management: The constructor initializes
JarOutputStreamonFileOutputStream. It's crucial to ensure that these streams are closed properly in case of exceptions to avoid resource leaks.
Consider adding parameter validation and enhancing exception handling.
| /** | ||
| * Internal process used to copy {@link FileCopyDetails file details} to the zip file. | ||
| */ | ||
| private class Processor { | ||
|
|
||
| private final ZipArchiveOutputStream out; | ||
|
|
||
| private final JarArchiveOutputStream jarOutput; | ||
|
|
||
| private File arkFile; | ||
|
|
||
|
|
||
| private LoaderZipEntries.WrittenEntries writtenLoaderEntries; | ||
|
|
||
| private final Set<String> writtenDirectories = new LinkedHashSet<>(); | ||
|
|
||
| private final Set<String> writtenLibraries = new LinkedHashSet<>(); | ||
|
|
||
|
|
||
| Processor(ZipArchiveOutputStream out, JarArchiveOutputStream outputStream) throws IOException { | ||
| this.out = out; | ||
| jarOutput = outputStream; | ||
| } | ||
|
|
||
| void process(FileCopyDetails details) { | ||
| if(details.getName().contains("sofa-ark-all")){ | ||
| this.arkFile = details.getFile(); | ||
| return; | ||
| } | ||
| if (skipProcessing(details)) { | ||
| return; | ||
| } | ||
| try { | ||
| //writeLoaderEntriesIfNecessary(details); | ||
| if (details.isDirectory()) { | ||
| processDirectory(details); | ||
| } | ||
| else { | ||
| processFile(details); | ||
| } | ||
| } | ||
| catch (IOException ex) { | ||
| throw new GradleException("Failed to add " + details + " to " + ArkCopyAction.this.output, ex); | ||
| } | ||
| } |
There was a problem hiding this comment.
Review of Processor Class
The Processor class is a crucial component within ArkCopyAction, handling the actual file and directory processing for the archive. Key points:
- Complexity: The class is quite complex and handles multiple responsibilities, including file and directory processing, and special cases like handling
sofa-ark-allfiles. - Error Handling: Proper exception handling is in place, with specific errors wrapped in
GradleExceptionproviding context about what failed.
Consider refactoring to reduce complexity and improve maintainability, possibly by breaking down responsibilities into smaller, more focused methods or classes.
| System.out.println("||"+entry.getName()); | ||
| if (entry.getName().contains("sofa-ark-archive") | ||
| || entry.getName().contains("sofa-ark-spi") | ||
| || entry.getName().contains("sofa-ark-common")) { | ||
|
|
||
| JarInputStream inputStream = new JarInputStream(new BufferedInputStream( | ||
| jarFileSource.getInputStream(entry))); | ||
| writeLoaderClasses(inputStream, jarFileSource); | ||
| } | ||
| } | ||
| } catch (NullPointerException exception){ | ||
| throw new RuntimeException("No sofa-ark-all file find, please configure it"); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| private void writeLoaderClasses(JarInputStream jarInputStream, JarFile jarFileSource) throws IOException { | ||
| JarEntry entry; | ||
| while ((entry = jarInputStream.getNextJarEntry()) != null) { | ||
| if (entry.getName().endsWith(".class") | ||
| && (entry.getName().contains("com/alipay/sofa/ark/spi/archive") | ||
| || entry.getName().contains("com/alipay/sofa/ark/loader") | ||
| || entry.getName().contains("com/alipay/sofa/ark/bootstrap") | ||
| || entry.getName().contains("com/alipay/sofa/ark/common/util/StringUtils") | ||
| || entry.getName().contains("com/alipay/sofa/ark/common/util/AssertUtils") || entry | ||
| .getName().contains("com/alipay/sofa/ark/spi/constant"))) { | ||
|
|
||
|
|
||
| // byte[] buffer = new byte[BUFFER_SIZE]; | ||
| // int bytesRead; | ||
| // while ((bytesRead = this.inputStream.read(buffer)) != -1) { | ||
| // outputStream.write(buffer, 0, bytesRead); | ||
| // } | ||
|
|
||
| ZipArchiveEntry zipArchiveEntry = new ZipArchiveEntry(entry.getName()); | ||
| prepareEntry(zipArchiveEntry, entry.getName(), getTime(), getFileMode()); | ||
| this.out.putArchiveEntry(zipArchiveEntry); | ||
|
|
||
| byte[] bytes = new byte[1024]; | ||
| int length; | ||
| while ((length = jarInputStream.read(bytes)) >= 0) { | ||
| this.out.write(bytes, 0, length); | ||
| } | ||
| // this.out.write(entry.); | ||
| this.out.closeArchiveEntry(); | ||
|
|
||
|
|
||
| } | ||
| } | ||
| jarInputStream.close(); | ||
| } | ||
|
|
||
| private void writeEntry(JarEntry entry, EntryWriter entryWriter, JarFile jarFileSource) throws IOException { | ||
| System.out.println(">>>>>>>>>>>>>>>>>>>>>>>>>>>>>"+entry.getName()); | ||
|
|
||
| JarArchiveEntry newEntry = new JarArchiveEntry(entry.getName()); | ||
| newEntry.setSize(entry.getSize()); | ||
| newEntry.setTime(entry.getTime()); | ||
|
|
||
| jarOutput.putArchiveEntry(newEntry); | ||
|
|
||
| // | ||
| try (InputStream is = jarFileSource.getInputStream(entry)) { | ||
| IOUtils.copy(is, jarOutput); | ||
| } | ||
|
|
||
| jarOutput.closeArchiveEntry(); | ||
|
|
||
|
|
||
| // String parent = entry.getName(); | ||
| // if (parent.endsWith("/")) { | ||
| // parent = parent.substring(0, parent.length() - 1); | ||
| // } | ||
| // if (parent.lastIndexOf("/") != -1) { | ||
| // parent = parent.substring(0, parent.lastIndexOf("/") + 1); | ||
| // if (parent.length() > 0) { | ||
| // writeEntry(new JarEntry(parent), null); | ||
| // } | ||
| // } | ||
| // | ||
| // | ||
| // this.jarOutput.putNextEntry(entry); | ||
| // if (entryWriter != null) { | ||
| // entryWriter.write(this.jarOutput); | ||
| // } | ||
| // this.jarOutput.closeEntry(); | ||
|
|
||
| } | ||
|
|
||
|
|
||
|
|
||
| private void writeArkBizMark() throws IOException { | ||
| String str = "a mark file included in sofa-ark module."; | ||
| String name = "com/alipay/sofa/ark/biz/mark"; | ||
| ZipArchiveEntry entry = new ZipArchiveEntry(name); | ||
| prepareEntry(entry, name, getTime(), getFileMode()); | ||
| this.out.putArchiveEntry(entry); | ||
| this.out.write(str.getBytes()); | ||
| this.out.closeArchiveEntry(); | ||
| } |
There was a problem hiding this comment.
Review of writeArkBizMark Method
The writeArkBizMark method adds a specific entry to the archive, which seems to be a marker for the sofa-ark module. Observations:
- Hardcoded Values: The method contains hardcoded strings for both the entry name and the content. These could be extracted as constants or configurable properties to enhance flexibility and maintainability.
- Single Responsibility: The method is focused and handles only one specific task, which is good for maintainability.
Consider extracting hardcoded values to constants or configuration settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkArchiveSupport.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkBizCopyAction.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkCopyAction.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkJar.java (1 hunks)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/LoaderZipEntries.java (1 hunks)
Files skipped from review due to trivial changes (1)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkJar.java
Files skipped from review as they are similar to previous changes (4)
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkArchiveSupport.java
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkBizCopyAction.java
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/ArkCopyAction.java
- sofa-ark-parent/support/ark-gradle-plugin/src/main/java/com/alipay/sofa/ark/plugin/LoaderZipEntries.java
Motivation
Related issue: #73
Enable SofaArk to support building Biz and Ark packages in the Gradle environment. Subsequent implementations, documentation, testing, and optimizations will be carried out in this PR.
Summary by CodeRabbit
New Features
ArkArchiveSupport,ArkBizCopyAction, andArkCopyAction.ArkJarclass.spring-boot-loader.jarto enhance class file organization.Documentation
Bug Fixes