Use bazel's native json decode method #659
Merged
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
shs96c
force-pushed
the
native-json
branch
2 times, most recently
from
e9681e6
to
3e10de6
Compare
This means that `rules_jvm_external` is only compatible with Bazel 4 and later. That shipped on 2021-01-21, and so should be widely adopted already. Older versions of this ruleset will continue to work for repos using older versions of Bazel so it's not removing any existing functionality, and the new Bazel 5 release should have already shipped. The main reason for doing this is that the native json decoding is significantly faster than the starlark-based one. The following options were explored as alternatives: 1. Making the `json_parse` method check the bazel version and optionally use the `json` module if it was present. This doesn't work because of #12735 2. Create a `json` repository rule that creates a repo that exposes a `json.bzl` file that delegates to the native `json` module if present, or the starlark one if not (as determined by looking at the major version number of bazel). This doesn't work because `maven_install` is used by RJE's own `repositories.bzl` file, so there's no chance to load the repository rule before it's needed. Links: 12735: bazelbuild/bazel#12735
jin
approved these changes
Feb 24, 2022
coursier.bzl
Outdated
| @@ -132,7 +122,9 @@ def _execute(repository_ctx, cmd, timeout = 600, environment = {}, progress_mess | |||
| # The representation of a Windows path when read from the parsed Coursier JSON | |||
| # is delimited by 4 back slashes. Replace them with 1 forward slash. | |||
| def _normalize_to_unix_path(path): | |||
| return path.replace("\\\\", "/") | |||
| if path == None: | |||
There was a problem hiding this comment.
Why this change? Seems error prone to have None paths being passed around.
There was a problem hiding this comment.
Just being paranoid. I'll back it out.
coursier.bzl
Outdated
| @@ -227,7 +219,7 @@ def _compute_dependency_tree_signature(artifacts): | |||
| if artifact["file"] != None: | |||
| artifact_group.extend([ | |||
| artifact["sha256"], | |||
| artifact["file"], | |||
| artifact["file"].replace("\\", "/"), # Make sure we represent files in a stable way cross-platform | |||
There was a problem hiding this comment.
why not use _normalize_to_unix_path? here and below
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.
This means that
rules_jvm_externalis only compatible withBazel 4 and later. That shipped on 2021-01-21, and so should
be widely adopted already. Older versions of this ruleset
will continue to work for repos using older versions of Bazel
so it's not removing any existing functionality, and the new
Bazel 5 release should have already shipped.
The main reason for doing this is that the native json
decoding is significantly faster than the starlark-based one.
The following options were explored as alternatives:
json_parsemethod check the bazel version andoptionally use the
jsonmodule if it was present. Thisdoesn't work because of
Starlark: provide a way to probe builtins availability bazel#12735
jsonrepository rule that creates a repo thatexposes a
json.bzlfile that delegates to the nativejsonmodule if present, or the starlark one if not(as determined by looking at the major version number of
bazel). This doesn't work because
maven_installis usedby RJE's own
repositories.bzlfile, so there's no chanceto load the repository rule before it's needed.