Derive jvm_import debug label from explicit attribute #1210
Conversation
The delimiter used in Bzlmod for repo names has never been stable and will be changing soon.[^1] String-parsing to derive shorter, user-friendly names for labels will be broken by this change and is brittle in general. Instead, passing the right values down from the contexts where the true values are known is preferred. [^1]: bazelbuild/bazel#23127
It is easier to make all callers consistent than to handle more cases in the implementation.
plobsing
requested review from
jin,
Wyverald,
shs96c and
cheister
as code owners
Its a more accurate name for what this is.
| @@ -123,6 +123,7 @@ def maven_install( | |||
| # created from the maven_install.json file in the coursier_fetch | |||
| # invocation after this. | |||
| name = name if maven_install_json == None else "unpinned_" + name, | |||
| user_provided_name = name, | |||
There was a problem hiding this comment.
Instead of adding another parameter, why not derive this from pinned_repo_name, and if it's None or not an attr in the case of pinned_coursier_fetch, use name?
There was a problem hiding this comment.
Inside a repo rule,repository_ctx.attr.name is the fully-qualified repo name; it is no longer the shortname passed when declaring the repo. Using that value yields the following test failure:
INFO: From Testing //tests/unit/manifest_stamp:diff_manifest_test:
==================== Test output for //tests/unit/manifest_stamp:diff_manifest_test:
2c2
< Target-Label: @manifest_stamp_testing//:com_google_guava_guava
---
> Target-Label: @_main~maven~manifest_stamp_testing//:com_google_guava_guava
FAIL: files "tests/unit/manifest_stamp/guava_MANIFEST.MF.golden.unix" and "tests/unit/manifest_stamp/guava_MANIFEST.MF" differ.
IIUC, the preferred way to deal with this is to pass a second string-valued attr, that gets the same value as name, at the declaration site; since this attr is user-declared, it doesn't have any platform magic associated with it - it keeps the same value into the repo rule.
So, the way I see it, we have a need for two user-declared attrs: the shortname of this repo, and the shortname of the assocaited pinned repo (in the case of unpinned repos). I could be wrong, and maybe there's some way to get by without yet another attr here, but I couldn't work out how to do it - the magic associated with the name attr interferes with what we're trying to do.
There was a problem hiding this comment.
bazelbuild/bazel#20486 is relevant here. I wonder whether the limitation to debug_print might be a problem. We also have several places in the repo where we calculate the "user visible" name from the repo name. We should really gather those up and have a utility method that will do this for us. #1219 is also relevant.
There was a problem hiding this comment.
Neat. Should we go with that change instead? Happy to close this if we do; I only care about the result.
|
I've opened #1224 to help make it easier to get the visible name of a repo. |
|
I think that this has now been fixed by #1219. Could you please check and close this PR if necessary? I'd be happy to re-review if necessary. |
|
Fixed by PR #1224. |
The delimiter used in Bzlmod for repo names has never been stable and
will be changing soon.1 String-parsing to derive shorter,
user-friendly names for labels will be broken by this change and is
brittle in general. Instead, passing the right values down from the
contexts where the true values are known is preferred.
Footnotes
https://github.com/bazelbuild/bazel/issues/23127 ↩