Skip to content

Conversation

@jwendell
Copy link

Currently it only creates a static library.

@bazel-io
Copy link
Member

Hello @UebelAndre, modules you maintain (numactl) have been updated in this PR.
Please review the changes. You can view a diff against the previous version in the "Generate module diff" check.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds a new BCR version for numactl to enable building a shared library. The changes look mostly good, with a new overlay for Bazel build files and a test module. I have two main points of feedback on the BUILD.bazel overlay file, both of which are based on the BCR style guide and best practices for creating reusable modules.

  1. The :util library is defined with linkstatic = True and alwayslink = True, which is overly restrictive for a public library. I've suggested removing these.
  2. The BCR style guide suggests having a library target named after the module. I've recommended adding an alias numactl for the main :numa library.

Addressing these points will improve the usability of the module for downstream consumers. Also, there's a small typo in the PR title ('lirary' instead of 'library').

Comment on lines +95 to +109
cc_library(
name = "util",
srcs = [
"util.c",
":gen_config_h",
],
hdrs = ["util.h"],
copts = COMMON_COPTS,
includes = ["."],
linkstatic = True,
target_compatible_with = LINUX_ONLY,
visibility = ["//visibility:public"],
deps = [":numa"],
alwayslink = True,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The util library is defined with linkstatic = True and alwayslink = True. These attributes are quite restrictive for consumers of this library. linkstatic = True forces static linking on all dependents, and alwayslink = True forces the library to be linked even if no symbols are used from it. Since this library has public visibility, these settings can be problematic for downstream users. It's better to let consumers decide how to link. The binaries within this module already request static linking via their own linkstatic attribute, so these attributes on the library are not necessary for them.

To provide more flexibility, I suggest removing linkstatic = True and alwayslink = True.

cc_library(
    name = "util",
    srcs = [
        "util.c",
        ":gen_config_h",
    ],
    hdrs = ["util.h"],
    copts = COMMON_COPTS,
    includes = ["."],
    target_compatible_with = LINUX_ONLY,
    visibility = ["//visibility:public"],
    deps = [":numa"],
)

Comment on lines +228 to +232
alias(
name = "libnuma",
actual = ":numa",
visibility = ["//visibility:public"],
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

According to the BCR style guide, for C++ overlays, there should be a public target named after the module.1 The module name is numactl, but there is no library target with this name. Please add an alias from numactl to the main library target :numa for better usability and to conform to the style guide.

alias(
    name = "libnuma",
    actual = ":numa",
    visibility = ["//visibility:public"],
)

alias(
    name = "numactl",
    actual = ":numa",
    visibility = ["//visibility:public"],
)

Style Guide References

Footnotes

  1. For C++ overlays: ensure a public target named after the module (or alias libfoo -> foo); visibility is minimal but includes //visibility:public for intended APIs.

Currently it only creates a static library.

Signed-off-by: Jonh Wendell <[email protected]>
@jwendell
Copy link
Author

The fact that we have to copy the whole directory to make a patch version makes review harder I think.

Here's the actual difference between current state and my changes:

--- 2.0.19/overlay/BUILD.bazel  2025-11-25 21:26:37.804820241 -0500
+++ 2.0.19.bcr.1/overlay/BUILD.bazel    2025-11-25 21:41:49.036779752 -0500
@@ -78,9 +78,14 @@
         "numacompat1.h",
         "numaif.h",
     ],
+    additional_linker_inputs = ["versions.ldscript"],
     copts = COMMON_COPTS,
     includes = ["."],
-    linkstatic = True,
+    linkopts = [
+        "-Wl,--version-script=$(location versions.ldscript)",
+        "-Wl,-init,numa_init",
+        "-Wl,-fini,numa_fini",
+    ],
     target_compatible_with = LINUX_ONLY,
     visibility = ["//visibility:public"],
     alwayslink = True,

@UebelAndre
Copy link
Contributor

@UebelAndre
Copy link
Contributor

@meteorcloudy this issue seems unrelated to the PR

(02:59:19) ERROR: /var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/5eb6e9292cf871cd001f959b7cd9a983/external/numactl+/BUILD.bazel:146:10: While resolving toolchains for target @@numactl+//:numademo (57c96fd): invalid registered toolchain '@local_jdk//:bootstrap_runtime_toolchain_definition': error loading package '@@rules_java++toolchains+local_jdk//': Unable to find package for @@[unknown repo 'local_config_platform' requested from @@rules_java++toolchains+local_jdk]//:constraints.bzl: The repository '@@[unknown repo 'local_config_platform' requested from @@rules_java++toolchains+local_jdk]' could not be resolved: No repository visible as '@local_config_platform' from repository '@@rules_java++toolchains+local_jdk'.
Target @@numactl+//:gen_config_h up-to-date:
  bazel-bin/external/numactl+/config.h

I'm wondering if it'd be better to remove rolling from CI. I've always felt it wasn't ideal to add rolling to CI. Is it possible to make jobs optional for CI? For any project I work on I only care about released LTS versions.

@meteorcloudy
Copy link
Member

It's up to the module maintainer to decide, I'm fine with removing rolling in the test matrix. And indeed, this particular failure is more like an issue of the rolling release.

@UebelAndre
Copy link
Contributor

@jwendell if you wanna remove it that's fine with me

@jwendell
Copy link
Author

/hold

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants