Skip to content

Refactor repo rules into more cohesive files#11

Merged
reutermj merged 1 commit intomainfrom
refactor
Jun 29, 2025
Merged

Refactor repo rules into more cohesive files#11
reutermj merged 1 commit intomainfrom
refactor

Conversation

@reutermj
Copy link
Copy Markdown
Owner

No description provided.

Copilot AI review requested due to automatic review settings June 29, 2025 17:14
@reutermj reutermj enabled auto-merge (squash) June 29, 2025 17:14
@reutermj reutermj merged commit f26608b into main Jun 29, 2025
4 checks passed
@reutermj reutermj deleted the refactor branch June 29, 2025 17:15
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Refactors the C++ toolchain repository rules by extracting inline implementations into dedicated impl files and updating load statements.

  • Moved lazy download and eager declaration logic into impl/declare_tools.bzl and impl/declare_toolchain.bzl
  • Updated toolchains_cc.bzl to load new repository rules (lazy_download_bins, eager_declare_toolchain)
  • Cleaned up debug print and centralized configuration loading

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File Description
toolchains_cc.bzl Removed inline _lazy_download_bins_impl/_eager_declare_toolchain_impl, now loads from impl
impl/declare_tools.bzl Added _lazy_download_bins_impl and lazy_download_bins rule, removed debug print
impl/declare_toolchain.bzl Added _eager_declare_toolchain_impl and eager_declare_toolchain rule, loaded config
Comments suppressed due to low confidence (2)

impl/declare_tools.bzl:1

  • [nitpick] The directory and subdirectory symbols are loaded but not used in this file. Consider removing these unused imports to clean up the code.
load("@bazel_skylib//rules/directory:directory.bzl", "directory")

impl/declare_toolchain.bzl:108

  • The declare_toolchain macro implementation signature expects all_tools and target_triple parameters, but they are not defined in the attrs dictionary. Add all_tools (attr.label) and target_triple (attr.string) entries to avoid missing attribute errors.
declare_toolchain = macro(

Comment on lines +154 to +157
"toolchain_name": attr.string(
mandatory = True,
doc = "The name of the toolchain, used for registration.",
),
Copy link

Copilot AI Jun 29, 2025

Choose a reason for hiding this comment

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

[nitpick] The toolchain_name attribute declared on lazy_download_bins is never used within _lazy_download_bins_impl. Either remove this unused attribute or reference rctx.attr.toolchain_name in the implementation.

Suggested change
"toolchain_name": attr.string(
mandatory = True,
doc = "The name of the toolchain, used for registration.",
),

Copilot uses AI. Check for mistakes.
eager_declare_toolchain = repository_rule(
implementation = _eager_declare_toolchain_impl,
attrs = {
"toolchain_name": attr.string(
Copy link

Copilot AI Jun 29, 2025

Choose a reason for hiding this comment

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

[nitpick] The toolchain_name attribute in eager_declare_toolchain is never accessed in its implementation. Consider removing it or using rctx.attr.toolchain_name to avoid unused parameters.

Copilot uses AI. Check for mistakes.
Comment on lines +111 to +112
# a select(...) which cant be used when inlining the labels and strings in
# the toolchain declaraltions.
Copy link

Copilot AI Jun 29, 2025

Choose a reason for hiding this comment

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

There's a typo in the comment: 'declaraltions' should be 'declarations', and 'doesnt following' should be 'doesn't follow'.

Suggested change
# a select(...) which cant be used when inlining the labels and strings in
# the toolchain declaraltions.
# a select(...) which can't be used when inlining the labels and strings in
# the toolchain declarations.

Copilot uses AI. Check for mistakes.
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.

2 participants