Reuse executable when no-change in project #44151
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Thevakumar-Luheerathan
requested review from
azinneera,
sameerajayasoma,
hasithaa,
gimantha and
MaryamZi
as code owners
| if (CommandUtil.isFilesModifiedSinceLastBuild(buildJson, project)) { | ||
| return true; | ||
| } | ||
| if (isRebuildForCurrCmd()) { |
There was a problem hiding this comment.
AFAIU, this method checks if there are any additional flags, and if that is the case, we rebuild. Can't we rename the method to reflect that?
| if (isRebuildForCurrCmd()) { | ||
| return true; | ||
| } | ||
| return !CommandUtil.isPrevCurrCmdCompatible(project.buildOptions(), buildJson.getBuildOptions()); |
There was a problem hiding this comment.
Suggested change
| return !CommandUtil.isPrevCurrCmdCompatible(project.buildOptions(), buildJson.getBuildOptions()); | |
| return !CommandUtil.isPrevAndCurrCmdCompatible(project.buildOptions(), buildJson.getBuildOptions()); |
Comment on lines
1303
to
1309
| if (isProjectFilesModified(buildJson, project)) { | ||
| return true; | ||
| } | ||
| if (isSettingsFileModified(buildJson)) { | ||
| return true; | ||
| } | ||
| return isExecutableModified(buildJson, project); |
There was a problem hiding this comment.
Suggested change
| if (isProjectFilesModified(buildJson, project)) { | |
| return true; | |
| } | |
| if (isSettingsFileModified(buildJson)) { | |
| return true; | |
| } | |
| return isExecutableModified(buildJson, project); | |
| return isProjectFilesModified(buildJson, project) || isSettingsFileModified(buildJson) || isExecutableModified(buildJson, project); |
Thevakumar-Luheerathan
force-pushed
the
improvment-1
branch
from
98912f8 to
b6a190b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #44136 by reusing the executable when no changes occur in the project. Key changes include:
- Adding new fields and getters/setters in BuildJson to record build metadata.
- Updating BuildOptions, command implementations, and various tasks to support skipping rebuilds when no modifications are detected.
- Enhancing test coverage for build/test scenarios and adjusting task execution logic (e.g. skipTask flag).
Reviewed Changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| compiler/ballerina-lang/src/main/java/io/ballerina/projects/internal/model/BuildJson.java | Added new metadata fields and associated accessors. |
| compiler/ballerina-lang/src/main/java/io/ballerina/projects/BuildOptions.java | Introduced a new method to retrieve locking mode. |
| cli/ballerina-cli/… | Updated multiple test and command files to support skipping rebuilds and reusing executable output. |
| cli/ballerina-cli/src/main/java/io/ballerina/cli/task/* | Introduced and propagated a skipTask flag to control task execution. |
| cli/ballerina-cli/src/main/java/io/ballerina/cli/cmd/* | Adjusted rebuild detection and task execution logic based on file changes. |
| cli/ballerina-cli/src/main/java/io/ballerina/cli/cmd/CommandUtil.java | Added SHA-256 digest computation and file modification checks for build fingerprinting. |
Comments suppressed due to low confidence (1)
cli/ballerina-cli/src/main/java/io/ballerina/cli/cmd/RunCommand.java:343
- [nitpick] It would be beneficial to add inline documentation for the isRebuildForCurrCmd() method explaining the rationale behind each condition to improve future maintainability.
}
|
|
||
| public static String getSHA256Digest(File fileToEvaluate) throws NoSuchAlgorithmException, IOException { | ||
| MessageDigest digest = MessageDigest.getInstance(SHA_256); | ||
| byte[] fileBytes = Files.readAllBytes(fileToEvaluate.toPath()); |
There was a problem hiding this comment.
Consider using a buffered stream (e.g., DigestInputStream) to compute the SHA-256 digest for large files instead of reading the entire file into memory.
Copilot uses AI. Check for mistakes.
Comment on lines
83
to
85
| if (!project.buildOptions().nativeImage()) { | ||
| this.out.println("Generating executable"); | ||
| this.out.println("Generating executable" + (skipTask ? "(skipped)" : "")); | ||
| } |
There was a problem hiding this comment.
[nitpick] The repeated pattern of checking 'skipTask' and printing '(skipped)' is seen in multiple tasks; consider abstracting this logic into a helper function to reduce duplication and improve readability.
Copilot uses AI. Check for mistakes.
Thevakumar-Luheerathan
force-pushed
the
improvment-1
branch
from
eb9d49d to
6618e4c
Compare
Thevakumar-Luheerathan
force-pushed
the
improvment-1
branch
from
6618e4c to
303d917
Compare
Thevakumar-Luheerathan
force-pushed
the
improvment-1
branch
from
ee7841f to
b5e0483
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Fixes #44136
Approach
Samples
Consecutive
bal build,bal run,bal testRemarks
Check List