cmake: improve: configure proxyres_config.h for preprocessor conditions#129
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a build-time configuration system that detects system header availability via CMake, generating Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #129 +/- ##
==========================================
+ Coverage 58.57% 63.83% +5.26%
==========================================
Files 32 34 +2
Lines 2607 2912 +305
Branches 526 546 +20
==========================================
+ Hits 1527 1859 +332
+ Misses 745 707 -38
- Partials 335 346 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a CMake-generated configuration header (proxyres_config.h) and updates several source files to use CMake-detected header-availability macros (e.g., HAVE_NETDB_H) to control conditional includes, aiming to make future portability patches simpler.
Changes:
- Add a new
proxyres_config.h.intemplate and generateproxyres_config.hvia CMake. - Replace unconditional
<netdb.h>includes with#if HAVE_NETDB_Hguards across multiple source files. - Update CMake target include paths and header lists to include the generated config header from the build directory.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| wpad_dns.c | Includes generated config header and gates <netdb.h> behind HAVE_NETDB_H. |
| wpad_dhcp_posix.c | Includes generated config header and gates <netdb.h> behind HAVE_NETDB_H. |
| proxyres_config.h.in | New CMake config header template defining HAVE_NET_IF_ARP_H / HAVE_NETDB_H. |
| net_util.c | Includes generated config header and gates <netdb.h> behind HAVE_NETDB_H. |
| net_adapter_mac.c | Includes generated config header (used for HAVE_NET_IF_ARP_H). |
| net_adapter_linux.c | Includes generated config header and gates <netdb.h> behind HAVE_NETDB_H. |
| fetch_posix.c | Includes generated config header and gates <netdb.h> behind HAVE_NETDB_H. |
| CMakeLists.txt | Adds header checks and configure_file() to generate proxyres_config.h; adds build dir include path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fetch_posix.c`:
- Around line 11-13: Replace the fragile preprocessor guard that uses `#if
HAVE_NETDB_H` with a defined() check to avoid -Wundef/MSVC C4668 warnings;
specifically change the conditional before the `#include <netdb.h>` lines to use
`#if defined(HAVE_NETDB_H) && HAVE_NETDB_H` (or at minimum `#if
defined(HAVE_NETDB_H)`) in every place the `HAVE_NETDB_H` macro is checked so
all occurrences are updated consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d8387f2-f28c-440b-a954-bb7658b7233a
📒 Files selected for processing (8)
CMakeLists.txtfetch_posix.cnet_adapter_linux.cnet_adapter_mac.cnet_util.cproxyres_config.h.inwpad_dhcp_posix.cwpad_dns.c
| check_include_file("netdb.h" HAVE_NETDB_H) | ||
| endif() | ||
|
|
||
| configure_file(proxyres_config.h.in proxyres_config.h @ONLY NEWLINE_STYLE UNIX) |
There was a problem hiding this comment.
Where is roxyres_config.h.in defined?
There was a problem hiding this comment.
proxyres_config.h.in - is a new file in the commit.
There was a problem hiding this comment.
What do you think about the name build_config.h?
There was a problem hiding this comment.
Other projects use such template <project>_config.h because build_config.h from distinct client projects would conflict. build_config.h is a free name in our code, but proxyres is OSS and we can break other user projects.
There was a problem hiding this comment.
Hmm I wonder about this now that I approved it, because proxyres_config.h is only used internally. Not externally. But you are suggesting that it would be used externally?
There was a problem hiding this comment.
No, I don't suggest it, and -I${PROJECT_BINARY_DIR} is in the private target properties. User -I order is unpredictable, so it's better to use a more unique header name than a less unique one.
| check_include_file("netdb.h" HAVE_NETDB_H) | ||
| endif() | ||
|
|
||
| configure_file(proxyres_config.h.in proxyres_config.h @ONLY NEWLINE_STYLE UNIX) |
There was a problem hiding this comment.
What do you think about the name build_config.h?
This will make possible future patches simpler: adding new lines rather than changing them (i.e. removing and adding).
Summary by CodeRabbit