ability to silence warnings when using multiple repositories #349
Comments
|
Yes, this is pretty annoying. I'd expect this feature to be wired up to |
|
Suppressing the warning should be handled in But some options to actually do some dependency management are applicable here - essentially I think it's not a safe assumption that every artifact exists in every repository in a given maven_install target. In my particular case I have a large set of artifacts from Maven Central, but also a few artifacts from another repository. I could create two maven_install targets, but it leads to a potential issue as the transitive deps of the second artifact set may not be pinned to the versions I request from Central, plus developers have to figure out which workspace contains which artifact. Could rules_jvm_external avoid producing Perhaps the most elegant option would be to allow composing several maven_install targets into a single external workspace/maven_install.json - so we could specify repositories+artifacts in known-good combinations, but still benefit from a single |
|
How come the the old |
emitting the known-good URLs would be my preferred choice, maybe just checking if there's a HTTP 200 on the http response headers to reduce the overhead.
The r_j_e implementation is naive -- all |
I agree, this is the approach I've taken in bazel-dependencies-gradle-plugin - HEAD each artifact (including sources jars) from each repository and if you get a 200-399 code, treat it as a valid source for the artifact. The plugin can also emit a maven_install.json compatible with rules_jvm_external, so it's essentially just running the pinning step via another tool. But we end up with only safe URLs, and have the advantages of Gradle's dependency management flexibility (including e.g. Spring's dependency-management-plugin). |
Also the warning may be sort of new. I see our old code (before rules_jvm_external) used to use |
|
Hey @jin should we file an issue in bazelbuild/bazel on the repository_ctx.download function? Or do you think |
|
It's something that |
This is for bazel-contrib#349 and tangentially for bazel-contrib#723. Users are often confused by (expected) 404s when using multiple Maven repositories. These 404s can be safely ignored (Bazel's repository fetching logic gracefully tries all mirrors), but they are chatty and there is as of yet no way to silence them. See also: bazelbuild/bazel#13394 While we cannot solve the issue, we can reduce its impact by ordering the URLs: `http_file` is documented to try them in order: https://bazel.build/rules/lib/repo/http#http_archive-urls
This is for bazel-contrib#349 and tangentially for bazel-contrib#723. Users are often confused by (expected) 404s when using multiple Maven repositories. These 404s can be safely ignored (Bazel's repository fetching logic gracefully tries all mirrors), but they are chatty and there is as of yet no way to silence them. See also: bazelbuild/bazel#13394 While we cannot solve the issue, we can reduce its impact by ordering the URLs: `http_file` is documented to try them in order: https://bazel.build/rules/lib/repo/http#http_file-urls
This is for bazel-contrib#349 and tangentially for bazel-contrib#723. Users are often confused by (expected) 404s when using multiple Maven repositories. These 404s can be safely ignored (Bazel's repository fetching logic gracefully tries all mirrors), but they are chatty and there is as of yet no way to silence them. See also: bazelbuild/bazel#13394 While we cannot solve the issue, we can reduce its impact by ordering the URLs: `http_file` is documented to try them in order: https://bazel.build/rules/lib/repo/http#http_file-urls
This is for bazel-contrib#349 and tangentially for bazel-contrib#723. Users are often confused by (expected) 404s when using multiple Maven repositories. These 404s can be safely ignored (Bazel's repository fetching logic gracefully tries all mirrors), but they are chatty and there is as of yet no way to silence them. See also: bazelbuild/bazel#13394 While we cannot solve the issue, we can reduce its impact by ordering the URLs: `http_file` is documented to try them in order: https://bazel.build/rules/lib/repo/http#http_file-urls
This is for #349 and tangentially for #723. Users are often confused by (expected) 404s when using multiple Maven repositories. These 404s can be safely ignored (Bazel's repository fetching logic gracefully tries all mirrors), but they are chatty and there is as of yet no way to silence them. See also: bazelbuild/bazel#13394 While we cannot solve the issue, we can reduce its impact by ordering the URLs: `http_file` is documented to try them in order: https://bazel.build/rules/lib/repo/http#http_file-urls
|
#743 fixed this, but it looks like the change to the V2 lockfile format introduced a regression. |
## Explanation Fix part of #59 At a high-level, this PR migrates the project to using rules_jvm_external 5.1 (from the current version 4.1). The main benefit of this is a **much** simpler ``maven_install.json`` file (>50% reduction in file size). This is especially appealing as downstream PRs for #59 make a _lot_ of changes to third-party dependencies, causing ``maven_install.json`` to be regenerated each time. A smaller and more compact format results in fewer actual deltas needing to be checked in between changes. Introducing support for the new format has required a lot of changes elsewhere in the project, however, including: - A new Maven install Moshi model needing to be defined. - ``LicenseFetcher`` was extended to include support for verifying a URL's artifact validity (i.e. by using an HTTP HEAD request) for better robustness and performance when compiling licenses. This is because of a significant difference in the new ``maven_install.json`` format: we no longer know which Maven repositories correspond to which artifacts, so we need to test each artifact against all repositories when checking for licenses. This also has some repercussions on warnings from Bazel's own file downloader (see bazel-contrib/rules_jvm_external#349). - Per the previous point, ``MavenDependenciesRetriever`` required substantial reworking to properly fetch pom information from dependencies. For robustness, it was also updated to use ``ScriptBackgroundCoroutineDispatcher`` to parallelize the fetching with some basic retry built into it (since I noticed on one of my workstations much more flakiness in the HEAD requests and license fetches). - ``LicenseFetcher`` was also renamed to ``MavenArtifactPropertyFetcher`` since it's actually used for more than just license fetching (besides the new ``HEAD`` check, it's also used for fetching an artifact's POM file when processing the list of licenses for the artifact). Separate details worthy of noting: - The new version of rules_jvm_external introduces better support for duplicate dependency error handling and strict visibility. - Both of these are just nice-to-have robustness checks against the existing //third_party wrapping setup (so they won't impact developers directly unless changes are made to the //third_party wrapping Bazel code). - It also provides the ability to selectively override specific dependency targets (which is needed in #4931 to customize Guava in the build graph). - Source Jars are no longer being fetched to cut down on the amount of files that need to be downloaded. This may be reverted in the future. - Some minor dependency management was cleaned up in WORKSPACE. - The new ``MavenArtifactPropertyFetcherImpl`` & model changes don't have test files for the same reasons the old ``LicenseFetcherImpl`` and models didn't. - A lot of testing rework was needed for the license checks & update pathways since the means of representing licenses has fundamentally changed with the new ``maven_install.json`` format. The new retry functionality mentioned above also has resulted in a bit more output for these scripts (which results in changes to their tests). - The new ``MavenCoordinate`` class added as part of ``MavenDependenciesRetriever`` also had a bunch of tests added since this is now a public class of the retriever (and was added to simplify a lot of the coord manipulation work). - Some minor build time warnings were addressed during development. - Some of the new licenses for Crashlytics transitive dependencies refer to broken links that I cannot be confident in replacements for, but it seems reasonable to assume all of Crashlytics falls under Apache & the Crashlytics ToS based on the existing Crashlytics GitHub repository. Important: ``maven_dependencies.textproto`` was significantly changed due to two reasons: 1. The order of the output for ``GenerateMavenDependenciesList`` is different due to a combination of the reworking of ``MavenDependenciesRetriever`` and the new ``maven_install.json`` format. 2. Per my suspicion, I think the previous implementation wasn't including transitive dependencies correctly. The new ``maven_install.json`` format makes it much easier to track these dependencies, and so we'll now be including many more third party license information in released versions of the app. It is **not** expected that there are any actual version differences in this PR (intentionally since it would be very hard to verify that, and this allows ``maven_install.json`` to be easier to review since it's generated file). This was verified by looking at the conflicts section of both the old and new ``maven_install.json`` files, and the Maven dependencies CI check. ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only N/A -- This is a build system & script infrastructure change. It's expected to have no user-facing functional side effects. --------- Co-authored-by: Adhiambo Peres <[email protected]> Co-authored-by: Sean Lip <[email protected]>
When using multiple repositories, the assumption is that the repositories will be iterated in order and a dependency will be resolved from the first that succeeds. Failing to find a dependency in any given repository is not usually of interest if it is found in at least one. When dependency resolution fails, however, it is convenient to have this information to recreate each successive resolution attempt. Silencing these warnings in the positive case, if possible, would be helpful, but perhaps the call doesn't need to be made at all: I see multiple urls for "mirror_urls" in my pin file, where, for any given dependency, I expect only one to work. If the dependency was resolved once from a given mirror_url, could that information be stored, and that url be tried first?
An example of the error message I am referring to:
WARNING: Download from ... failed: class com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException GET returned 404 Not FoundThe text was updated successfully, but these errors were encountered: