Let Label#debugPrint emit label strings in display form#21963
Let Label#debugPrint emit label strings in display form#21963fmeum wants to merge 4 commits intobazelbuild:masterfrom
Label#debugPrint emit label strings in display form#21963Conversation
|
@tetromino I have one more, but it's purely mechanical. :-) |
tetromino
left a comment
There was a problem hiding this comment.
Could you please make the motivating change (for label printing) in the same PR?
StarlarkValue#debugPrint access to the StarlarkThreadLabel#debugPrint emit label strings in display form
Label#debugPrint emit label strings in display formLabel#debugPrint emit label strings in display form
|
@Wyverald Friendly ping |
Wyverald
left a comment
There was a problem hiding this comment.
sorry for the delay -- looks good to me! I'd still like @tetromino to take a look before we merge.
tetromino
left a comment
There was a problem hiding this comment.
Overall LGTM with the exception of a question about getMainRepositoryMapping
| BzlLoadValue.Key key, StarlarkSemantics starlarkSemantics, Environment env) | ||
| throws InterruptedException { | ||
| if (!starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD)) { | ||
| return Optional.empty(); |
There was a problem hiding this comment.
I don't understand why return empty here; you can have a repo map in legacy WORKSPACE mode.
There was a problem hiding this comment.
This has always been broken in numerous ways, @Wyverald can probably say more about it. As far as I know you also couldn't define a non-trivial repo mapping for the main repo, only for other repos.
There was a problem hiding this comment.
One undesirable result of a null main repo map though is that in legacy WORKSPACE mode, all non-main-repo labels will get printed in @@ form.
There was a problem hiding this comment.
@fmeum - observe that the weird corner cases for legacy WORKSPACE, bazel_tools etc. are already well-handled by getRepositoryMapping. So how about the following implementation of getMainRepositoryMapping, which falls back to getRepositoryMapping for the edge case logic:
@Nullable
private static RepositoryMapping getMainRepositoryMapping(
BzlLoadValue.Key key, StarlarkSemantics starlarkSemantics, Environment env)
throws InterruptedException {
boolean bzlmod = starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD);
RepositoryMappingValue.Key repoMappingKey;
if (key instanceof BzlLoadValue.KeyForBuild) {
repoMappingKey = RepositoryMappingValue.key(RepositoryName.MAIN);
} else if ((key instanceof BzlLoadValue.KeyForBzlmod
&& !(key instanceof BzlLoadValue.KeyForBzlmodBootstrap))
|| (bzlmod && key instanceof BzlLoadValue.KeyForWorkspace)) {
// Since the main repo mapping requires evaluating WORKSPACE, but WORKSPACE can load from
// extension repos, requesting the full main repo mapping would cause a cycle.
repoMappingKey = RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS;
} else {
// For builtins, @bazel_tools, and legacy WORKSPACE, the key's local repo mapping can be used
// as the main repo mapping.
return getRepositoryMapping(key, starlarkSemantics, env);
}
RepositoryMappingValue mainRepositoryMappingValue =
(RepositoryMappingValue) env.getValue(repoMappingKey);
if (mainRepositoryMappingValue == null) {
return null;
}
return mainRepositoryMappingValue.getRepositoryMapping();
}Note that this will also let you get rid of the Optional wrapper as a side effect
There was a problem hiding this comment.
Also, @brandjon - do you have any comments on putting the main repo mapping in module context? Or maybe we can rather stick it into StarlarkThread somehow (I don't see any clean way to do it, but maybe you do)?
There was a problem hiding this comment.
Thanks for the suggestion, this looks much better. I also added a test for the --noenable_bzlmod case.
There was a problem hiding this comment.
I talked to @brandjon about this PR in our daily sync; he suggested we try to put the main repository mapping pointer in BazelStarlarkContext rather than in module context (mainly for memory usage reasons; there may be a vast number of module context objects in flight in memory at any one time, but only a small number of starlark threads).
There was a problem hiding this comment.
BazelStarlarkContext isn't available when a module extension is evaluated. Should I introduce a new Phase for that or reuse LOADING?
There was a problem hiding this comment.
I am now using BazelStarlarkContext and the existing WORKSPACE phase for module extension evaluation, PTAL.
@Wyverald Could you verify that adding BazelStarlarkContext to module extensions' Starlark threads doesn't give extensions access to functions they shouldn't have access to?
src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
Show resolved
Hide resolved
6586341 to
2c03231
Compare
|
@tetromino friendly ping :) |
5426f16 to
c902c41
Compare
Since `debugPrint` is meant for emitting potentially non-deterministic, it is safe to give it access to `StarlarkThread` information. This will be used in a follow-up to let `print` and `fail` emit `Label`s in display form without having to track the reverse repo mapping for the sake of invalidation of repo rules or module extensions. Work towards bazelbuild#20486
c902c41 to
182568c
Compare
src/main/java/com/google/devtools/build/lib/cmdline/BazelStarlarkContext.java
Show resolved
Hide resolved
tetromino
left a comment
There was a problem hiding this comment.
LGTM and I apologize for the wait!
Thank you for getting this complex PR to a polished state!
This is a reland of 30b95e3 with a different approach to emitting display form labels that avoids adding a new `to_display_form()` method to `Label`: * In action command lines, which are the most frequent use of labels in rule implementation functions, labels are automatically emitted in display form since 9d3a8b0. * In module extensions and repository rules, if labels can be turned into display form, the inverse of the repository mapping would need to be tracked in lockfiles and marker files for correct incrementality. Furthermore, allowing implementation functions to access apparent names would allow them to "discriminate" against them, thus possibly breaking the user's capability to map repos at will via `use_repo` and `repo_name`. Similar to how providers on a target can't be enumerated, it is thus safer to not provide this information to the implementation functions directly. This change thus implements `StarlarkValue#debugPrint` for `Label` to allow ruleset authors to emit labels in display form in warnings and error messages while ensuring that Starlark logic doesn't have access to this information. `print("My message", label)` degrades gracefully with older Bazel versions (it just prints a canonical label literal) and can thus be adopted immediately without a need for feature detection. This requires changing the signature of `StarlarkValue#debugPrint` to receive the `StarlarkThread` instead of just the `StarlarkSemantics`. Since `debugPrint` is meant for emitting potentially non-deterministic information, it is safe to give it access to `StarlarkThread`. Also improves the Bzlmod cycle reporter so that it prints helpful information on a cycle encountered in a previous iteration of this PR. Fixes bazelbuild#20486 RELNOTES: `Label` instances passed to `print` or `fail` as positional arguments are now formatted with apparent repository names (optimized for human readability). Closes bazelbuild#21963. PiperOrigin-RevId: 635589078 Change-Id: If60fdc632a59f19dad0cb02312690c15a0540c8e
This is a reland of 30b95e3 with a different approach to emitting display form labels that avoids adding a new `to_display_form()` method to `Label`: * In action command lines, which are the most frequent use of labels in rule implementation functions, labels are automatically emitted in display form since 9d3a8b0. * In module extensions and repository rules, if labels can be turned into display form, the inverse of the repository mapping would need to be tracked in lockfiles and marker files for correct incrementality. Furthermore, allowing implementation functions to access apparent names would allow them to "discriminate" against them, thus possibly breaking the user's capability to map repos at will via `use_repo` and `repo_name`. Similar to how providers on a target can't be enumerated, it is thus safer to not provide this information to the implementation functions directly. This change thus implements `StarlarkValue#debugPrint` for `Label` to allow ruleset authors to emit labels in display form in warnings and error messages while ensuring that Starlark logic doesn't have access to this information. `print("My message", label)` degrades gracefully with older Bazel versions (it just prints a canonical label literal) and can thus be adopted immediately without a need for feature detection. This requires changing the signature of `StarlarkValue#debugPrint` to receive the `StarlarkThread` instead of just the `StarlarkSemantics`. Since `debugPrint` is meant for emitting potentially non-deterministic information, it is safe to give it access to `StarlarkThread`. Also improves the Bzlmod cycle reporter so that it prints helpful information on a cycle encountered in a previous iteration of this PR. Fixes bazelbuild#20486 RELNOTES: `Label` instances passed to `print` or `fail` as positional arguments are now formatted with apparent repository names (optimized for human readability). Closes bazelbuild#21963. PiperOrigin-RevId: 635589078 Change-Id: If60fdc632a59f19dad0cb02312690c15a0540c8e
…2460) This is a reland of 30b95e3 with a different approach to emitting display form labels that avoids adding a new `to_display_form()` method to `Label`: * In action command lines, which are the most frequent use of labels in rule implementation functions, labels are automatically emitted in display form since 9d3a8b0. * In module extensions and repository rules, if labels can be turned into display form, the inverse of the repository mapping would need to be tracked in lockfiles and marker files for correct incrementality. Furthermore, allowing implementation functions to access apparent names would allow them to "discriminate" against them, thus possibly breaking the user's capability to map repos at will via `use_repo` and `repo_name`. Similar to how providers on a target can't be enumerated, it is thus safer to not provide this information to the implementation functions directly. This change thus implements `StarlarkValue#debugPrint` for `Label` to allow ruleset authors to emit labels in display form in warnings and error messages while ensuring that Starlark logic doesn't have access to this information. `print("My message", label)` degrades gracefully with older Bazel versions (it just prints a canonical label literal) and can thus be adopted immediately without a need for feature detection. This requires changing the signature of `StarlarkValue#debugPrint` to receive the `StarlarkThread` instead of just the `StarlarkSemantics`. Since `debugPrint` is meant for emitting potentially non-deterministic information, it is safe to give it access to `StarlarkThread`. Also improves the Bzlmod cycle reporter so that it prints helpful information on a cycle encountered in a previous iteration of this PR. Fixes #20486 RELNOTES: `Label` instances passed to `print` or `fail` as positional arguments are now formatted with apparent repository names (optimized for human readability). Closes #21963. PiperOrigin-RevId: 635589078 Change-Id: If60fdc632a59f19dad0cb02312690c15a0540c8e Closes #22136
This is a reland of 30b95e3 with a different approach to emitting display form labels that avoids adding a new
to_display_form()method toLabel:use_repoandrepo_name. Similar to how providers on a target can't be enumerated, it is thus safer to not provide this information to the implementation functions directly.This change thus implements
StarlarkValue#debugPrintforLabelto allow ruleset authors to emit labels in display form in warnings and error messages while ensuring that Starlark logic doesn't have access to this information.print("My message", label)degrades gracefully with older Bazel versions (it just prints a canonical label literal) and can thus be adopted immediately without a need for feature detection.This requires changing the signature of
StarlarkValue#debugPrintto receive theStarlarkThreadinstead of just theStarlarkSemantics. SincedebugPrintis meant for emitting potentially non-deterministic information, it is safe to give it access toStarlarkThread.Also improves the Bzlmod cycle reporter so that it prints helpful information on a cycle encountered in a previous iteration of this PR.
Fixes #20486
RELNOTES:
Labelinstances passed toprintorfailas positional arguments are now formatted with apparent repository names (optimized for human readability).