fixup! feat: add pre-commit hooks and fix build issues #42
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/build-amd64.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build-amd64.yml
137-137: string should not be empty
(syntax-check)
138-138: key "run" is duplicated in element of "steps" section. previously defined at line:137,col:7
(syntax-check)
🪛 yamllint (1.35.1)
.github/workflows/build-amd64.yml
[error] 138-138: duplication of key "run" in mapping
(key-duplicates)
🔇 Additional comments (1)
.github/workflows/build-amd64.yml (1)
142-144: Verify the necessity of node upgrade for newer clang versions.
The step upgrades node only for clang versions >= 18 on macOS, but:
- The relationship between node version and clang version isn't clear
- There's no comment explaining why this is needed
Could you:
- Add a comment explaining why node needs to be upgraded for newer clang versions
- Consider making this more maintainable by:
- name: update node for newer clang versions if: ${{ matrix.os == 'macosx' && matrix.clang-version >= '18' }} run: | # Node upgrade needed for clang >= 18 because... brew upgrade node
.github/workflows/build-amd64.yml
Outdated
| run: | | ||
| run: | | ||
| sed -i.backup 's/gcc_eh.\*|/gcc_eh.*|gcc_ext.*|/g' "$(find /opt/homebrew/Cellar -name CMakeParseImplicitLinkInfo.cmake)" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of CMake configuration modification.
The current implementation has a few concerns:
- The
findcommand might return multiple matches, making thesedoperation unpredictable. - Installing gcc@11 is mixed with CMake configuration modification, violating single responsibility principle.
Consider splitting into two steps and making the CMake modification more robust:
- name: patch cmake implicit link libraries on macosx
if: ${{ matrix.os == 'macosx' }}
run: |
CMAKE_FILE=$(find /opt/homebrew/Cellar -name CMakeParseImplicitLinkInfo.cmake)
if [ $(echo "$CMAKE_FILE" | wc -l) -ne 1 ]; then
echo "Error: Found multiple or no CMake files"
echo "$CMAKE_FILE"
exit 1
fi
sed -i.backup 's/gcc_eh.\*|/gcc_eh.*|gcc_ext.*|/g' "$CMAKE_FILE"
- name: install gcc@11
if: ${{ matrix.os == 'macosx' }}
run: brew install gcc@11🧰 Tools
🪛 actionlint (1.7.4)
137-137: string should not be empty
(syntax-check)
138-138: key "run" is duplicated in element of "steps" section. previously defined at line:137,col:7
(syntax-check)
🪛 yamllint (1.35.1)
[error] 138-138: duplication of key "run" in mapping
(key-duplicates)
.github/workflows/build-amd64.yml
Outdated
| run: | | ||
| run: | |
There was a problem hiding this comment.
Fix duplicate run key in YAML.
There's a syntax error due to duplicate run keys that would cause the workflow to fail.
Apply this fix:
run: |
- run: |
sed -i.backup 's/gcc_eh.\*|/gcc_eh.*|gcc_ext.*|/g' "$(find /opt/homebrew/Cellar -name CMakeParseImplicitLinkInfo.cmake)"
brew install gcc@11📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| run: | | |
| run: | | |
| sed -i.backup 's/gcc_eh.\*|/gcc_eh.*|gcc_ext.*|/g' "$(find /opt/homebrew/Cellar -name CMakeParseImplicitLinkInfo.cmake)" | |
| brew install gcc@11 |
🧰 Tools
🪛 actionlint (1.7.4)
137-137: string should not be empty
(syntax-check)
138-138: key "run" is duplicated in element of "steps" section. previously defined at line:137,col:7
(syntax-check)
🪛 yamllint (1.35.1)
[error] 138-138: duplication of key "run" in mapping
(key-duplicates)
Summary by CodeRabbit