-
-
Notifications
You must be signed in to change notification settings - Fork 0
build: expose module for external consumption #41
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
build.zig
Outdated
| const sentry_module = b.addModule("sentry_zig", .{ | ||
| .root_source_file = b.path("src/root.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| }); | ||
|
|
||
| // Add the same dependencies that the library has | ||
| sentry_module.addOptions("sentry_build", sentry_build_opts); | ||
| sentry_module.addImport("types", types); | ||
|
|
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.
Potential bug: The new sentry_module in build.zig imports the types module but fails to provide its required utils dependency, causing compilation failures for external consumers.
-
Description: The new
sentry_moduleis configured to provide thetypesmodule to external consumers. However, thetypesmodule itself has a dependency on autilsmodule, as seen insrc/types/Event.zig. The build script fails to declare this nested dependency for thesentry_moduleby omitting a call liketypes.addImport("utils", sentry_module). This will cause a compilation failure for any external project that consumes the library via the new module system and calls a function likecaptureError, which relies on thetypesmodule. -
Suggested fix: In
build.zig, after adding thetypesimport tosentry_module, also add the requiredutilsimport for thetypesmodule. This can be done by addingtypes.addImport("utils", sentry_module);to ensure the dependency is resolved during compilation.
severity: 0.8, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
README.md
Outdated
| const sentry_zig_dep = b.addStaticLibrary(.{ | ||
| .name = "sentry_zig", | ||
| .root_source_file = b.path("deps/sentry-zig/src/root.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| }); | ||
| exe.linkLibrary(sentry_zig_dep); | ||
| exe.root_module.addImport("sentry_zig", sentry_zig_dep.root_module); |
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.
Potential bug: The "Git Submodule" integration method in README.md is broken. It bypasses the necessary build script setup, causing a compilation failure due to an unresolved types module import.
-
Description: The
README.mdfile presents two methods for integrating the library: Git Submodule and Package Manager. The "Git Submodule" method instructs the user to create a new static library directly fromsrc/root.zig. This approach completely bypasses the project'sbuild.zigfile, which is responsible for setting up necessary module dependencies, such as thetypesmodule. Sincesrc/root.zigdirectly importstypes, following these instructions will lead to a guaranteed compilation failure because thetypesmodule will not be found. -
Suggested fix: The "Git Submodule" instructions in
README.mdshould be removed or corrected. The correct approach must involve using the project'sbuild.zigto properly configure the library and its dependencies, rather than creating a static library from a single source file.
severity: 0.75, confidence: 0.9
Did we get this right? 👍 / 👎 to inform future reviews.
This way it can be used in other projects.