[tool](fe) add fast-compile-fe.sh for incremental FE compilation#61429
[tool](fe) add fast-compile-fe.sh for incremental FE compilation#61429englefly wants to merge 3 commits intoapache:masterfrom
Conversation
Add `tools/fast-compile-fe.sh`, a developer utility that incrementally compiles only changed Java source files and patches them directly into `doris-fe.jar`, avoiding a full Maven rebuild during development. Key features: - Auto-detects stale `.java` files by comparing mtime with `.class` files - Supports explicit file arguments (by path or filename search under src/) - Caches the classpath derived from `target/lib` to skip repeated mvn calls - Updates both `target/doris-fe.jar` and `output/fe/lib/doris-fe.jar` - Handles inner classes and anonymous classes (e.g. `Foo$Bar.class`) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
|
run buildall |
|
run buildall |
|
/review |
| FE_CORE="$DORIS_HOME/fe/fe-core" | ||
| SRC_ROOT="$FE_CORE/src/main/java" | ||
| TARGET_CLASSES="$FE_CORE/target/classes" | ||
| TARGET_LIB="$FE_CORE/target/lib" | ||
| OUTPUT_JAR="$DORIS_HOME/output/fe/lib/doris-fe.jar" | ||
| TARGET_JAR="$FE_CORE/target/doris-fe.jar" | ||
| CP_CACHE="$FE_CORE/target/fast-compile-cp.txt" | ||
|
|
||
| # Generated source directories (protobuf/thrift/annotation processor generated java files) | ||
| GEN_SOURCES=( | ||
| "$FE_CORE/target/generated-sources/doris" | ||
| "$FE_CORE/target/generated-sources/org" | ||
| "$FE_CORE/target/generated-sources/java" | ||
| "$FE_CORE/target/generated-sources/annotations" | ||
| "$FE_CORE/target/generated-sources/antlr4" |
There was a problem hiding this comment.
could we get these paths from mvn pom.xml?
There was a problem hiding this comment.
Code Review Summary: tools/fast-compile-fe.sh
This PR adds a developer utility script for incremental FE compilation. The script is well-structured and useful. A few issues and suggestions below.
Critical Checkpoint Conclusions
- Goal/correctness: The script achieves its stated goal of incremental compilation for
fe-core. However, it only coversfe/fe-corewhile the FE has many sub-modules (fe-common,fe-catalog,fe-foundation,fe-type, etc.). Changes in those modules won't be picked up. This scope limitation should be clearly documented. - Modification minimal/focused: Yes, single new file, well-scoped.
- Concurrency: N/A (shell script, single-threaded).
- Lifecycle/static init: N/A.
- Configuration items: N/A.
- Incompatible changes: N/A — dev tool only, no production impact.
- Parallel code paths: N/A.
- Special conditional checks: The stale-file detection only checks mtime of the main
.classfile, not inner classes. If only an inner class is stale (e.g., due to a dependency change), it won't be detected. Acceptable trade-off for a dev tool. - Test coverage: No automated tests, which is fine for a developer utility script.
- Observability: Colored info/warn/error output is adequate.
- Performance: The
find_stale_java_filesfunction spawns one shell loop over all.javafiles inSRC_ROOT. For large source trees this is slow; afind ... -newerapproach orrsync --dry-runwould be faster, but acceptable for a dev tool.
Issues Found
-
(Medium)
javac -source 8 -target 8differs from the Maven build's<release>8</release>. The--releaseflag restricts API usage to the target JDK's boot classpath;-source/-targetdoes not. This means the fast-compile path could successfully compile code that uses JDK 17+ APIs, which would then fail in the real Maven build. Consider using--release 8instead. -
(Medium)
collect_updated_classesuses${java_file#$SRC_ROOT/}to compute class file paths. If a user passes a file that is not underSRC_ROOT(e.g., a generated-sources file, or a file resolved from a different module), the prefix stripping is a no-op and the resulting class path will be completely wrong, silently producing no matches. -
(Low) The
trap "rm -f $tmpfile" EXITon line 200 replaces any prior EXIT trap. Currently there is no prior trap so this is benign, but it's fragile for future edits. Consider using a single top-level cleanup function. -
(Low)
xargs jar ufon lines 211/217 splits on whitespace by default. If any class file path contains spaces, this will break. Usexargs -d '\n'(GNU xargs) for safety. -
(Info) The script only supports
fe/fe-core. The usage header and help text should mention this explicitly, since developers editingfe-commonorfe-catalogmight expect this tool to handle those modules too.
|
|
||
| # Compile with javac | ||
| javac \ | ||
| -source 8 -target 8 \ |
There was a problem hiding this comment.
javac flags mismatch with Maven build: The Maven compiler plugin uses <release>8</release> (equivalent to --release 8), which restricts available APIs to the JDK 8 boot classpath. Using -source 8 -target 8 here only sets syntax/bytecode levels but allows using newer JDK APIs, meaning this fast-compile could succeed on code that would fail in the real Maven build.
Suggestion: replace with --release 8 (requires JDK 9+, which the project already mandates):
javac \
--release 8 \
-encoding UTF-8 \| local class_files=() | ||
|
|
||
| for java_file in "${java_files[@]}"; do | ||
| local rel_path="${java_file#$SRC_ROOT/}" |
There was a problem hiding this comment.
collect_updated_classes breaks for files outside SRC_ROOT: The prefix stripping ${java_file#$SRC_ROOT/} is a no-op if the file is not under SRC_ROOT (e.g., a generated-sources file or a file from another module). The resulting class_prefix will point to a nonsensical path and silently produce zero matches.
This can happen when a user passes an absolute path to a file outside fe-core/src/main/java/. Consider adding a validation or falling back to basename-based lookup.
| # Convert class file paths to relative paths for jar command | ||
| local tmpfile | ||
| tmpfile="$(mktemp)" | ||
| trap "rm -f $tmpfile" EXIT |
There was a problem hiding this comment.
Minor: trap replaces prior EXIT handler: Each trap ... EXIT replaces the previous one. This is fine today but fragile if anyone adds cleanup elsewhere. A common pattern is to define a single cleanup function at the top and append to it:
_cleanup_files=()
cleanup() { rm -f "${_cleanup_files[@]}"; }
trap cleanup EXIT
# ... later:
_cleanup_files+=("$tmpfile")|
|
||
| # Update target/doris-fe.jar | ||
| if [[ -f "$TARGET_JAR" ]]; then | ||
| xargs jar uf "$TARGET_JAR" < "$tmpfile" |
There was a problem hiding this comment.
xargs splits on whitespace by default: If any class-file relative path ever contains spaces (unlikely but possible with generated code), this will break. Consider using xargs -d '\n' for robustness:
xargs -d '\n' jar uf "$TARGET_JAR" < "$tmpfile"| set -euo pipefail | ||
|
|
||
| DORIS_HOME="$(cd "$(dirname "$0")/.." && pwd)" | ||
| FE_CORE="$DORIS_HOME/fe/fe-core" |
There was a problem hiding this comment.
Scope limitation not documented: This script only handles fe/fe-core, but the FE workspace contains many sub-modules (fe-common, fe-catalog, fe-foundation, fe-type, etc.). Developers editing those modules might expect this tool to handle them.
Consider adding a note to the usage comment, e.g.:
# Note: Only supports fe/fe-core source files. Changes in other FE
# sub-modules (fe-common, fe-catalog, etc.) require a full Maven build.
|
|
||
| # Compile with javac | ||
| javac \ | ||
| -source 8 -target 8 \ |
There was a problem hiding this comment.
source and target read from pom.xml is better?
Add
tools/fast-compile-fe.sh, a developer utility that incrementally compiles only changed Java source files and patches them directly intodoris-fe.jar, avoiding a full Maven rebuild during development.Key features:
.javafiles by comparing mtime with.classfilestarget/libto skip repeated mvn callstarget/doris-fe.jarandoutput/fe/lib/doris-fe.jarFoo$Bar.class)What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)