Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add repository fields to all metadata.json files #3103

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adzenith
Copy link
Contributor

@adzenith adzenith commented Nov 4, 2024

There were ten metadata.json files that had no repository set:

$ rg homepage -tjson | wc -l
504
$ rg repository -tjson | wc -l
494

This PR sets all the missing repositories.

@bazel-io
Copy link
Member

bazel-io commented Nov 4, 2024

Hello @fmeum, modules you maintain (bazel_env.bzl, rules_jni, toolchains_musl) have been updated in this PR. Please review the changes.

@bazel-io
Copy link
Member

bazel-io commented Nov 4, 2024

Hello @Vertexwahn, modules you maintain (nasm) have been updated in this PR. Please review the changes.

@bazel-io
Copy link
Member

bazel-io commented Nov 4, 2024

Hello @comius, modules you maintain (rules_cc) have been updated in this PR. Please review the changes.

@bazel-io
Copy link
Member

bazel-io commented Nov 4, 2024

Hello @armandomontanez, @tpudlik, @cramertj, modules you maintain (rules_libusb) have been updated in this PR. Please review the changes.

@bazel-io
Copy link
Member

bazel-io commented Nov 4, 2024

Hello @avdv, @aherrmann, modules you maintain (rules_sh) have been updated in this PR. Please review the changes.

@bazel-io
Copy link
Member

bazel-io commented Nov 4, 2024

Hello @jmillikin, modules you maintain (sqlite3) have been updated in this PR. Please review the changes.

@bazel-io
Copy link
Member

bazel-io commented Nov 4, 2024

Hello @illicitonion, modules you maintain (toolchains_musl) have been updated in this PR. Please review the changes.

@bazel-io
Copy link
Member

bazel-io commented Nov 4, 2024

Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (bzip2, darts-clone) have been updated in this PR. Please review the changes.

@@ -7,6 +7,9 @@
"name": "John Millikin"
}
],
"repository": [
"github:sqlite/sqlite"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the official GitHub mirror. SQLite is developed in a custom VCS so I thought a Git repository link would be more useful

@meteorcloudy
Copy link
Member

meteorcloudy commented Nov 4, 2024

We should probably add "minItems": 1 at

"repository": {
"type": "array",
"items": {
"description": "repository, typically in the form github:[github org]/[github repo]",
"type": "string"
}

to make sure future modules are not missing this attribute?

/cc @alexeagle

@jmillikin
Copy link
Contributor

Modules with a GitHub repository set are required to use source archives hosted on GitHub:

def verify_source_archive_url_match_github_repo(self, module_name, version):
"""Verify the source archive URL matches the github repo. For now, we only support github repositories check."""
source_url = self.registry.get_source(module_name, version)["url"]
source_repositories = self.registry.get_metadata(module_name).get("repository", [])
matched = not source_repositories
for source_repository in source_repositories:
if matched:
break
repo_type, repo_path = source_repository.split(":")
if repo_type == "github":
parts = urlparse(source_url)
matched = (
parts.scheme == "https"
and parts.netloc == "github.com"
and os.path.abspath(parts.path).startswith(f"/{repo_path}/")
)
elif repo_type == "https":
repo = urlparse(source_repository)
parts = urlparse(source_url)
matched = (
parts.scheme == repo.scheme
and parts.netloc == repo.netloc
and os.path.abspath(parts.path).startswith(f"{repo.path}/")
)
if not matched:
self.report(
BcrValidationResult.FAILED,
f"The source URL of {module_name}@{version} ({source_url}) doesn't match any of the module's source repositories {source_repositories}.",
)
else:
self.report(BcrValidationResult.GOOD, "The source URL matches one of the source repositories.")

Wouldn't this change break uploads of modules that have GitHub projects, but host their releases elsewhere?

@meteorcloudy
Copy link
Member

Wouldn't this change break uploads of modules that have GitHub projects, but host their releases elsewhere?

http url is also supported as source repo, but yeah, we should make sure they actually match what's being used.

@jmillikin
Copy link
Contributor

I think I'm -1 on this as currently written, after spot-checking a few modules:

  • darts-clone has one source archive, under https://storage.googleapis.com/google-code-archive-downloads/, so setting a single github: repository will fail.
  • nasm has one source archive, under http://www.nasm.us/pub/nasm/releasebuilds/, so setting a single github: repository will fail.
  • Setting the bzip2 repository list to ["git://sourceware.org/git/bzip2.git"] will (if I read the code correctly) cause the source archive validation to always fail, because git: isn't a recognized repository type. You'd need to also have an https: repository in there, presumably https://sourceware.org/pub/bzip2/.
  • sqlite3 has source archives under https://sqlite.org/{year}/, so setting a single github: repository will fail.
  • rules_libusb has one source archive, under https://storage.googleapis.com/storage/v1/b/pigweed-bazel-tarballs/, so it won't match the repository prefix https://pigweed.googlesource.com/pigweed/rules_libusb.

It seems like there's a conflict of purposes between using repositories as metadata (for linking to upstream source control), and as a validation of archive URLs in BCR tooling.

@Wyverald
Copy link
Member

Wyverald commented Nov 5, 2024

I agree with @jmillikin -- IMO the "repository" field should only be set if we want to check the archive URL, and shouldn't be set just to document the location of the repository (the "homepage" field is perfectly fine for that). However, it could just mean that this PR should be updated to use generic URLs where appropriate instead of github:X/X identifiers as the "repository" field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants