Skip to content

Commit 78342d0

Browse files
committed
refactor(pypi): print a better error message for duplicate repos
Before this PR the error message would not be super helpful and may potentially make it hard to debug and report errors. This PR does the following: * Add a better error message which also adds comparison of the args with which we create the whl library. * Add a test that ensures that the error message is legible and works. * Add the necessary plumbing to logger to allow for testing error messages. A proper fix requires more work, so just adding better logging and error messages may be useful here. Work towards #3749
1 parent 2de083f commit 78342d0

File tree

4 files changed

+138
-11
lines changed

4 files changed

+138
-11
lines changed

python/private/pypi/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ bzl_library(
173173
":whl_repo_name_bzl",
174174
"//python/private:full_version_bzl",
175175
"//python/private:normalize_name_bzl",
176+
"//python/private:text_util_bzl",
176177
"//python/private:version_bzl",
177178
"//python/private:version_label_bzl",
178179
],

python/private/pypi/hub_builder.bzl

Lines changed: 63 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
load("//python/private:full_version.bzl", "full_version")
44
load("//python/private:normalize_name.bzl", "normalize_name")
55
load("//python/private:repo_utils.bzl", "repo_utils")
6+
load("//python/private:text_util.bzl", "render")
67
load("//python/private:version.bzl", "version")
78
load("//python/private:version_label.bzl", "version_label")
89
load(":attrs.bzl", "use_isolated")
@@ -86,6 +87,16 @@ def hub_builder(
8687
### PUBLIC methods
8788

8889
def _build(self):
90+
ret = struct(
91+
whl_map = {},
92+
group_map = {},
93+
extra_aliases = {},
94+
exposed_packages = [],
95+
whl_libraries = {},
96+
)
97+
if self._logger.failed():
98+
return ret
99+
89100
whl_map = {}
90101
for key, settings in self._whl_map.items():
91102
for setting, repo in settings.items():
@@ -196,6 +207,41 @@ def _add_extra_aliases(self, extra_hub_aliases):
196207
{alias: True for alias in aliases},
197208
)
198209

210+
def _diff_dict(first, second):
211+
"""A simple utility to shallow compare dictionaries.
212+
213+
to return a list of (key, value of first, value of second)
214+
"""
215+
keys = {}
216+
217+
missing = {}
218+
extra = {
219+
key: value
220+
for key, value in second.items()
221+
if key not in first
222+
}
223+
common = {}
224+
different = {}
225+
226+
for key, value in first.items():
227+
keys[key] = None
228+
if key not in second:
229+
missing[key] = value
230+
elif value == second[key]:
231+
common[key] = value
232+
else:
233+
different[key] = (value, second[key])
234+
235+
if missing or extra or different:
236+
return {
237+
"common": common,
238+
"different": different,
239+
"extra": extra,
240+
"missing": missing,
241+
}
242+
else:
243+
return None
244+
199245
def _add_whl_library(self, *, python_version, whl, repo, enable_pipstar):
200246
if repo == None:
201247
# NOTE @aignas 2025-07-07: we guard against an edge-case where there
@@ -207,13 +253,26 @@ def _add_whl_library(self, *, python_version, whl, repo, enable_pipstar):
207253

208254
# TODO @aignas 2025-06-29: we should not need the version in the repo_name if
209255
# we are using pipstar and we are downloading the wheel using the downloader
256+
#
257+
# However, for that we should first have a different way to reference closures with
258+
# extras. For example, if some package depends on `foo[extra]` and another depends on
259+
# `foo`, we should have 2 py_library targets.
210260
repo_name = "{}_{}_{}".format(self.name, version_label(python_version), repo.repo_name)
211261

212262
if repo_name in self._whl_libraries:
213-
fail("attempting to create a duplicate library {} for {}".format(
214-
repo_name,
215-
whl.name,
216-
))
263+
diff = _diff_dict(self._whl_libraries[repo_name], repo.args)
264+
if diff:
265+
self._logger.fail(lambda: (
266+
"Attempting to create a duplicate library {repo_name} for {whl_name} with different arguments. Already existing declaration has:\n".format(
267+
repo_name = repo_name,
268+
whl_name = whl.name,
269+
) + "\n".join([
270+
" {}: {}".format(key, render.indent(render.dict(value)).lstrip())
271+
for key, value in diff.items()
272+
if value
273+
])
274+
))
275+
return
217276
self._whl_libraries[repo_name] = repo.args
218277

219278
if not enable_pipstar and "experimental_target_platforms" in repo.args:

python/private/repo_utils.bzl

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,17 @@ def _is_repo_debug_enabled(mrctx):
3131
"""
3232
return _getenv(mrctx, REPO_DEBUG_ENV_VAR) == "1"
3333

34-
def _logger(mrctx = None, name = None, verbosity_level = None):
34+
def _logger(mrctx = None, name = None, verbosity_level = None, printer = None):
3535
"""Creates a logger instance for printing messages.
3636
3737
Args:
3838
mrctx: repository_ctx or module_ctx object. If the attribute
3939
`_rule_name` is present, it will be included in log messages.
4040
name: name for the logger. Optional for repository_ctx usage.
4141
verbosity_level: {type}`int | None` verbosity level. If not set,
42-
taken from `mrctx`
42+
taken from `mrctx`.
43+
printer: a function to use for printing. Defaults to `print` or `fail` depending
44+
on the logging method.
4345
4446
Returns:
4547
A struct with attributes logging: trace, debug, info, warn, fail.
@@ -70,10 +72,15 @@ def _logger(mrctx = None, name = None, verbosity_level = None):
7072
elif not name:
7173
fail("The name has to be specified when using the logger with `module_ctx`")
7274

75+
failures = []
76+
7377
def _log(enabled_on_verbosity, level, message_cb_or_str, printer = print):
7478
if verbosity < enabled_on_verbosity:
7579
return
7680

81+
if level == "FAIL":
82+
failures.append(None)
83+
7784
if type(message_cb_or_str) == "string":
7885
message = message_cb_or_str
7986
else:
@@ -86,11 +93,12 @@ def _logger(mrctx = None, name = None, verbosity_level = None):
8693
), message) # buildifier: disable=print
8794

8895
return struct(
89-
trace = lambda message_cb: _log(3, "TRACE", message_cb),
90-
debug = lambda message_cb: _log(2, "DEBUG", message_cb),
91-
info = lambda message_cb: _log(1, "INFO", message_cb),
92-
warn = lambda message_cb: _log(0, "WARNING", message_cb),
93-
fail = lambda message_cb: _log(-1, "FAIL", message_cb, fail),
96+
trace = lambda message_cb: _log(3, "TRACE", message_cb, printer or print),
97+
debug = lambda message_cb: _log(2, "DEBUG", message_cb, printer or print),
98+
info = lambda message_cb: _log(1, "INFO", message_cb, printer or print),
99+
warn = lambda message_cb: _log(0, "WARNING", message_cb, printer or print),
100+
fail = lambda message_cb: _log(-1, "FAIL", message_cb, printer or fail),
101+
failed = lambda: len(failures) != 0,
94102
)
95103

96104
def _execute_internal(

tests/pypi/hub_builder/hub_builder_tests.bzl

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ def hub_builder(
4848
whl_overrides = {},
4949
evaluate_markers_fn = None,
5050
simpleapi_download_fn = None,
51+
log_printer = None,
5152
available_interpreters = {}):
5253
builder = _hub_builder(
5354
name = "pypi",
@@ -96,6 +97,7 @@ def hub_builder(
9697
),
9798
),
9899
"unit-test",
100+
printer = log_printer,
99101
),
100102
)
101103
self = struct(
@@ -1245,6 +1247,63 @@ optimum[onnxruntime-gpu]==1.17.1 ; sys_platform == 'linux'
12451247

12461248
_tests.append(_test_pipstar_platforms_limit)
12471249

1250+
def _test_err_duplicate_repos(env):
1251+
logs = {}
1252+
log_printer = lambda key, message: logs.setdefault(key.strip(), []).append(message)
1253+
builder = hub_builder(
1254+
env,
1255+
available_interpreters = {
1256+
"python_3_15_1_host": "unit_test_interpreter_target_1",
1257+
"python_3_15_2_host": "unit_test_interpreter_target_2",
1258+
},
1259+
log_printer = log_printer,
1260+
)
1261+
builder.pip_parse(
1262+
_mock_mctx(
1263+
os_name = "osx",
1264+
arch_name = "aarch64",
1265+
),
1266+
_parse(
1267+
hub_name = "pypi",
1268+
python_version = "3.15.1",
1269+
requirements_lock = "requirements.txt",
1270+
),
1271+
)
1272+
builder.pip_parse(
1273+
_mock_mctx(
1274+
os_name = "osx",
1275+
arch_name = "aarch64",
1276+
),
1277+
_parse(
1278+
hub_name = "pypi",
1279+
python_version = "3.15.2",
1280+
requirements_lock = "requirements.txt",
1281+
),
1282+
)
1283+
pypi = builder.build()
1284+
1285+
pypi.exposed_packages().contains_exactly([])
1286+
pypi.group_map().contains_exactly({})
1287+
pypi.whl_map().contains_exactly({})
1288+
pypi.whl_libraries().contains_exactly({})
1289+
pypi.extra_aliases().contains_exactly({})
1290+
env.expect.that_dict(logs).keys().contains_exactly(["rules_python:unit-test FAIL:"])
1291+
env.expect.that_collection(logs["rules_python:unit-test FAIL:"]).contains_exactly([
1292+
"""\
1293+
Attempting to create a duplicate library pypi_315_simple for simple with different arguments. Already existing declaration has:
1294+
common: {
1295+
"dep_template": "@pypi//{name}:{target}",
1296+
"config_load": "@pypi//:config.bzl",
1297+
"requirement": "simple==0.0.1 --hash=sha256:deadbeef --hash=sha256:deadbaaf",
1298+
}
1299+
different: {
1300+
"python_interpreter_target": ("unit_test_interpreter_target_1", "unit_test_interpreter_target_2"),
1301+
}\
1302+
""",
1303+
]).in_order()
1304+
1305+
_tests.append(_test_err_duplicate_repos)
1306+
12481307
def hub_builder_test_suite(name):
12491308
"""Create the test suite.
12501309

0 commit comments

Comments
 (0)