-
Notifications
You must be signed in to change notification settings - Fork 0
Add interactive build script for KorGE Forge #3
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
base: 252.25557
Are you sure you want to change the base?
Conversation
(cherry picked from commit dd980256f072d0d8e723fba8cb30277c713940a8) IJ-MR-171824 GitOrigin-RevId: 74aa0aa0ad484fce4cfcc9a9b88774aeca205ae1
(cherry picked from commit 96873bdfdd5532fbb991b696a596ac4cf576dd63) IJ-MR-174206 GitOrigin-RevId: f29b6da4c13deffa18e070f7649356bdb635aa06
(cherry picked from commit dd980256f072d0d8e723fba8cb30277c713940a8) IJ-MR-171824 GitOrigin-RevId: 74aa0aa0ad484fce4cfcc9a9b88774aeca205ae1
(cherry picked from commit 96873bdfdd5532fbb991b696a596ac4cf576dd63) IJ-MR-174206 GitOrigin-RevId: f29b6da4c13deffa18e070f7649356bdb635aa06
… delegate is found (cherry picked from commit 33f3f20055b420d9bca4c58c09a784fd626bd6e7) IJ-CR-174133 GitOrigin-RevId: 43796ad8bd6e93ddadd784a0712859c97e7e86cc
…' must not be null' Merge-request: IJ-MR-173816 Merged-by: Egor Eliseev <Egor.Eliseev@jetbrains.com> (cherry picked from commit 726dd1849e7988d33d4937188542ba10fdb9617b) IJ-MR-173816 GitOrigin-RevId: da21302cabc66d4defd88ac060d3b4e5b3aa36c1
…' must not be null' Fix callers Merge-request: IJ-MR-174209 Merged-by: Egor Eliseev <Egor.Eliseev@jetbrains.com> (cherry picked from commit 751bea82479695737e6319d2145fca095c8af35c) IJ-MR-174209 GitOrigin-RevId: 8f8161f71e387ee6c8b8e24e813a8c186809323b
… await job (PY-83799) (cherry picked from commit c84b85c2af157a61206fcd18ddd0676fb648407c) IJ-MR-174328 GitOrigin-RevId: dc0d5a216177a4c222609a47dbdf3df27863466c
(cherry picked from commit 10ad96109428f795fc1522dd04544318ecab1249) IJ-MR-174364 GitOrigin-RevId: 1d2cf082f40ae2fd131018b94c62685c08aa37bc
…s been removed" error when fetching tasks (cherry picked from commit b1ef765175e99e32ac4277bd7316891ebebc2bf9) IJ-CR-174067 GitOrigin-RevId: fd5437597aec2d894cb87ea023e73c497c576886
|
@copilot remove breaking changes and follow the rest of the instructions |
Co-authored-by: FSaurenbach <87531743+FSaurenbach@users.noreply.github.com>
Created a new interactive build script
Also updated HOW-TO-BUILD.md with documentation. Commit: f4485ec |
|
augment review |
…ript Co-authored-by: FSaurenbach <87531743+FSaurenbach@users.noreply.github.com>
Merged the interactive features into Commit: b721b2f |
|
augment review |
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.
Review completed. 3 suggestions posted. Main themes: ensure the interactive options match actual build behavior, and make Java detection more robust.
Comment augment review to trigger a new review at any time.
build_forge_interactive.sh
Outdated
| # Incremental build option | ||
| select_incremental() { | ||
| echo -e "\n${BOLD}Incremental Build:${NC}" | ||
| echo " 1) Full build (clean)" |
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.
The menu labels this as a “Full build (clean)”, but the script doesn’t appear to perform any clean step (it only toggles an incremental compilation property), so users may still get a non-clean build. Consider aligning the wording/behavior so the choice matches what actually happens.
🤖 Was this useful? React with 👍 or 👎
build_forge_interactive.sh
Outdated
|
|
||
| if [ -n "$JAVA_HOME" ]; then | ||
| if [ -x "$JAVA_HOME/bin/java" ]; then | ||
| JAVA_VERSION=$("$JAVA_HOME/bin/java" -version 2>&1 | head -n 1 | cut -d'"' -f2) |
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.
detect_java reports success for any java found, but the build may require a full JDK (e.g., javac) and specifically JDK 21; consider validating both the major version and that a JDK is present. Also, inferring JAVA_HOME from which java can yield a non-JDK path on some systems (notably macOS), so it may be worth validating the derived JAVA_HOME_PATH before exporting it.
🤖 Was this useful? React with 👍 or 👎
build_forge_interactive.sh
Outdated
|
|
||
| # Run the build (using eval to properly handle empty BUILD_ARGS) | ||
| if [ -n "$BUILD_ARGS" ]; then | ||
| eval "./installers.cmd $BUILD_ARGS" |
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.
Using eval here is avoidable and can create quoting/injection hazards if BUILD_ARGS is ever extended to include user-provided values. A safer approach is to invoke ./installers.cmd without eval while preserving argument boundaries.
🤖 Was this useful? React with 👍 or 👎
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.
Review completed. 2 suggestions posted. Main theme: make the build experience more robust and predictable across environments.
Comment augment review to trigger a new review at any time.
|
|
||
| if [ -n "$JAVA_HOME" ]; then | ||
| if [ -x "$JAVA_HOME/bin/java" ]; then | ||
| JAVA_VERSION=$(get_java_version "$JAVA_HOME/bin/java") |
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.
detect_java reports “Please install JDK 21”, but it currently accepts any java it finds (and may derive a JAVA_HOME_PATH that points to a JRE). Consider validating the detected major version (and that it’s a full JDK) early so failures don’t happen later inside installers.cmd.
🤖 Was this useful? React with 👍 or 👎
| - Automatic Java detection | ||
| - Platform selection (current platform or all platforms) | ||
| - Incremental build option | ||
| - Colored output for better visibility |
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.
This file still has no trailing newline (shown as “No newline at end of file” in the diff), which can cause minor tooling issues.
🤖 Was this useful? React with 👍 or 👎
Upgrades the existing
build_forge.shscript with a user-friendly interactive command-line interface for building the project.Features
JAVA_HOMEor system PATHChanges Made
build_forge.sh- replaced the old simple script with interactive featuresHOW-TO-BUILD.md- updated documentation to reflect the new interactive featuresUsage
The script will guide you through the build configuration with interactive prompts.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.