-
Notifications
You must be signed in to change notification settings - Fork 171
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
propose bazel
type for Bazel modules
#317
base: main
Are you sure you want to change the base?
Conversation
CC @mzeren-vmw |
Any status on the feedback you waited for? |
@oej Yes, this has been approved and is ready for review! |
@stevespringett Could you review this? |
@pombredanne Not sure who to ask for a review, could you take a look? |
bazel
type for Bazel modulesbazel
type for Bazel modules
See also [1]. [1]: package-url/purl-spec#317 Signed-off-by: Frank Viernau <frank_viernau@epam.com>
See also [1]. [1]: package-url/purl-spec#317 Signed-off-by: Frank Viernau <frank_viernau@epam.com>
@fmeum please rebase to resolve conflicts. |
@sschuberth Done |
See also [1]. [1]: package-url/purl-spec#317 Signed-off-by: Frank Viernau <frank_viernau@epam.com>
See also [1]. [1]: package-url/purl-spec#317 Signed-off-by: Frank Viernau <frank_viernau@epam.com>
PURL-TYPES.rst
Outdated
- The ``version`` is the module version in `Bazel's relaxed semver format | ||
<https://bazel.build/external/module#version_format>`_. | ||
- The optional ``repository_url`` can be used to specify the URL of an | ||
alternative registry, with any trailing forward slashes removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with any trailing forward slashes removed.
I'm not sure about that bit. This is not one of the "type-specific normalizations" that are allowed for the namespace segments and name. And semantically, having a trailing slash or not does not make a difference for the URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just dropped this in a new commit.
PURL-TYPES.rst
Outdated
- The optional ``subpath`` can name a particular Bazel target in the module via | ||
a label with the leading double slash (``//``) removed and canonicalized by | ||
omitting the target name if it is equal to the name of the containing package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. The spec describes the subpath
as
extra subpath within a package, relative to the package root.
How is what you describe "relative to the package root"? Would a custom qualifier maybe make more sense to store the Bazel target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified this part of the spec in a new commit so that it's always a regular file path, corresponding to a package (not target or file) in the Bazel module. This is analogous to the usage of subpath
for golang
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now, but I'd like others to share their opinion as well.
See [1]. [1]: package-url/purl-spec#317 Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
The algorithm description at [1] demands to "apply type-specific normalization" to namespace segments and the name before applying percent-encoding. In general, type-specific requirements are documented at [2]. For Bazel the PR still pending, but in the current state lowercasing of the name should be performed [3]. [1]: https://github.com/package-url/purl-spec/blob/master/PURL-SPECIFICATION.rst#how-to-build-purl-string-from-its-components [2]: https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst [3]: package-url/purl-spec#317 Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
The algorithm description at [1] demands to "apply type-specific normalization" to namespace segments and the name before applying percent-encoding. In general, type-specific requirements are documented at [2]. For Bazel the PR still pending, but in the current state lowercasing of the name should be performed [3]. [1]: https://github.com/package-url/purl-spec/blob/master/PURL-SPECIFICATION.rst#how-to-build-purl-string-from-its-components [2]: https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst [3]: package-url/purl-spec#317 Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Can we add a test case for valid purls that Bazel will reject (e.g., pkg:bazel/Curl@1234
) to have coverage for the non-validation case?
Added via b29e1ea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about removing the reference to the version format for brevity?
# Conflicts: # test-suite-data.json # Conflicts: # test-suite-data.json
Co-authored-by: Philippe Ombredanne <pombredanne@gmail.com>
Co-authored-by: Jan Kowalleck <jan.kowalleck@gmail.com>
Co-authored-by: Philippe Ombredanne <pombredanne@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I really like the way it is set up now.
all the "external" bazel rules are left out, and we stay with just the bare mapping of purl components.
This way the type definition is functional, but not over-specific and free to evolve with bazel's needs.
Bazel 6 introduced a new system for managing external dependencies centered around the concept of Bazel modules, which are hosted in a registry. The default registry is the Bazel Central Registry. This system will become the default this year and its predecessor will be turned off next year.
As discussed in bazelbuild/bazel#23166, we would thus like to register the
bazel
purl type for Bazel modules, as specified in this PR.(Approved by the Rules Authors SIG: https://docs.google.com/document/d/1YGCYAGLzTfqSOgRFVsB8hDz-kEoTgTEKKp9Jd07TJ5c/edit#heading=h.9h67icc19g8f)