build: fix race condition for pdb generation#1642
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCMakeLists.txt was changed to move plugin DLL/PDB copy steps out of an inline PREPARE_AIO phase into a POST_BUILD custom command so files are copied only after the linker finishes and releases them. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CMakeLists.txt (1)
335-336: Comment is accurate, but POST_BUILD should be self-sufficient
Right now${AIO_DIR}/SKSE/Pluginsis created byPREPARE_AIO; consider also creating it in thePOST_BUILDblock so the copy step remains correct if target ordering/ALL-ness changes later.add_custom_command( TARGET ${PROJECT_NAME} POST_BUILD + COMMAND + ${CMAKE_COMMAND} -E make_directory + "${AIO_DIR}/SKSE/Plugins" COMMAND ${CMAKE_COMMAND} -E copy_if_different "$<TARGET_FILE:${PROJECT_NAME}>" "${AIO_DIR}/SKSE/Plugins/$<TARGET_FILE_NAME:${PROJECT_NAME}>"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CMakeLists.txt(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
CMakeLists.txt
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Agent
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix a race condition during the build process by moving DLL and PDB file copying from a custom command (that runs as part of prepare_aio.stamp) to a POST_BUILD command attached to the main project target. The goal is to ensure the linker has fully released these files before attempting to copy them.
Key Changes:
- Removed DLL and PDB copy operations from the
_prepare_aio_cmdslist (lines 346-366 removed) - Added a new POST_BUILD command to copy DLL and PDB files after the target is built (lines 438-453)
- Updated comments to clarify the new approach
alandtse
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
CMakeLists.txt (2)
438-454: Update PR title to follow conventional commits and add issue reference.Per the repository's coding guidelines, the PR title should follow the conventional commits format
type(scope): description(lowercase, no period). Consider revising the title tofix(build): avoid dll/pdb copy race conditionor similar, and add a reference likeFixes #<issue-number>in the PR description if this addresses a tracked issue.As per coding guidelines, recommended title:
- Current:
build: fix race condition for pdb generation- Suggested:
fix(build): avoid dll/pdb copy race condition
449-451: Guard$<TARGET_PDB_FILE:...>to avoid empty-path failure.On generators/configurations where no PDB is produced (MinGW, non-MSVC toolchains, certain Debug builds),
$<TARGET_PDB_FILE:${PROJECT_NAME}>evaluates to an empty string, andcmake -E copy_if_different "" <dst>may hard-fail the build. Wrap the PDB copy command in a generator-expression guard so it becomes a no-op when there is no PDB.Apply this diff to conditionally guard the PDB copy:
COMMAND ${CMAKE_COMMAND} -E copy_if_different "$<TARGET_FILE:${PROJECT_NAME}>" "${AIO_DIR}/SKSE/Plugins/$<TARGET_FILE_NAME:${PROJECT_NAME}>" COMMAND - ${CMAKE_COMMAND} -E copy_if_different - "$<TARGET_PDB_FILE:${PROJECT_NAME}>" - "${AIO_DIR}/SKSE/Plugins/$<TARGET_PDB_FILE_NAME:${PROJECT_NAME}>" + ${CMAKE_COMMAND} -E $<IF:$<BOOL:$<TARGET_PDB_FILE:${PROJECT_NAME}>>,copy_if_different,true> + "$<TARGET_PDB_FILE:${PROJECT_NAME}>" + "${AIO_DIR}/SKSE/Plugins/$<TARGET_PDB_FILE_NAME:${PROJECT_NAME}>"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CMakeLists.txt(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
CMakeLists.txt
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
|
✅ A pre-release build is available for this PR: |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.