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

Allow root module's override tags to take precedence over the overridees from transitive deps. #1278

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -732,6 +732,25 @@ dev_maven.install(
],
)


dev_maven.install(
name = "root_module_can_override",
artifacts = ["com.squareup:javapoet:1.11.1"],
)

bazel_dep(name = "transitive_module_can_override", version = "0.0.0")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be marked as a dev_dependency so that consumers of us don't try and look up transitive_module_can_override in the BCR.

local_path_override(
module_name = "transitive_module_can_override",
path = "tests/integration/override_targets/module",
)

dev_maven.override(
# This override demonstrates that this root module's override takes precedence over that transitive override definition.
# Use something absurd for testing, like overriding okhttp3 to javapoet.
coordinates = "com.squareup.okhttp3:okhttp",
target = "@root_module_can_override//:com_squareup_javapoet",
)

# Where there are file locks, the pinned and unpinned repos are listed
# next to each other. Where compat repositories are created, they are
# listed next to the repo that created them. The list is otherwise kept
Expand Down Expand Up @@ -815,6 +834,7 @@ use_repo(
"starlark_aar_import_test",
"starlark_aar_import_with_sources_test",
"strict_visibility_testing",
"root_module_can_override",

# Repo with compat repos
"com_google_http_client_google_http_client_gson",
Expand Down
15 changes: 15 additions & 0 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -937,3 +937,18 @@ maven_install(
"https://repo1.maven.org/maven2",
],
)

# This failure mode is bzlmod only. But the test still runs on Bazel 5/6, which
# is WORKSPACE based, so we add a shim here to keep the test passing until
# WORKSPACE support is no longer needed.
maven_install(
name = "root_module_can_override",
artifacts = [
"com.squareup:javapoet:1.11.1",
"com.squareup.okhttp3:okhttp:4.12.0",
],
override_targets = {
"com.squareup.okhttp3:okhttp": "@root_module_can_override//:com_squareup_javapoet",
},
repositories = ["https://repo1.maven.org/maven2"],
)
17 changes: 13 additions & 4 deletions private/extensions/maven.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -227,13 +227,22 @@ def maven_impl(mctx):
# module attempts to update a maven repo (which is normally undesired behaviour)
repo_name_2_module_name = {}

for mod in mctx.modules:
# First compute the overrides. The order of the transitive overrides do not matter, but the root
# overrides take precedence over all transitive ones.
for idx, mod in enumerate(reversed(mctx.modules)):
# Rotate the root module to the last to be visited.
is_root_module = idx == (len(mctx.modules) - 1)
for override in mod.tags.override:
value = str(override.target)
current = overrides.get(override.coordinates, None)
to_use = _fail_if_different("Target of override for %s" % override.coordinates, current, value, [None])
overrides.update({override.coordinates: to_use})
if is_root_module:
# Allow the root module's overrides to take precedence over any transitive overrides.
to_use = value
else:
current = overrides.get(override.coordinates, None)
to_use = _fail_if_different("Target of override for %s" % override.coordinates, current, value, [None])
overrides.update({override.coordinates: value})

for mod in mctx.modules:
for artifact in mod.tags.artifact:
_check_repo_name(repo_name_2_module_name, artifact.name, mod.name)

Expand Down
17 changes: 17 additions & 0 deletions tests/integration/override_targets/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,20 @@ sh_test(
"@bazel_tools//tools/bash/runfiles",
],
)

genquery(
name = "root_module_can_override",
expression = "deps(@root_module_can_override//:com_squareup_okhttp3_okhttp)",
opts = [
"--nohost_deps",
"--noimplicit_deps",
],
scope = ["@root_module_can_override//:com_squareup_okhttp3_okhttp"],
)

sh_test(
name = "root_module_can_override_test",
srcs = ["root_module_can_override_test.sh"],
data = [":root_module_can_override"],
deps = ["@bazel_tools//tools/bash/runfiles"],
)
18 changes: 18 additions & 0 deletions tests/integration/override_targets/module/MODULE.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
module(name = "transitive_module_can_override", version = "0.0.0")

bazel_dep(name = "rules_jvm_external", version = "0.0")
local_path_override(
module_name = "rules_jvm_external",
path = "../../../..",
)

maven = use_extension("@rules_jvm_external//:extensions.bzl", "maven")
maven.install(
name = "root_module_can_override",
artifacts = ["com.squareup.okhttp3:okhttp:4.12.0"],
)

maven.override(
coordinates = "com.squareup.okhttp3:okhttp3",
target = "//:poison_pill_non_existent_target",
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# --- begin runfiles.bash initialization v2 ---
# Copy-pasted from the Bazel Bash runfiles library v2.
set -uo pipefail; f=bazel_tools/tools/bash/runfiles/runfiles.bash
source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \
source "$0.runfiles/$f" 2>/dev/null || \
source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
{ echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e
# --- end runfiles.bash initialization v2 ---

set -euox pipefail

deps_file=$(rlocation rules_jvm_external/tests/integration/override_targets/root_module_can_override)

function clean_up_workspace_names() {
local file_name="$1"
local target="$2"
# The first `sed` command replaces `@@` with `@`. The second extracts the visible name
# from the bzlmod mangled workspace name
cat "$file_name" | sed -e 's|^@@|@|g; s|\r||g' | sed -e 's|^@[^/]*[+~]|@|g; s|\r||g' | grep "$target"
cat "$file_name" | sed -e 's|^@@|@|g; s|\r||g' | sed -e 's|^@[^/]*[+~]|@|g; s|\r||g' | grep -q "$target"
}

if ! clean_up_workspace_names "$deps_file" "@root_module_can_override//:com_squareup_okhttp3_okhttp"; then
exit 1
fi

if ! clean_up_workspace_names "$deps_file" "@root_module_can_override//:com_squareup_javapoet"; then
exit 1
fi