-
Notifications
You must be signed in to change notification settings - Fork 604
Add [email protected] #6600
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
base: main
Are you sure you want to change the base?
Add [email protected] #6600
Conversation
|
Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (fpm) have been updated in this PR. |
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.
Code Review
This pull request adds the fpm module at version 1.1.0 to the Bazel Central Registry. The overall structure and configuration align well with BCR practices, including the use of an overlay for this header-only C++ library and a comprehensive presubmit matrix. However, there are a couple of critical typos in the metadata.json file for the homepage and repository URLs that need to be corrected.
|
@bazel-io skip_check unstable_url |
| - ubuntu2204 | ||
| bazel: | ||
| - 7.x | ||
| - 8.x |
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.
maybe add - rolling to make sure the thing works with upcoming Bazel 9
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.
good point, thank's
| @@ -0,0 +1,8 @@ | |||
| cc_library( | |||
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.
you need to a load statement for cc_library (otherwise Bazel 9 will not work)
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.
run buildifier -lint=fix -r .
|
For what it's worth the name seems to conflict with https://github.com/jordansissel/fpm which is what I hoped this was 😅 |
|
I think that would be rules_fpm? So I assume fpm is fine here. Nevertheless, first come first served :) - and there is still the possibility of creating an own registry, overriding, prefixing with repo orga, alias magic, etc. - not sure what happens when two registry's are used that offer a module with the same name, but different contents and those are also used in transitive dependencies... |
I don't think that's the case. I know a name change has been requested for tiny libraries mirroring the wider known one and when I Google "fpm" the one I linked is the number one result. I leave it up to @meteorcloudy to decide though and remove myself from the conversation |
|
Some options for module names: |
|
Thank's both of you for the feedback and the input. I will rename it to |
|
Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (cpp_fpm) have been updated in this PR. |
|
To further bike-shed, could call it per:
Again, I personally wouldn't care if you went with |
Adds fpm version 1.1.0 (https://github.com/MikeLankamp/fpm) to BCR.