-
Notifications
You must be signed in to change notification settings - Fork 9
bazel: fix "cc_*" is not global anymore
#24
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?
bazel: fix "cc_*" is not global anymore
#24
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
rmaddikery
left a comment
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.
Lets discuss the review in Logging in CFT
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.
Lets please discuss the review in the Logging CFT since the changes in mw/log affects sync in reference integration
Can you please clarify what do you mean ? Generally, we shall not always wait to next CFT meeting to do something with PR but continue here until possible or do fast ad-hoc call ;) |
Our codebase includes sections synchronized with contributions to score through automated bidirectional sync. Changes to these areas require approval from original code authors to prevent sync failures. While we're improving this process, CODEOWNERS are not yet configured for these directories. Given this is a beta contribution, we suggest aligning on the approach in the CFT review before implementation. |
So You dont mean refernce_integration repo but copybara. ok lets align in CFT. |
rmaddikery
left a comment
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.
Can you explain what issue you encounter without the redundant load statements?
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
|
|
||
| load("@rules_cc//cc:defs.bzl", "cc_binary", "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.
AFAIU, this load statement is redundant since rules_cc is declared as a bazel_dep
Line 23 in a37a044
| bazel_dep(name = "rules_cc", version = "0.1.1") |
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.
Copying comment from @PiotrKorkus
Original PR: #21
@rmaddikery Please take a look into official C++ bazel example - https://github.com/bazelbuild/examples/blob/main/cpp-tutorial/stage1/main/BUILD
First versions of bazel did not require load statement for cc_binary / library but since 2019, there is a hint to use it as it will be a future incompatibility already.
Please not wait until deprecated, especially since those warnings increase noise level.
1caf212 to
00ba38a
Compare
00ba38a to
cdb88f5
Compare
Load required `cc_*` functions.
cdb88f5 to
ad7e65a
Compare
pawelrutkaq
left a comment
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.
e
Load required
cc_*functions.Notes for Reviewer
Pre-Review Checklist for the PR Author
Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References
Closes #