Repository navigation
Fix package name format for ort url - #3489
Conversation
dosubot
Bot
added
the
size:XS
📝 WalkthroughWalkthroughThis PR updates the ONNX Runtime package filename in the CMake configuration for RISC-V64 SpacemiT builds by URL-encoding the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the onnxruntime_pkg_name in the RISC-V CMake configuration to use URL encoding for the '+' character. A review comment points out that this change will likely break local file detection, as the filesystem expects a literal '+' rather than '%2B', and suggests encoding the character only for the URL.
| endif() | ||
|
|
||
| set(onnxruntime_pkg_name "spacemit-ort.riscv64.2.0.2+rc1.tar.gz") | ||
| set(onnxruntime_pkg_name "spacemit-ort.riscv64.2.0.2%2Brc1.tar.gz") |
There was a problem hiding this comment.
Changing the + character to %2B in onnxruntime_pkg_name will fix the download URL, but it will break the local file detection logic in lines 23-29. The if(EXISTS ${f}) check (line 32) will look for a file literally named with %2B on the disk, whereas the file is likely stored with a literal + character.
To support both the URL and local file detection, consider keeping the literal + in onnxruntime_pkg_name and only using the encoded version for the URL. For example, you could hardcode the encoded filename in the onnxruntime_URL definition on line 17, or use string(REPLACE "+" "%2B" ...) to create a separate variable for the URL.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmake/onnxruntime-linux-riscv64-spacemit.cmake (1)
16-29:⚠️ Potential issue | 🟠 MajorKeep the local archive filename unescaped; escape only the URL segment.
Using
%2Binonnxruntime_pkg_namealso changespossible_file_locations, so offline lookup now expects a literal%2Bfilename. Manually downloaded archives are commonly saved as...+rc1.tar.gz, which will be missed, causing unnecessary network fetch/failure in offline builds.💡 Proposed fix
-set(onnxruntime_pkg_name "spacemit-ort.riscv64.2.0.2%2Brc1.tar.gz") -set(onnxruntime_URL "https://archive.spacemit.com/spacemit-ai/onnxruntime/${onnxruntime_pkg_name}") +set(onnxruntime_pkg_file "spacemit-ort.riscv64.2.0.2+rc1.tar.gz") +string(REPLACE "+" "%2B" onnxruntime_pkg_url_path "${onnxruntime_pkg_file}") +set(onnxruntime_URL "https://archive.spacemit.com/spacemit-ai/onnxruntime/${onnxruntime_pkg_url_path}") @@ - $ENV{HOME}/Downloads/${onnxruntime_pkg_name} - ${CMAKE_SOURCE_DIR}/${onnxruntime_pkg_name} - ${CMAKE_BINARY_DIR}/${onnxruntime_pkg_name} - /tmp/${onnxruntime_pkg_name} - /star-fj/fangjun/download/github/${onnxruntime_pkg_name} + $ENV{HOME}/Downloads/${onnxruntime_pkg_file} + ${CMAKE_SOURCE_DIR}/${onnxruntime_pkg_file} + ${CMAKE_BINARY_DIR}/${onnxruntime_pkg_file} + /tmp/${onnxruntime_pkg_file} + /star-fj/fangjun/download/github/${onnxruntime_pkg_file}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/onnxruntime-linux-riscv64-spacemit.cmake` around lines 16 - 29, The package filename variable onnxruntime_pkg_name should contain the unescaped '+' (e.g., spacemit-ort.riscv64.2.0.2+rc1.tar.gz) so possible_file_locations matches locally downloaded archives; update onnxruntime_pkg_name to use the literal '+' and change onnxruntime_URL to use an encoded form for the URL segment (e.g., derive an encoded_pkg_name for the URL or URL-encode only when constructing onnxruntime_URL) so network fetch still uses %2B while offline file lookup uses the actual '+' filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmake/onnxruntime-linux-riscv64-spacemit.cmake`:
- Around line 16-29: The package filename variable onnxruntime_pkg_name should
contain the unescaped '+' (e.g., spacemit-ort.riscv64.2.0.2+rc1.tar.gz) so
possible_file_locations matches locally downloaded archives; update
onnxruntime_pkg_name to use the literal '+' and change onnxruntime_URL to use an
encoded form for the URL segment (e.g., derive an encoded_pkg_name for the URL
or URL-encode only when constructing onnxruntime_URL) so network fetch still
uses %2B while offline file lookup uses the actual '+' filename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3370824c-5835-4611-8088-4e74e7785449
📒 Files selected for processing (1)
cmake/onnxruntime-linux-riscv64-spacemit.cmake
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Spacemit RISC-V64 ONNX Runtime package filename to use URL-encoded formatting so the download URL resolves correctly.
Changes:
- URL-encode the
+character in the ONNX Runtime tarball name (%2B). - Keep the computed download URL consistent with the new package name format.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| endif() | ||
|
|
||
| set(onnxruntime_pkg_name "spacemit-ort.riscv64.2.0.2+rc1.tar.gz") | ||
| set(onnxruntime_pkg_name "spacemit-ort.riscv64.2.0.2%2Brc1.tar.gz") |
There was a problem hiding this comment.
Encoding + as %2B changes the downloaded artifact bytes if the server previously served the unencoded + filename; the onnxruntime_HASH may no longer match. Please verify the URL resolves to the intended tarball and update onnxruntime_HASH to the SHA256 of the artifact actually fetched from this URL.
| set(onnxruntime_pkg_name "spacemit-ort.riscv64.2.0.2%2Brc1.tar.gz") | |
| set(onnxruntime_pkg_name "spacemit-ort.riscv64.2.0.2+rc1.tar.gz") |
csukuangfj
left a comment
csukuangfj
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
Summary by CodeRabbit