-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix PG version check for structs in separate header files #11278
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
Conversation
The script now checks the corresponding .h file when PG_REGISTER is in a .c file but the struct typedef is in the header. This fixes false negatives where struct changes in headers weren't detected. Example: blackboxConfig_t is registered in blackbox.c but the struct typedef blackboxConfig_s is defined in blackbox.h. Previous version only checked blackbox.c diff and missed the struct field removal.
PR Compliance Guide 🔍All compliance sections have been disabled in the configurations. |
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.
High-level Suggestion
The current method of parsing C code with sed and grep is fragile. A better long-term solution is to use a compiler-based tool to analyze the code's Abstract Syntax Tree (AST), which would provide a more reliable way to detect struct changes. [High-level, importance: 9]
Solution Walkthrough:
Before:
# In check-pg-versions.sh
struct_pattern="typedef struct ${struct_type%_t}_s"
# Get diff for current file
diff_output=$(git diff ...)
# Try to find struct definition using sed
struct_body_diff=$(echo "$diff_output" | sed -n "/${struct_pattern}/, /.../p")
# If not found, try companion file
if [ -z "$struct_body_diff" ]; then
companion_diff=$(git diff ... -- "$companion_file")
struct_body_diff=$(echo "$companion_diff" | sed -n "/${struct_pattern}/, /.../p")
fi
# Find changes in the text-based diff
struct_changes=$(echo "$struct_body_diff" | grep -E "^[-+]" | grep -v ...)
After:
# Conceptual script using a C parser (e.g., libclang)
import clang.cindex
def get_struct_signature(file_content, struct_name):
# Parse code into an Abstract Syntax Tree (AST)
idx = clang.cindex.Index.create()
tu = idx.parse('tmp.c', unsaved_files=[('tmp.c', file_content)])
# Find struct in AST and generate a signature from its members
# (This is far more robust than text matching)
...
return signature
# Get file content from base and head commits
base_content = git.show(f"{BASE_COMMIT}:{file}")
head_content = git.show(f"{HEAD_COMMIT}:{file}")
# Compare signatures
base_sig = get_struct_signature(base_content, "my_struct_t")
head_sig = get_struct_signature(head_content, "my_struct_t")
if base_sig != head_sig:
print("Struct has changed!")
| if [ -n "$companion_file" ] && git diff --name-only $BASE_COMMIT..$HEAD_COMMIT | grep -q "^${companion_file}$"; then | ||
| echo " 🔍 Struct not in $file, checking $companion_file" | ||
| local companion_diff=$(git diff $BASE_COMMIT..$HEAD_COMMIT -- "$companion_file") | ||
| struct_body_diff=$(echo "$companion_diff" | sed -n "/${struct_pattern}/,/\}.*${struct_type};/p") | ||
| fi | ||
| fi |
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.
Suggestion: To improve performance, avoid repeated git diff calls inside the loop by getting all diffs once at the start of the script and storing them in an associative array for later lookup. [general, importance: 6]
| if [ -n "$companion_file" ] && git diff --name-only $BASE_COMMIT..$HEAD_COMMIT | grep -q "^${companion_file}$"; then | |
| echo " 🔍 Struct not in $file, checking $companion_file" | |
| local companion_diff=$(git diff $BASE_COMMIT..$HEAD_COMMIT -- "$companion_file") | |
| struct_body_diff=$(echo "$companion_diff" | sed -n "/${struct_pattern}/,/\}.*${struct_type};/p") | |
| fi | |
| fi | |
| # This suggestion requires changes outside the visible diff. | |
| # At the start of the script, you would add: | |
| # declare -A all_diffs | |
| # while IFS= read -r changed_file; do | |
| # all_diffs["$changed_file"]=$(git diff $BASE_COMMIT..$HEAD_COMMIT -- "$changed_file") | |
| # done < <(git diff --name-only $BASE_COMMIT..$HEAD_COMMIT) | |
| # Then, the existing code block would be changed to: | |
| if [ -n "$companion_file" ] && [ -v "all_diffs[$companion_file]" ]; then | |
| echo " 🔍 Struct not in $file, checking $companion_file" | |
| local companion_diff=${all_diffs[$companion_file]} | |
| struct_body_diff=$(echo "$companion_diff" | sed -n "/${struct_pattern}/,/\}.*${struct_type};/p") | |
| fi |
User description
Summary
Fixes a bug in the PG version check workflow where struct modifications in header files weren't detected when PG_REGISTER is in the corresponding .c file.
Problem
PR #11276 removed a field from
blackboxConfig_sstruct without incrementing the PG version, but the workflow didn't catch it because:PG_REGISTERforblackboxConfig_tis insrc/main/blackbox/blackbox.ctypedef blackboxConfig_sis defined insrc/main/blackbox/blackbox.hSolution
When checking a file with PG_REGISTER:
Testing
The fix will correctly detect the issue in PR #11276 once it's re-run.
Related
PR Type
Bug fix
Description
This description is generated by an AI tool. It may have inaccuracies
Fixes PG version check to detect struct changes in companion header files
Script now checks corresponding .h file when struct typedef is separate from PG_REGISTER location
Resolves false negatives where struct field modifications in headers were missed
Enables proper version validation for structs split across .c and .h files
Diagram Walkthrough
flowchart LR A["PG_REGISTER in .c file"] --> B["Check struct in same .c file"] B --> C{"Struct found?"} C -->|Yes| D["Validate struct changes"] C -->|No| E["Check companion .h file"] E --> F["Validate struct changes"] D --> G["Report version issues"] F --> GFile Walkthrough
check-pg-versions.sh
Add companion file struct checking logic.github/scripts/check-pg-versions.sh
found in the primary file
corresponding header file
parse it
matching as primary file