propose bazel type for Bazel modules
#317
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? |
jkowalleck
changed the title
bazel type for Bazel modulesbazel type for Bazel modules
See also [1]. [1]: package-url/purl-spec#317 Signed-off-by: Frank Viernau <[email protected]>
See also [1]. [1]: package-url/purl-spec#317 Signed-off-by: Frank Viernau <[email protected]>
|
@fmeum please rebase to resolve conflicts. |
|
@sschuberth Done |
See also [1]. [1]: package-url/purl-spec#317 Signed-off-by: Frank Viernau <[email protected]>
See also [1]. [1]: package-url/purl-spec#317 Signed-off-by: Frank Viernau <[email protected]>
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.
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.
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.
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.
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.
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
|
@stevespringett @shibumi @johnmhoran @pombredanne Apologies for the multi-ping, but it's hard to tell who would be able to move this forward. Could you add a second review? |
|
@fmeum Hi, I am "only" the packageurl-go maintainer and have not much to say when it comes to purl specification issues. I am afraid you will have to wait for @pombredanne or anyone else of the "steering committee". |
There was a problem hiding this comment.
Thanks. Looking good overall! ❤️ It would be great to add at a link to the spec for modules at https://bazel.build/external/module and I have posted a few small nits, suggestions and questions for you consideration.
PURL-TYPES.rst
Outdated
|
|
||
| - The default repository ("registry") is ``https://bcr.bazel.build``, the | ||
| Bazel Central Registry (BCR). | ||
| - The ``name`` is the module name. It must be lowercased. |
There was a problem hiding this comment.
Could it make sense to integrate the whole name spec at https://bazel.build/rules/lib/globals/module#module.name ?
A valid module name must: 1) only contain lowercase letters (a-z), digits (0-9), dots (.), hyphens (-), and underscores (_); 2) begin with a lowercase letter; 3) end with a lowercase letter or digit.
Also, about names, I guess that when there is name as possible in the spec, there is no purl possible, right?
The name of the module. Can be omitted only if this module is the root module (as in, if it's not going to be depended on by another module).
There was a problem hiding this comment.
Validating package names via the PURL spec makes PURL and its implementations more complicated and leads to a PURL spec crisis if the real validation rules are relaxed in the future (eg @a/b was invalid before NPM 2.0) where people will argue that it's a breaking change to allow something that was previously forbidden in PURL. I understand the value of canonicalizing the package name so multiple distinct PURLs don't refer to the same package, and that should be safe because changing canonicalization rules would break builds, but I don't think there's a need for PURL implementations in general to validate the name of the package to prevent PURLs that cannot refer to a package.
Are Bazel module names case insensitive, or are uppercase characters currently invalid? There's a difference, and some existing PURL types get this wrong (eg NPM).
There was a problem hiding this comment.
Bazel module names are enforced to be of the form @pombredanne linked. I agree that keeping validation out of the spec makes sense. We could just allow PURLs to reference module names that aren't valid as per the current rules enforced by Bazel, they just won't be useful.
There was a problem hiding this comment.
Having recently worked on implementing PURL in Starlark (Bazel's python dialect), my vote would be to keep "type"-specific validation and normalization to a minimum. That was the most complex part to implement. It also allows Bazel to change the allowed chars for module names without having to change the PURL spec, which I think would be a plus. We would allow PURLs that are not Bazel modules, but we can't really prevent that anyway because pkg:bazel/foo@1234 is definitely a valid PURL, but is not going to ever exist in BCR. Sure, pkg:bazel/[email protected] would point to a non-existing module, but PURL by itself can't prevent that given my example above. So why adding the additional complexity of doing the validation/normalization?
There was a problem hiding this comment.
I dropped validation from the type spec and the test suite, PTAL.
Added via b29e1ea |
Co-authored-by: Philippe Ombredanne <[email protected]>
Co-authored-by: Jan Kowalleck <[email protected]>
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
bazelpurl 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)