Fix: Update VAD package installation to support multiple paths - #34
Fix: Update VAD package installation to support multiple paths#34anshuman9468 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdded an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@quickstart/quickstart.sh`:
- Around line 32-43: run_virtuoso_cmd's non-zero exit is currently ignored so
failures with "[ERROR]" get treated as success; capture its exit code (e.g.,
rc=$?) immediately after calling run_virtuoso_cmd and treat any rc != 0 as a
failure: log a warning/error including "$OUTPUT" and the rc, avoid falling into
the success branch, and return a non-zero status instead of returning 0. Update
the logic around OUTPUT/run_virtuoso_cmd (and the success echo that references
VAD_NAME) to include this rc check so connection/command failures are handled
properly.
- Around line 105-106: The call to install_vad in quickstart.sh currently
ignores its return value so the script continues on failure; update the script
to check the exit status of install_vad "dbpedia_dav.vad" (e.g., if !
install_vad "dbpedia_dav.vad"; then echo an error to stderr and exit 1) so that
a failed VAD installation aborts before proceeding to the data-loading steps.
| local OUTPUT | ||
| OUTPUT=$(run_virtuoso_cmd "vad_install('${path}', 0);") | ||
| echo "$OUTPUT" | ||
|
|
||
| if [[ "$OUTPUT" == *"Errors detected"* ]] || [[ "$OUTPUT" == *"result set is empty"* ]]; then | ||
| echo "[WARN] Installation from '${path}' failed." | ||
| else | ||
| # If no errors detected, assume success. | ||
| # We could double check registry but that might be overkill if error detection is reliable. | ||
| echo "[INFO] Successfully installed '${VAD_NAME}'" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
Bug: run_virtuoso_cmd failure (non-zero exit) is treated as success.
When run_virtuoso_cmd returns non-zero (e.g., connection failure), its output contains [ERROR] but neither "Errors detected" nor "result set is empty". The condition on line 36 won't match, so execution falls through to the else branch and incorrectly reports success.
You should also check the return code of run_virtuoso_cmd:
🐛 Proposed fix
local OUTPUT
- OUTPUT=$(run_virtuoso_cmd "vad_install('${path}', 0);")
+ OUTPUT=$(run_virtuoso_cmd "vad_install('${path}', 0);")
+ local RC=$?
echo "$OUTPUT"
- if [[ "$OUTPUT" == *"Errors detected"* ]] || [[ "$OUTPUT" == *"result set is empty"* ]]; then
+ if [[ $RC -ne 0 ]] || [[ "$OUTPUT" == *"Errors detected"* ]] || [[ "$OUTPUT" == *"result set is empty"* ]]; then
echo "[WARN] Installation from '${path}' failed."
else🤖 Prompt for AI Agents
In `@quickstart/quickstart.sh` around lines 32 - 43, run_virtuoso_cmd's non-zero
exit is currently ignored so failures with "[ERROR]" get treated as success;
capture its exit code (e.g., rc=$?) immediately after calling run_virtuoso_cmd
and treat any rc != 0 as a failure: log a warning/error including "$OUTPUT" and
the rc, avoid falling into the success branch, and return a non-zero status
instead of returning 0. Update the logic around OUTPUT/run_virtuoso_cmd (and the
success echo that references VAD_NAME) to include this rc check so
connection/command failures are handled properly.
| echo "[INFO] Installing VAD package 'dbpedia_dav.vad'" | ||
| run_virtuoso_cmd "vad_install('/opt/virtuoso-opensource/vad/dbpedia_dav.vad', 0);" | ||
| install_vad "dbpedia_dav.vad" |
There was a problem hiding this comment.
Return value of install_vad is ignored — script continues silently on failure.
If the VAD package cannot be installed from any known path, install_vad returns 1 but the script proceeds with data loading regardless. This could lead to a broken setup without any clear indication to the operator.
🛡️ Proposed fix
echo "[INFO] Installing VAD package 'dbpedia_dav.vad'"
-install_vad "dbpedia_dav.vad"
+install_vad "dbpedia_dav.vad"
+if [ $? -ne 0 ]; then
+ echo "[ERROR] VAD package installation failed; aborting."
+ exit 1
+fi📝 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.
| echo "[INFO] Installing VAD package 'dbpedia_dav.vad'" | |
| run_virtuoso_cmd "vad_install('/opt/virtuoso-opensource/vad/dbpedia_dav.vad', 0);" | |
| install_vad "dbpedia_dav.vad" | |
| echo "[INFO] Installing VAD package 'dbpedia_dav.vad'" | |
| install_vad "dbpedia_dav.vad" | |
| if [ $? -ne 0 ]; then | |
| echo "[ERROR] VAD package installation failed; aborting." | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In `@quickstart/quickstart.sh` around lines 105 - 106, The call to install_vad in
quickstart.sh currently ignores its return value so the script continues on
failure; update the script to check the exit status of install_vad
"dbpedia_dav.vad" (e.g., if ! install_vad "dbpedia_dav.vad"; then echo an error
to stderr and exit 1) so that a failed VAD installation aborts before proceeding
to the data-loading steps.
2 participants
Summary by CodeRabbit