[build] 优化构建过程的打印输出和spawn输出目录#38
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request optimizes the build process by improving output printing and organizing spawn temporary files into the build directory. The changes affect two SCons build scripts for different Edgi_Talk projects.
Changes:
- Modified spawn function to place temporary files in the
builddirectory instead of the project root - Refactored library path detection to use VariantDir for better build organization
- Added simplified build output messages (AR, AS, CC, CXX, LINK) for cleaner console output
- Updated source file categorization logic to properly handle library and build directory paths
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| projects/Edgi_Talk_M55_XiaoZhi/SConstruct | Updated spawn function to write temporary files to ./build/ directory |
| projects/Edgi_Talk_M33_S_Template/Sconstruct | Refactored library path handling with VariantDir, added build output customization, and improved source path categorization logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| libraries_variant_abs = os.path.normpath(os.path.join(PROJECT_ROOT, libraries_variant)) | ||
| libraries_real_path = libraries_src_abs | ||
| libraries_variant_rel = libraries_variant | ||
| VariantDir(libraries_variant, libraries_src, duplicate=0) | ||
| libraries_real_path = _posix(libraries_real_path) | ||
| libraries_variant = _posix(libraries_variant_rel) | ||
| LIBRARIES_PREFIX = libraries_variant + '/' | ||
|
|
||
| # Check if libs exist locally or need to use external path | ||
| if os.path.exists(os.path.join(PROJECT_ROOT, 'libs')): | ||
| LIBS_PATH = PROJECT_ROOT + '/libs' | ||
| LIBS_PREFIX = '' | ||
| local_libs_dir = os.path.join(PROJECT_ROOT, 'libs') | ||
| if os.path.exists(local_libs_dir): | ||
| libs_src = 'libs' | ||
| libs_variant = os.path.join(BUILD_DIR, 'libs_local') | ||
| else: | ||
| LIBS_PATH = os.path.normpath(os.path.join(PROJECT_ROOT, '..', 'libs')) | ||
| LIBS_PREFIX = '../libs/' | ||
| libs_src = os.path.relpath(os.path.join(PROJECT_ROOT, '..', 'libs'), PROJECT_ROOT) | ||
| libs_variant = os.path.join(BUILD_DIR, 'libs_ext') | ||
| libs_src_abs = os.path.normpath(os.path.join(PROJECT_ROOT, libs_src)) | ||
| libs_variant_abs = os.path.normpath(os.path.join(PROJECT_ROOT, libs_variant)) |
There was a problem hiding this comment.
The variables libraries_variant_abs and libs_variant_abs are computed but never used. Consider removing these unused variable assignments or using them if they were intended for some purpose.
| libraries_variant_rel = libraries_variant | ||
| VariantDir(libraries_variant, libraries_src, duplicate=0) | ||
| libraries_real_path = _posix(libraries_real_path) | ||
| libraries_variant = _posix(libraries_variant_rel) | ||
| LIBRARIES_PREFIX = libraries_variant + '/' | ||
|
|
||
| # Check if libs exist locally or need to use external path | ||
| if os.path.exists(os.path.join(PROJECT_ROOT, 'libs')): | ||
| LIBS_PATH = PROJECT_ROOT + '/libs' | ||
| LIBS_PREFIX = '' | ||
| local_libs_dir = os.path.join(PROJECT_ROOT, 'libs') | ||
| if os.path.exists(local_libs_dir): | ||
| libs_src = 'libs' | ||
| libs_variant = os.path.join(BUILD_DIR, 'libs_local') | ||
| else: | ||
| LIBS_PATH = os.path.normpath(os.path.join(PROJECT_ROOT, '..', 'libs')) | ||
| LIBS_PREFIX = '../libs/' | ||
| libs_src = os.path.relpath(os.path.join(PROJECT_ROOT, '..', 'libs'), PROJECT_ROOT) | ||
| libs_variant = os.path.join(BUILD_DIR, 'libs_ext') | ||
| libs_src_abs = os.path.normpath(os.path.join(PROJECT_ROOT, libs_src)) | ||
| libs_variant_abs = os.path.normpath(os.path.join(PROJECT_ROOT, libs_variant)) | ||
| libs_real_path = libs_src_abs | ||
| libs_variant_rel = libs_variant | ||
| VariantDir(libs_variant, libs_src, duplicate=0) | ||
| libs_real_path = _posix(libs_real_path) | ||
| libs_variant = _posix(libs_variant_rel) |
There was a problem hiding this comment.
The variable libraries_variant_rel is assigned on line 163 but immediately overwritten on line 166 after being passed through the _posix function. The same issue occurs with libs_variant_rel on lines 180 and 183. Consider removing the intermediate variable assignment on lines 163 and 180 and directly assign the result on lines 166 and 183.
| if src_abs_norm.startswith(libs_src_norm): | ||
| rel_in_libs = os.path.relpath(src_abs, libs_src_abs).replace('\\', '/') | ||
| local_src.append(LIBS_PREFIX + rel_in_libs) | ||
| continue | ||
|
|
||
| if src_abs_norm.startswith(libraries_src_norm): | ||
| rel_in_libraries = os.path.relpath(src_abs, libraries_src_abs).replace('\\', '/') | ||
| local_src.append(LIBRARIES_PREFIX + rel_in_libraries) | ||
| continue |
There was a problem hiding this comment.
The path separator check on line 396 and 401 using startswith may be insufficient. On Windows, paths use backslashes by default, but the separator used in libs_src_norm and libraries_src_norm might not end with a separator. This could lead to false positives where a path like 'C:\libs_extended' would match 'C:\libs'. Consider adding os.sep at the end of the comparison paths or using a more robust check like os.path.commonpath.
| # set spawn | ||
| def ourspawn(sh, escape, cmd, args, e): | ||
| filename = str(uuid.uuid4()) | ||
| filename = os.path.join('./', 'build', str(uuid.uuid4())) |
There was a problem hiding this comment.
The temporary file is now placed in the 'build' directory, but the code doesn't ensure this directory exists before attempting to create the file. This could cause an error if the 'build' directory hasn't been created yet. Consider adding directory creation logic such as: os.makedirs(os.path.dirname(filename), exist_ok=True) before line 80 where the file is opened.
No description provided.