-
Notifications
You must be signed in to change notification settings - Fork 802
Unify RISC-V toolchain environment variables and remove default path #22710
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: main
Are you sure you want to change the base?
Conversation
- Unify and reduce environment variable usage (RISCV_TOOLCHAIN_ROOT and RISCV_RV64_LINUX_TOOLCHAIN_ROOT) - Remove CMake default RISCV_TOOL_PATH and require users to specify RISCV_TOOLCHAIN_ROOT Signed-off-by: Han-Kuan Chen <[email protected]>
mshockwave
left a comment
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.
Are we able to build IREE for RISC-V using Bazel? If that's the case then will this patch affect it?
I’ve never used Bazel for cross-building, but this patch modifies only the CMake part. |
mshockwave
left a comment
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.
LGTM
hanhanW
left a comment
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.
I'm not an expert, so I asked AI for helping the review. The only concern on my side is about documentation. Can you take a look? Thanks a lot for the cleanups!
Analysis
Positive Changes:
- Reduces confusion - Previously there were multiple variables (RISCV_TOOL_PATH, RISCV_TOOLCHAIN_ROOT, RISCV_RV64_LINUX_TOOLCHAIN_ROOT,
RISCV_RV32_NEWLIB_TOOLCHAIN_ROOT) with overlapping purposes. Consolidating to one makes the build easier to understand. - Explicit is better than implicit - Removing the default path $HOME/riscv/toolchain/clang/linux/RISCV forces users to explicitly specify their
toolchain location, which prevents silent failures when the default doesn't exist. - Cleaner CMake invocation - The -DRISCV_TOOLCHAIN_ROOT=${RISCV_TOOLCHAIN_ROOT} is now passed early in build_riscv.sh:42, making the flow clearer.
Issues/Concerns:
- Breaking change not clearly documented - The PR removes the documentation note explaining that RISCV_TOOLCHAIN_ROOT must be set (from
riscv.md:40-44). This seems backwards - if the variable is now required, the documentation should make this even more prominent, not remove the
mention entirely. - The -L flag is now passed twice for QEMU - In run_riscv_test.sh, the PR removes the -L flag from the script. But in iree_cc_test.cmake:174 and
iree_native_test.cmake:142, it adds -L "${RISCV_TOOLCHAIN_ROOT}/sysroot" as a command-line argument passed to run_riscv_test.sh. This works, but the
semantics are subtle - the -L is now part of the "$@" passed to QEMU. This is a valid approach, but the comment in run_riscv_test.sh should be updated
to clarify that -L is expected to be passed in. - Missing RISCV_RV32_NEWLIB_TOOLCHAIN_ROOT handling - In build_riscv.sh, the old code set RISCV_TOOLCHAIN_ROOT="${RISCV_RV32_NEWLIB_TOOLCHAIN_ROOT}"
for the generic-riscv_32 case. The new code assumes RISCV_TOOLCHAIN_ROOT is already set correctly for this case. This should work since
RISCV_TOOLCHAIN_ROOT is now always passed, but users who previously relied on setting RISCV_RV32_NEWLIB_TOOLCHAIN_ROOT separately will need to update
their workflow. - riscv_bootstrap.sh removes helpful guidance - The script previously printed a message telling users to export RISCV_TOOLCHAIN_ROOT=... after
downloading. Now users get no guidance at all on what to do next.
Recommendations
- Keep the documentation note or enhance it. Don't remove the note from riscv.md - instead update it to say this is now mandatory.
- Consider adding validation in riscv.toolchain.cmake:
if(NOT DEFINED RISCV_TOOLCHAIN_ROOT)
message(FATAL_ERROR "RISCV_TOOLCHAIN_ROOT must be set. See docs/website/docs/building-from-source/riscv.md")
endif() - This would provide a clear error message instead of cryptic failures about missing compilers.
- Update riscv_bootstrap.sh to still print the helpful message about setting the environment variable, or print it to a README/instructions file.
- Add a comment in run_riscv_test.sh explaining that the -L sysroot argument is now expected to be passed in via command line arguments.
Verdict
The core idea is sound and the implementation is largely correct. The PR has already been approved by a maintainer. The concerns above are minor
polish issues rather than blockers.
No blocking issues, but the documentation regression (removing the note about RISCV_TOOLCHAIN_ROOT) should ideally be addressed before merge.
| !!! note | ||
| The `RISCV_TOOLCHAIN_ROOT` environment variable needs | ||
| to be set to the root directory of the installed GNU toolchain when building | ||
| the RISC-V compiler target and the runtime library. | ||
|
|
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.
Why is it removed, but we still have the env in workflows file?
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.
They serve different purposes.
For building IREE in a RISC-V environment, we set RISCV_TOOLCHAIN_ROOT in cmake.
In contrast, the RISCV_TOOLCHAIN_ROOT used in the workflows is simply to avoid repeating the same path.
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.
Does it mean that we need to update below part for RISCV_TOOLCHAIN_ROOT?
https://iree.dev/building-from-source/riscv/#configure-and-build
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.
oh, I read wrong section. It is already set in https://iree.dev/building-from-source/riscv/#target-configuration Is it correct?
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 old document has a lot of conflict information. That is why I remove the environment variable (RISCV_TOOLCHAIN_ROOT).
riscv.md is updated to use the same parameters as build_riscv.sh.
Signed-off-by: Han-Kuan Chen <[email protected]>
0e807b9 to
aa54e70
Compare
Uh oh!
There was an error while loading. Please reload this page.