[smart] rename the Group name to 'lwProcess' and optimize the error h…#10436
[smart] rename the Group name to 'lwProcess' and optimize the error h…#10436Rbb666 merged 2 commits intoRT-Thread:masterfrom
Conversation
…andling for vDSO building.
There was a problem hiding this comment.
Pull Request Overview
This PR standardizes the SConscript group name to lwProcess, refactors error handling for the vDSO build step, and removes two legacy architecture-specific scripts.
- Refactor vDSO build: consolidate clean logic into one command, wrap
subprocess.runin atry/except, and report failures. - Rename all
DefineGroupcalls fromvDSO/lwPtolwProcess. - Delete
components/lwp/arch/x86/i386/SConscriptandcomponents/lwp/arch/risc-v/rv64/SConscript.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| components/lwp/vdso/SConscript | Refactored clean command logic, added try/except, renamed group. |
| components/lwp/arch/x86/i386/SConscript | Removed legacy i386 SConscript file. |
| components/lwp/arch/risc-v/rv64/SConscript | Removed legacy rv64 SConscript file. |
| components/lwp/SConscript | Renamed top-level group from lwP to lwProcess. |
Comments suppressed due to low confidence (3)
components/lwp/vdso/SConscript:54
- [nitpick] Renaming the vDSO group to
lwProcessmay conflict with the main LWP group and obscure the vDSO target. Consider using a distinct name likelwProcess-vdsofor clarity.
group = DefineGroup('lwProcess', src, depend = ['RT_USING_SMART','RT_USING_VDSO'], CPPPATH = CPPPATH)
| # generic error handle | ||
| except : | ||
| print('exec command: "%s" failed.' % ' '.join(command)) | ||
| exit(1) |
There was a problem hiding this comment.
Avoid using a bare except: which also catches system-exiting exceptions. Catch specific errors (e.g., except subprocess.CalledProcessError as e:) to prevent unintended behavior.
| # generic error handle | |
| except : | |
| print('exec command: "%s" failed.' % ' '.join(command)) | |
| exit(1) | |
| except subprocess.CalledProcessError as e: | |
| print(f'Command "{e.cmd}" failed with return code {e.returncode}.') | |
| exit(1) | |
| except FileNotFoundError as e: | |
| print(f'Command not found: {e}. Please ensure "scons" is installed and in your PATH.') | |
| exit(1) |
components/lwp/vdso/SConscript
Outdated
| if result.returncode == 0: | ||
| print("Command executed successfully") | ||
| else: | ||
| print("Command failed with exit code:", result.returncode) | ||
| exit(1) |
There was a problem hiding this comment.
When using check=True, subprocess.run will raise on non-zero exits, so this manual returncode check is redundant. Consider removing it or use check=False if you need to handle exit codes explicitly.
| if result.returncode == 0: | |
| print("Command executed successfully") | |
| else: | |
| print("Command failed with exit code:", result.returncode) | |
| exit(1) | |
| print("Command executed successfully") |
unicornx
left a comment
There was a problem hiding this comment.
建议 rebase 到最新的 master 后再 review,因为最近 master 上又合入了多个 和 vdso 相关的 pr。
另外请考虑改进一下 copilot 的意见。
这个PR只涉及到SConscript的修改,主要是把Group的名称做了修改,从lwP更改成lwProcess,更能反映意义。目前主干代码中的Group Name挺乱了,在逐步进行调整。 vdso部分的修改是参考了copilot的意见。 |
|
@Rbb666 来帮忙合并下 |
…andling for vDSO building.
拉取/合并请求描述:(PR description)
[
为什么提交这份PR (why to submit this PR)
smart的Group Name稍显凌乱,统一调整成lwProcess,以反映“轻量级进程”
你的解决方案是什么 (what is your solution)
对SConscript脚本进行调整。
请提供验证的bsp和config (provide the config and bsp)
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality:
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up