-
Notifications
You must be signed in to change notification settings - Fork 31
Fix include paths for static_reflection_with_serialization headers #35
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
base: main
Are you sure you want to change the base?
Conversation
Source files were using incorrect relative paths for static_reflection_with_serialization headers, causing compilation failures with "No such file or directory" errors. The build system configures include directories that point to: - score/static_reflection_with_serialization/visitor/include - score/static_reflection_with_serialization/serialization/include But source files were incorrectly referencing headers with full paths instead of using the configured include directories. Changes: - Update include paths from 'static_reflection_with_serialization/visitor/' to 'visitor/' - Update include paths from 'static_reflection_with_serialization/serialization/' to 'serialization/' - Fix paths in main application files, internal headers, and test files Files affected: - JSON serializer and related headers/tests - Logging subsystem headers (log_entry, logging_identifier, dlt_message_builder) - Internal static_reflection_with_serialization library headers - All test files in serialization and visitor modules This resolves build failures in both local and container environments. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
6e8c064 to
d5806e6
Compare
| #include "static_reflection_with_serialization/visitor/visit.h" | ||
| #include "static_reflection_with_serialization/visitor/visit_as_struct.h" | ||
| #include "serialization/for_logging.h" | ||
| #include "visitor/visit.h" |
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.
Some use #include <visitor/visit.h>, some are like this ^. I would make them consistent.
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.
By the looks of it, it looks like claude kept the style that was used in each file.
I'm fine making them consistent but I'd wait to hear from a maintainer what are their preferred style :)
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.
FTR, claude is saying:
The C++ Include Convention:
Angle brackets < > are typically used for:
- System headers (standard library)
- External library headers
- Headers from separate packages/modules
Quotes " " are typically used for:
- Project-local headers
- Headers in the same module/component
- Internal implementation headers
And thus suggests we make them all use "" instead of <>
|
Note: Bazel is able to resolve the full path which plain gcc/g++ isn't. So this is fairly specific to the way I'm building the code. I'm fine keeping this patch on my fork it's not desired :) |
|
@4og can you please take a look |
Removes include_prefix from BUILD files to match source file expectations. Problem: - Bazel CI failing with "visitor/visit.h: No such file or directory" - Source files use short paths like visitor/visit.h (changed in d5806e6) - BUILD files still had include_prefix = "static_reflection_with_serialization" - This created headers at static_reflection_with_serialization/visitor/visit.h - Mismatch between what source expects vs what Bazel provides Investigation findings: - Oct 2025 (a7b3dc1): Added include_prefix + updated all source files to use full paths Purpose: Namespace headers to avoid conflicts with other libraries - Nov 2025 (d5806e6): Reverted source files to short paths, but forgot BUILD files Purpose: "Fix include paths" and "resolve build failures" Options considered: 1. Remove include_prefix (this commit) - matches recent source file changes 2. Revert source files to use full paths - matches namespacing intent Choosing Option 1 based on recent commit intent. Maintainer may prefer Option 2 if external API stability or namespace protection is more important. Files changed: - score/static_reflection_with_serialization/visitor/BUILD - score/static_reflection_with_serialization/serialization/BUILD 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
It looks like:
So I've added a patch that updates the BUILD files, which hopefully will fix the bazel build in CI. Again happy to keep this downstream if that doesn't align with your objectives. |
|
@pypingou, can you try to explain what failed to build in which environment, using what configuration and also how it failed? Because locally, the current main builds perfectly fine for me with |
|
@fbaeuerle I'm not building the project with Bazel but with gcc/g++ directly and that has some impacts. This is the reason why I'm offering this patch but I fully acknowledge that this isn't a problem you'll be facing with bazel, so I'm fine keeping this patch downstream just for me. I just wanted to bring this to your attention, bazel has a different way to deal with path in includes, so the current version works with bazel but not without. |
Source files were using incorrect relative paths for static_reflection_with_serialization
headers, causing compilation failures with "No such file or directory" errors.
The build system configures include directories that point to:
But source files were incorrectly referencing headers with full paths instead of
using the configured include directories.
Changes:
Files affected:
This resolves build failures in both local and container environments.
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com