Skip to content
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

Unexpected builtin dependencies being loaded #24871

Open
UebelAndre opened this issue Jan 9, 2025 · 21 comments
Open

Unexpected builtin dependencies being loaded #24871

UebelAndre opened this issue Jan 9, 2025 · 21 comments
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: support / not a bug (process)

Comments

@UebelAndre
Copy link
Contributor

UebelAndre commented Jan 9, 2025

Description of the bug:

I noticed on bazelbuild/rules_rust#3077 that one of the extensions fails with references to rules_foreign_cc which rules_rust does not use (to my knowledge) for any dependencies.
https://buildkite.com/bazel/rules-rust-rustlang/builds/13675#0194496d-7d43-4239-a8b9-bf9b265ad98d

(04:58:40) ERROR: Traceback (most recent call last):
	File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/c6c4a40d41fee053967cb47ffd3a0778/external/rules_foreign_cc+/toolchains/BUILD.bazel", line 317, column 15, in <toplevel>
		pkgconfig_tool(
	File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/c6c4a40d41fee053967cb47ffd3a0778/external/rules_foreign_cc+/foreign_cc/built_tools/pkgconfig_build.bzl", line 104, column 20, in pkgconfig_tool
		runnable_binary(
	File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/c6c4a40d41fee053967cb47ffd3a0778/external/rules_foreign_cc+/foreign_cc/utils.bzl", line 61, column 11, in runnable_binary
		native.sh_binary(
Error: no native function or rule 'sh_binary'
Available attributes: action_listener, alias, available_xcodes, cc_binary, cc_import, cc_libc_top_alias, cc_library, cc_shared_library, cc_static_library, cc_test, cc_toolchain, cc_toolchain_alias, cc_toolchain_suite, config_feature_flag, config_setting, constraint_setting, constraint_value, environment, existing_rule, existing_rules, exports_files, extra_action, fdo_prefetch_hints, fdo_profile, filegroup, genquery, genrule, glob, java_binary, java_import, java_library, java_package_configuration, java_plugin, java_plugins_flag_alias, java_runtime, java_test, java_toolchain, label_flag, label_setting, legacy_globals, memprof_profile, module_name, module_version, objc_import, objc_library, package, package_group, package_name, package_relative_label, platform, propeller_optimize, repo_name, repository_name, starlark_doc_extract, subpackages, test_suite, toolchain, toolchain_type, xcode_config, xcode_config_alias, xcode_version
(04:58:40) ERROR: /workdir/extensions/prost/private/tests/package_imports/BUILD.bazel:31:19: While resolving toolchains for target //private/tests/package_imports:package_importer_rs_proto (208597c): invalid registered toolchain '@rules_foreign_cc//toolchains:preinstalled_automake_toolchain': Error evaluating '@rules_foreign_cc//toolchains:preinstalled_automake_toolchain': error loading package '@@rules_foreign_cc+//toolchains': Package 'toolchains' contains errors

Note that rules_rust uses --incompatible_autoload_externally=

I ran bazel build --experimental_repository_resolved_file=resolved.json //... and found the following entry:

     {
          "original_rule_class": "@@bazel_tools//tools/build_defs/repo:http.bzl%http_archive",
          "definition_information": "Repository rules_foreign_cc~ instantiated at:\n  <builtin>: in <toplevel>\nRepository rule http_archive defined at:\n  /private/var/tmp/_bazel_user/ee718586e70ecc7cc89f275b4f545d0b/external/bazel_tools/tools/build_defs/repo/http.bzl:387:31: in <toplevel>\n",
          "original_attributes": {
               "name": "rules_foreign_cc~",
               "urls": [
                    "https://github.com/bazelbuild/rules_foreign_cc/releases/download/0.10.1/rules_foreign_cc-0.10.1.tar.gz"
               ],
               "integrity": "sha256-R2MDvQ8bBMwxH8JY8XCKX274LTCR5T/Rl3+iA4NCWmo=",
               "strip_prefix": "rules_foreign_cc-0.10.1",
               "remote_file_urls": {},
               "remote_file_integrity": {},
               "remote_patches": {
                    "https://bcr.bazel.build/modules/rules_foreign_cc/0.10.1/patches/module_dot_bazel.patch": "sha256-hDvLi+Nx91lvhEd2qRrPfPu0RjiG5w3a/c4N4AiJb3U="
               },
               "remote_patch_strip": 0
          },
          "repositories": [
               {
                    "rule_class": "@@bazel_tools//tools/build_defs/repo:http.bzl%http_archive",
                    "attributes": {
                         "url": "",
                         "urls": [
                              "https://github.com/bazelbuild/rules_foreign_cc/releases/download/0.10.1/rules_foreign_cc-0.10.1.tar.gz"
                         ],
                         "sha256": "",
                         "integrity": "sha256-R2MDvQ8bBMwxH8JY8XCKX274LTCR5T/Rl3+iA4NCWmo=",
                         "netrc": "",
                         "auth_patterns": {},
                         "canonical_id": "",
                         "strip_prefix": "rules_foreign_cc-0.10.1",
                         "add_prefix": "",
                         "type": "",
                         "patches": [],
                         "remote_file_urls": {},
                         "remote_file_integrity": {},
                         "remote_patches": {
                              "https://bcr.bazel.build/modules/rules_foreign_cc/0.10.1/patches/module_dot_bazel.patch": "sha256-hDvLi+Nx91lvhEd2qRrPfPu0RjiG5w3a/c4N4AiJb3U="
                         },
                         "remote_patch_strip": 0,
                         "patch_tool": "",
                         "patch_args": [
                              "-p0"
                         ],
                         "patch_cmds": [],
                         "patch_cmds_win": [],
                         "build_file_content": "",
                         "workspace_file_content": "",
                         "name": "rules_foreign_cc~"
                    },
                    "output_tree_hash": "066e589cb8a8722e5f06ca827d3a85fa31e659034f83eccd7ad4c1400a06aa37"
               }
          ]
     },

Why would this be showing up at all?

Which category does this issue belong to?

No response

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Which operating system are you running Bazel on?

Linux, Macos, Windows

What is the output of bazel info release?

8.0.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

No response

If this is a regression, please try to identify the Bazel commit where the bug was introduced with bazelisk --bisect.

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@fmeum
Copy link
Collaborator

fmeum commented Jan 9, 2025

@comius

@satyanandak satyanandak added the team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. label Jan 9, 2025
@meteorcloudy
Copy link
Member

The dependency path that led to rules_foreign_cc seems to be

├───[email protected]
│   ├───[email protected]
│   │   ├───[email protected]
│   │   │   ├───[email protected]
│   │   │   │   └───[email protected] (*)
│   │   │   └───[email protected]

@meteorcloudy
Copy link
Member

meteorcloudy commented Jan 20, 2025

Looks like what actually needs rules_foriegn_cc is only libpfm and its Makefile doesn't look too complicated. Maybe we should just create a proper BUILD file for it.

@UebelAndre
Copy link
Contributor Author

The dependency path that led to rules_foreign_cc seems to be

├───[email protected]
│   ├───[email protected]
│   │   ├───[email protected]
│   │   │   ├───[email protected]
│   │   │   │   └───[email protected] (*)
│   │   │   └───[email protected]

Why is google_benchmark a non-dev dependency to grpc? I would have expected builtins to be minimal given that they're versioned with the current version of Bazel and thus are hard for users to change. Or is that not the case?

@fmeum
Copy link
Collaborator

fmeum commented Jan 20, 2025

bazelbuild/bazel-central-registry#3472 may help cut the dependency on grpc.

@meteorcloudy
Copy link
Member

Why is google_benchmark a non-dev dependency to grpc?

/cc @veblush @mering Maybe it doesn't have to be?

@mering
Copy link

mering commented Jan 21, 2025

gRPC doesn't use a separate testing module. IIRC The problem was parsing BUILD files loading google_benchmark fails without this dependency.

@mering
Copy link

mering commented Jan 21, 2025

Checking where it is used in https://github.com/search?q=repo%3Agrpc%2Fgrpc+google_benchmark&type=code, doesn't yield any results. Maybe this dependency could just be removed?

@meteorcloudy
Copy link
Member

@mering Indeed, I guess so. At least for building the targets listed in the presubmit.yml file.

github-merge-queue bot pushed a commit to bazelbuild/rules_rust that referenced this issue Jan 29, 2025
…el8 (#3214)

bazelbuild/bazel#24871 will need to be
addressed somehow where unnecessary "built-ins" are either not forcibly
pulled in or all built-ins conform to this flag.
@UebelAndre
Copy link
Contributor Author

What is the expectation for users of Bazel? What is the fix we should be expecting and will it be something that requires a Bazel version upgrade? If so it would seem like this isn't gonna be addressed until Bazel 8.2

@meteorcloudy
Copy link
Member

In the latest grpc version, google_benchmark and a few other dependencies have been removed from MODULE.bazel:
https://github.com/bazelbuild/bazel-central-registry/blob/main/modules/grpc/1.70.1/MODULE.bazel

Can you check if upgrading to this version helps?

@UebelAndre
Copy link
Contributor Author

In the latest grpc version, google_benchmark and a few other dependencies have been removed from MODULE.bazel: https://github.com/bazelbuild/bazel-central-registry/blob/main/modules/grpc/1.70.1/MODULE.bazel

Can you check if upgrading to this version helps?

Trying the following diff to rules_rust still reproduces the issue

diff --git a/extensions/prost/MODULE.bazel b/extensions/prost/MODULE.bazel
index 7d2449ea..86d4c063 100644
--- a/extensions/prost/MODULE.bazel
+++ b/extensions/prost/MODULE.bazel
@@ -34,6 +34,12 @@ bazel_dep(
     version = "28.3",
     repo_name = "com_google_protobuf",
 )
+bazel_dep(
+    name = "grpc",
+    version = "1.70.1",
+    repo_name = "com_github_grpc_grpc",
+)
+

@meteorcloudy
Copy link
Member

I guess google_benchmark is still introduced via some other path?

@UebelAndre
Copy link
Contributor Author

While it would be good to understand the exact dependency graph of the <builtins> dependencies, I've put up bazelbuild/bazel-central-registry#3764 to try and mitigate this by solving the specific issue of using rules_foreign_cc.

meteorcloudy added a commit to bazelbuild/bazel-central-registry that referenced this issue Feb 10, 2025
This has been tested against
[googlebench:`9d8201efd4cbbe6271d0579ec2047dbfc396d22d`](https://github.com/google/benchmark/tree/9d8201efd4cbbe6271d0579ec2047dbfc396d22d)
using the following diff
```diff
diff --git a/MODULE.bazel b/MODULE.bazel
index 62a3aa8..107a955 100644
--- a/MODULE.bazel
+++ b/MODULE.bazel
@@ -12,6 +12,10 @@ bazel_dep(name = "rules_python", version = "1.0.0", dev_dependency = True)
 bazel_dep(name = "googletest", version = "1.14.0", dev_dependency = True, repo_name = "com_google_googletest")

 bazel_dep(name = "libpfm", version = "4.11.0")
+local_path_override(
+    module_name = "libpfm",
+    path = "../libpfm",
+)

 # Register a toolchain for Python 3.9 to be able to build numpy. Python
 # versions >=3.10 are problematic.
```

```
~/google_benchmark$ bazel test //test/...
WARNING: For repository 'platforms', the root module requires module version [email protected], but got [email protected] in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'rules_cc', the root module requires module version [email protected], but got [email protected] in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'com_google_googletest', the root module requires module version [email protected], but got [email protected] in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
INFO: Analyzed 44 targets (1 packages loaded, 105 targets configured).
INFO: Found 1 target and 43 test targets...
INFO: Elapsed time: 21.765s, Critical Path: 19.76s
INFO: 285 processes: 5 action cache hit, 101 internal, 184 linux-sandbox.
INFO: Build completed successfully, 285 total actions
//test:args_product_test                                                 PASSED in 0.3s
//test:basic_test                                                        PASSED in 1.3s
//test:benchmark_gtest                                                   PASSED in 0.1s
//test:benchmark_min_time_flag_iters_test                                PASSED in 0.1s
//test:benchmark_min_time_flag_time_test                                 PASSED in 17.0s
//test:benchmark_name_gtest                                              PASSED in 0.0s
//test:benchmark_random_interleaving_gtest                               PASSED in 0.1s
//test:benchmark_setup_teardown_test                                     PASSED in 0.1s
//test:benchmark_test                                                    PASSED in 5.8s
//test:commandlineflags_gtest                                            PASSED in 0.0s
//test:complexity_test                                                   PASSED in 3.5s
//test:cxx03_test                                                        PASSED in 7.9s
//test:diagnostics_test                                                  PASSED in 0.1s
//test:display_aggregates_only_test                                      PASSED in 0.1s
//test:donotoptimize_test                                                PASSED in 0.1s
//test:filter_test                                                       PASSED in 0.2s
//test:fixture_test                                                      PASSED in 0.1s
//test:internal_threading_test                                           PASSED in 1.9s
//test:link_main_test                                                    PASSED in 0.9s
//test:map_test                                                          PASSED in 0.3s
//test:memory_manager_test                                               PASSED in 0.1s
//test:min_time_parse_gtest                                              PASSED in 0.6s
//test:multiple_ranges_test                                              PASSED in 0.5s
//test:options_test                                                      PASSED in 5.3s
//test:perf_counters_gtest                                               PASSED in 0.1s
//test:perf_counters_test                                                PASSED in 0.1s
//test:profiler_manager_gtest                                            PASSED in 0.2s
//test:profiler_manager_iterations_test                                  PASSED in 0.1s
//test:profiler_manager_test                                             PASSED in 0.1s
//test:register_benchmark_test                                           PASSED in 0.2s
//test:repetitions_test                                                  PASSED in 0.2s
//test:report_aggregates_only_test                                       PASSED in 0.2s
//test:reporter_output_test                                              PASSED in 0.9s
//test:skip_with_error_test                                              PASSED in 0.4s
//test:spec_arg_test                                                     PASSED in 0.1s
//test:spec_arg_verbosity_test                                           PASSED in 0.1s
//test:statistics_gtest                                                  PASSED in 0.1s
//test:string_util_gtest                                                 PASSED in 0.1s
//test:templated_fixture_test                                            PASSED in 0.1s
//test:time_unit_gtest                                                   PASSED in 0.1s
//test:user_counters_tabular_test                                        PASSED in 0.8s
//test:user_counters_test                                                PASSED in 0.6s
//test:user_counters_thousands_test                                      PASSED in 0.2s

Executed 43 out of 43 tests: 43 tests pass.
```

relates to bazelbuild/bazel#24871

---------

Co-authored-by: Yun Peng <[email protected]>
@UebelAndre
Copy link
Contributor Author

UebelAndre commented Feb 10, 2025

With bazelbuild/bazel-central-registry#3764 now merged would someone be able to make sure however the old dependency was getting pulled in it's switched to this new one?

I don't have any context on how the builtins are setup and imposed on Bazel users.

@meteorcloudy
Copy link
Member

I think adding a bazel_dep on the new libpfm should help. Although it's not your direct dependency. #25214 might help do this more elegantly.

@UebelAndre
Copy link
Contributor Author

UebelAndre commented Feb 10, 2025

I think adding a bazel_dep on the new libpfm should help. Although it's not your direct dependency. #25214 might help do this more elegantly.

While I've reported this issue on behalf of rules_rust, it's gonna impact everyone everywhere, right? I don't think having everyone pin this is an effective solution if so. Also, isn't bzlmod designed to ensure repos requesting externals at a specific version are given that version? As long as googlebench is using this old pin, there is no way around this issue, right?

@UebelAndre
Copy link
Contributor Author

I have also opened google/benchmark#1922

@Wyverald
Copy link
Member

I don't have any context on how the builtins are setup and imposed on Bazel users.

I haven't carefully read through this thread yet, but just to answer this question: All modules have an implicit dependency on the builtin module bazel_tools, whose MODULE.bazel file is here: https://cs.opensource.google/bazel/bazel/+/master:src/MODULE.tools?q=module.tools&ss=bazel%2Fbazel

This module is shipped with Bazel itself, so whenever you update your Bazel version, this module's contents (and deps) can potentially change too.

@meteorcloudy
Copy link
Member

While I've reported this issue on behalf of rules_rust, it's gonna impact everyone everywhere, right?

Thanks for sending the fix to google-benchmark! However, I don't think this dependency was introduced via the builtin @bazel_tools module. We do have a check on which modules are fetched by default.

expected_modules=(
abseil-cpp
bazel_features
bazel_skylib
buildozer
googletest
jsoncpp
platforms
protobuf
pybind11_bazel
re2
rules_android
rules_cc
rules_fuzzing
rules_java
rules_jvm_external
rules_kotlin
rules_license
rules_pkg
rules_proto
rules_python
rules_shell
stardoc
zlib
)

And I think the root cause is actually for rules_foriegn_cc to fix Error: no native function or rule 'sh_binary'. /cc @jsharpe

@UebelAndre
Copy link
Contributor Author

UebelAndre commented Feb 11, 2025

While I've reported this issue on behalf of rules_rust, it's gonna impact everyone everywhere, right?

Thanks for sending the fix to google-benchmark! However, I don't think this dependency was introduced via the builtin @bazel_tools module. We do have a check on which modules are fetched by default.

expected_modules=(
abseil-cpp
bazel_features
bazel_skylib
buildozer
googletest
jsoncpp
platforms
protobuf
pybind11_bazel
re2
rules_android
rules_cc
rules_fuzzing
rules_java
rules_jvm_external
rules_kotlin
rules_license
rules_pkg
rules_proto
rules_python
rules_shell
stardoc
zlib
)

And I think the root cause is actually for rules_foriegn_cc to fix Error: no native function or rule 'sh_binary'. /cc @jsharpe

Yes, while that issue is in rules_foreign_cc, I've gone a step farther and am making the claim that rules_foreign_cc should not be in @bazel_tools at all. rules_foreign_cc is known to not be hermetic, deterministic, or {fast, correct} due to it being a rule set that spawns other build systems in actions to make building some older projects more convenient to it's users. It feels inappropriate for Bazel itself to effectively be introducing this kind of behavior for users through @bazel_tools so I opted to remove it's use from this tiny library. What is not clear to me is if there are more uses or not, but if there are, they should be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: support / not a bug (process)
Projects
None yet
Development

No branches or pull requests

8 participants