Remove legacy providers #6721
Merged
Remove legacy providers #6721
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yatbear
added
type:build/install
dependencies
arcra
added a commit
to arcra/tensorboard
that referenced
this pull request
Mar 5, 2024
We haven't updated this dependency on the bazel webtesting rules for a while, and it looks like we need to do it to be able to also update our reference to the rules_closure target, to unblock tensorflow#6721. To be clear, this PR is a prerequisite to pin the rules_closure version in a separate PR, which in turn will unblock tensorflow#6721. An attempt to use the versioned browsers provided by the rules_webtesting dependency was created in tensorflow#6772, but some tests were failing. It seems that some components or tests were written in a way that depends on the specific browser (?), so to be able to update this dependency, this continues to use the same browser version, only updating the rules we use to define our testing browser, to be compatible with a newer version of rules_webtesting.
arcra
added a commit
that referenced
this pull request
Mar 5, 2024
We haven't updated this dependency on the bazel webtesting rules for a while, and it looks like we need to do it to be able to also update our reference to the rules_closure target, to unblock #6721. To be clear, this PR is a prerequisite to pin the rules_closure version in a separate PR, which in turn will unblock #6721. (Or at least that's the expectation. It is yet to be confirmed that the rules_closure version we want to pin is compatible with this one). An attempt to use the versioned browsers provided by the rules_webtesting dependency was created in #6772, but some tests were failing. It seems that some components or tests were written in a way that depends on the specific browser (?), so to be able to update this dependency, this continues to use the same browser version, only updating the rules we use to define our testing browser, to be compatible with a newer version of rules_webtesting.
comius
force-pushed
the
remove-legacy-providers
branch
from
230e3f1
to
a08add7
Compare
arcra
requested changes
Mar 13, 2024
| @@ -191,6 +188,11 @@ http_archive( | |||
| ], | |||
| ) | |||
|
|
|||
| # WORKAROUND for rules_webtesting not declaring used com_github_gorilla_mux repo: | |||
There was a problem hiding this comment.
Can we move these lines above where the other load statement from rules_webtesting is?
There was a problem hiding this comment.
No, I tried. com_github_gorilla_mux needs setup for Go repositories. It doesn't work even when used after rules_closure.
| # interfaces ~must be the same. | ||
| return [ | ||
| WebFilesInfo( | ||
| manifest = manifest, |
There was a problem hiding this comment.
Just checking, are both of these expected? manifest and manifests (one plural, one singular)?
There was a problem hiding this comment.
This is what's in your code at the moment: https://github.com/tensorflow/tensorboard/blob/master/tensorboard/defs/web.bzl#L126-L127
| ctx.label, | ||
| )) | ||
| new_webpaths.append(webpath) | ||
| manifest_srcs.append(struct( |
There was a problem hiding this comment.
Just checking... I understand this change is removing use of struct (is that right?)
Should I be looking to catch any uses of struct? Or it it specific ones from how they're used by rules_closure, so it's tricky to know which ones are right and which ones aren't?
There was a problem hiding this comment.
No, it's not removing all structs, just the structs returned directly from a rule. This one is not a problem.
| strip = ctx.attr.strip_prefix | ||
| if strip: | ||
| if strip.startswith("/"): | ||
| _fail(ctx, "strip_prefix should not end with /") |
There was a problem hiding this comment.
Pre-existing issue, but this should be "should not start with /"?
There was a problem hiding this comment.
I don't want to mix semantic changes into a cleanup cl. Please do the fixes as a followup.
Legacy struct providers have been deprecated by Bazel. Replacing them with modern providers, will make it possible to simplify and remove legacy handling from Bazel.
comius
force-pushed
the
remove-legacy-providers
branch
from
a08add7
to
752bda1
Compare
arcra
approved these changes
Mar 14, 2024
c-mita
added a commit
to c-mita/tensorboard
that referenced
this pull request
Mar 20, 2024
Reapplies tensorflow#6778 after tensorflow#6721 inadvertently reverted part of it. The "to_proto" method on Starlark structs is deprecated and shouldn't be used. Instead, the proto module's "encode_text" function should be used. (https://bazel.build/rules/lib/toplevel/proto) It, along with "to_json", can be disabled in Bazel using the flag --incompatible_struct_has_no_methods The underlying implementation is the same, so there should be no observable changes in final outputs.
c-mita
added a commit
to c-mita/tensorboard
that referenced
this pull request
Mar 26, 2024
Reapplies tensorflow#6778 after tensorflow#6721 inadvertently reverted part of it. The "to_proto" method on Starlark structs is deprecated and shouldn't be used. Instead, the proto module's "encode_text" function should be used. (https://bazel.build/rules/lib/toplevel/proto) It, along with "to_json", can be disabled in Bazel using the flag --incompatible_struct_has_no_methods The underlying implementation is the same, so there should be no observable changes in final outputs.
bmd3k
pushed a commit
that referenced
this pull request
Mar 26, 2024
Reapplies #6778 after #6721 inadvertently reverted part of it. The "to_proto" method on Starlark structs is deprecated and shouldn't be used. Instead, the proto module's "encode_text" function should be used. (https://bazel.build/rules/lib/toplevel/proto) It, along with "to_json", can be disabled in Bazel using the flag --incompatible_struct_has_no_methods The underlying implementation is the same, so there should be no observable changes in final outputs.
AnuarTB
pushed a commit
to AnuarTB/tensorboard
that referenced
this pull request
May 3, 2024
We haven't updated this dependency on the bazel webtesting rules for a while, and it looks like we need to do it to be able to also update our reference to the rules_closure target, to unblock tensorflow#6721. To be clear, this PR is a prerequisite to pin the rules_closure version in a separate PR, which in turn will unblock tensorflow#6721. (Or at least that's the expectation. It is yet to be confirmed that the rules_closure version we want to pin is compatible with this one). An attempt to use the versioned browsers provided by the rules_webtesting dependency was created in tensorflow#6772, but some tests were failing. It seems that some components or tests were written in a way that depends on the specific browser (?), so to be able to update this dependency, this continues to use the same browser version, only updating the rules we use to define our testing browser, to be compatible with a newer version of rules_webtesting.
AnuarTB
pushed a commit
to AnuarTB/tensorboard
that referenced
this pull request
May 3, 2024
## Motivation for features / changes Legacy struct providers have been deprecated by Bazel. Replacing them with modern providers, will make it possible to simplify and remove legacy handling from Bazel. Prerequiste: bazelbuild/rules_closure#599 is merged and released. Googlers, see cl/597800285.
AnuarTB
pushed a commit
to AnuarTB/tensorboard
that referenced
this pull request
May 3, 2024
Reapplies tensorflow#6778 after tensorflow#6721 inadvertently reverted part of it. The "to_proto" method on Starlark structs is deprecated and shouldn't be used. Instead, the proto module's "encode_text" function should be used. (https://bazel.build/rules/lib/toplevel/proto) It, along with "to_json", can be disabled in Bazel using the flag --incompatible_struct_has_no_methods The underlying implementation is the same, so there should be no observable changes in final outputs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for features / changes
Legacy struct providers have been deprecated by Bazel. Replacing them with modern providers, will make it possible to simplify and remove legacy handling from Bazel.
Prerequiste: bazelbuild/rules_closure#599 is merged and released.
Googlers, see cl/597800285.