Skip to content

Commit e2bc1dd

Browse files
committed
refactor: extract constants and helpers for better maintainability
- Extract configuration constants (OS, CPU, version mappings) to module level - Create helper functions to eliminate code duplication (_normalize_cpu, _apply_sdp_version_mapping, _get_canonical_pkg_name, _apply_matrix_defaults) - Improve docstrings with complete type annotations and error conditions - Enhance error messages with supported values - Update documentation to reference new refactored helpers - Remove FIXME/TODO comments by implementing systematic solutions - No functional changes; fully backward compatible
1 parent 97957a7 commit e2bc1dd

3 files changed

Lines changed: 204 additions & 85 deletions

File tree

docs/generation_flow.md

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,14 +70,19 @@ Shared template:
7070

7171
- Some package definitions rely on the `%{toolchain_pkg}%` placeholder, which
7272
is rewritten to the canonical Bzlmod repository name during repository-rule
73-
generation.
73+
generation. This is handled by the `_get_canonical_pkg_name()` helper in
74+
`rules/gcc.bzl`.
7475
- QNX `aarch64` is mapped internally to `aarch64le` where required by the
75-
underlying SDK layout.
76-
- SDP version `8.0.4` is normalized to `8.0.0` in the generated toolchain
77-
configuration because platform constraint support currently uses the older
78-
identifier.
76+
underlying SDK layout. This CPU normalization is centralized in the
77+
`_normalize_cpu()` helper for consistency across both Linux and QNX paths.
78+
- SDP version mapping is handled through the `_SDP_VERSION_MAPPING` configuration
79+
in `rules/gcc.bzl`. Currently, SDP version `8.0.4` is mapped to `8.0.0` because
80+
platform constraint support uses the older identifier. This mapping is
81+
configurable for future extensibility.
7982
- Linux toolchains generate an extra `gcov_wrapper` script to work around the
80-
current `rules_cc` coverage integration behavior.
83+
current `rules_cc` coverage integration behavior. The gcov triple for each
84+
platform is constructed using `_TRIPLE_AARCH64_QNX_FMT` and `_TRIPLE_GENERIC_QNX_FMT`
85+
templates for consistency.
8186

8287
## Version Matrix Responsibilities
8388

extensions/gcc.bzl

Lines changed: 85 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,15 @@ load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
1818
load("@score_bazel_cpp_toolchains//packages:version_matrix.bzl", "VERSION_MATRIX")
1919
load("@score_bazel_cpp_toolchains//rules:gcc.bzl", "gcc_toolchain")
2020

21+
# Constants
22+
_PACKAGE_SUFFIX = "_pkg"
23+
_IDENTIFIER_GCC = "gcc"
24+
_IDENTIFIER_SDK = "sdk"
25+
_IDENTIFIER_SDP = "sdp"
26+
_SUPPORTED_CPUS = ["x86_64", "aarch64"]
27+
_SUPPORTED_OSS = ["linux", "qnx"]
28+
_SDP_VERSION_MAPPING = {"8.0.4": "8.0.0"}
29+
2130
# GCC interface API for archive tag class
2231
_attrs_sdp = {
2332
"build_file": attr.label(
@@ -113,18 +122,12 @@ _attrs_tc = {
113122
),
114123
"target_cpu": attr.string(
115124
mandatory = True,
116-
values = [
117-
"x86_64",
118-
"aarch64",
119-
],
125+
values = _SUPPORTED_CPUS,
120126
doc = "Target platform CPU",
121127
),
122128
"target_os": attr.string(
123129
mandatory = True,
124-
values = [
125-
"linux",
126-
"qnx",
127-
],
130+
values = _SUPPORTED_OSS,
128131
doc = "Target platform OS",
129132
),
130133
"use_default_package": attr.bool(
@@ -148,13 +151,13 @@ _attrs_tc = {
148151
}
149152

150153
def _get_packages(tags):
151-
"""Gets archive information from given tags.
154+
"""Converts archive tags to package dictionaries.
152155
153156
Args:
154-
tags: A list of tags containing archive information.
157+
tags: list of archive tag objects containing package information.
155158
156159
Returns:
157-
dict: A dictionary with archive information.
160+
list of dict: Each dict contains 'build_file', 'name', 'sha256', 'strip_prefix', 'url'.
158161
"""
159162
packages = []
160163
for tag in tags:
@@ -168,13 +171,13 @@ def _get_packages(tags):
168171
return packages
169172

170173
def _get_toolchains(tags):
171-
"""Gets toolchain information from given tags.
174+
"""Converts toolchain tags to toolchain configuration dictionaries.
172175
173176
Args:
174-
tags: A list of tags containing toolchain information.
177+
tags: list of toolchain tag objects containing toolchain configuration.
175178
176179
Returns:
177-
dict: A dictionary with toolchain information.
180+
list of dict: Each dict contains processed toolchain configuration with normalized keys.
178181
"""
179182
toolchains = []
180183
for tag in tags:
@@ -204,26 +207,49 @@ def _get_toolchains(tags):
204207
toolchains.append(toolchain)
205208
return toolchains
206209

210+
def _apply_matrix_defaults(toolchain_info, matrix):
211+
"""Applies default values from version matrix to toolchain configuration.
212+
213+
Args:
214+
toolchain_info: dict holding current toolchain information (modified in-place).
215+
matrix: dict containing default values from VERSION_MATRIX.
216+
"""
217+
if "extra_c_compile_flags" in matrix and not toolchain_info["tc_extra_c_compile_flags"]:
218+
toolchain_info["tc_extra_c_compile_flags"] = matrix["extra_c_compile_flags"]
219+
if "extra_cxx_compile_flags" in matrix and not toolchain_info["tc_extra_cxx_compile_flags"]:
220+
toolchain_info["tc_extra_cxx_compile_flags"] = matrix["extra_cxx_compile_flags"]
221+
if "extra_link_flags" in matrix and not toolchain_info["tc_extra_link_flags"]:
222+
toolchain_info["tc_extra_link_flags"] = matrix["extra_link_flags"]
223+
if "gcc_version" in matrix and not toolchain_info["gcc_version"]:
224+
toolchain_info["gcc_version"] = matrix["gcc_version"]
225+
if "compiler_library_search_paths" in matrix:
226+
toolchain_info["tc_compiler_library_search_paths"] = matrix["compiler_library_search_paths"]
227+
207228
def _create_and_link_sdp(toolchain_info):
208-
""" Create new archive based on information provided in extension.
229+
"""Creates archive package information from toolchain configuration using version matrix.
230+
231+
Resolves package identifiers, looks up version matrix entries, and applies defaults.
209232
210233
Args:
211-
toolchain_info: A dict of holding toolchain information.
234+
toolchain_info: dict holding toolchain information (modified in-place with sdp_to_link).
212235
213236
Returns:
214-
dict: A dict with archive information.
237+
dict: Archive information with 'build_file', 'name', 'sha256', 'strip_prefix', 'url'.
238+
239+
Fails:
240+
If the resolved matrix key is not found in VERSION_MATRIX.
215241
"""
216-
pkg_name = "{}_pkg".format(toolchain_info["name"])
242+
pkg_name = "{}{}".format(toolchain_info["name"], _PACKAGE_SUFFIX)
217243

218244
# Resolve package identifier from original version fields, not the
219245
# constraint-remapped tc_identifier. tc_identifier may differ from
220-
# the actual version (e.g., sdp 8.0.3 is remapped to 8.0.0 for
246+
# the actual version (e.g., sdp 8.0.4 is remapped to 8.0.0 for
221247
# platform constraint compatibility), but the version matrix uses
222248
# the real version.
223249
if toolchain_info["sdk_version"] != "":
224-
pkg_identifier = "sdk_{}".format(toolchain_info["sdk_version"])
250+
pkg_identifier = "{}_{}".format(_IDENTIFIER_SDK, toolchain_info["sdk_version"])
225251
elif toolchain_info["sdp_version"] != "":
226-
pkg_identifier = "sdp_{}".format(toolchain_info["sdp_version"])
252+
pkg_identifier = "{}_{}".format(_IDENTIFIER_SDP, toolchain_info["sdp_version"])
227253
else:
228254
pkg_identifier = toolchain_info["tc_identifier"]
229255

@@ -233,20 +259,17 @@ def _create_and_link_sdp(toolchain_info):
233259
identifier = "-{}".format(pkg_identifier) if pkg_identifier != "" else "",
234260
runtime_es = "-{}".format(toolchain_info["tc_runtime_ecosystem"]) if toolchain_info["tc_runtime_ecosystem"] != "" else "",
235261
)
262+
263+
# Validate and retrieve matrix entry
264+
if matrix_key not in VERSION_MATRIX:
265+
fail("Version matrix entry not found for key: {}. Available keys: {}".format(
266+
matrix_key,
267+
", ".join(sorted(VERSION_MATRIX.keys())),
268+
))
269+
236270
matrix = VERSION_MATRIX[matrix_key]
237271
toolchain_info["sdp_to_link"] = pkg_name
238-
239-
# TODO: Put this in separate function so that we can easy extend it (if needed).
240-
if "extra_c_compile_flags" in matrix and not toolchain_info["tc_extra_c_compile_flags"]:
241-
toolchain_info["tc_extra_c_compile_flags"] = matrix["extra_c_compile_flags"]
242-
if "extra_cxx_compile_flags" in matrix and not toolchain_info["tc_extra_cxx_compile_flags"]:
243-
toolchain_info["tc_extra_cxx_compile_flags"] = matrix["extra_cxx_compile_flags"]
244-
if "extra_link_flags" in matrix and not toolchain_info["tc_extra_link_flags"]:
245-
toolchain_info["tc_extra_link_flags"] = matrix["extra_link_flags"]
246-
if "gcc_version" in matrix and not toolchain_info["gcc_version"]:
247-
toolchain_info["gcc_version"] = matrix["gcc_version"]
248-
if "compiler_library_search_paths" in matrix:
249-
toolchain_info["tc_compiler_library_search_paths"] = matrix["compiler_library_search_paths"]
272+
_apply_matrix_defaults(toolchain_info, matrix)
250273

251274
return {
252275
"build_file": matrix["build_file"],
@@ -257,33 +280,47 @@ def _create_and_link_sdp(toolchain_info):
257280
}
258281

259282
def _resolve_identifier(toolchain_info):
260-
""" Create new archive based on information provided in extension.
283+
"""Resolves the toolchain identifier from version fields.
284+
285+
Determines which identifier type (gcc, sdk, or sdp) and version to use,
286+
applying any necessary version mappings.
261287
262288
Args:
263-
toolchain_info: A dict of holding toolchain information.
289+
toolchain_info: dict holding toolchain configuration with version fields.
264290
265291
Returns:
266-
dict: A dict with archive information.
292+
str: Identifier in format '{type}_{version}' (e.g., 'sdp_8.0.0', 'gcc_12.2.0').
267293
"""
268-
identifier = "gcc"
294+
identifier = _IDENTIFIER_GCC
269295
version = toolchain_info["gcc_version"]
296+
270297
if toolchain_info["sdk_version"] != "":
271-
identifier = "sdk"
298+
identifier = _IDENTIFIER_SDK
272299
version = toolchain_info["sdk_version"]
273300
elif toolchain_info["sdp_version"] != "":
274-
identifier = "sdp"
275-
version = "8.0.0" if toolchain_info["sdp_version"] == "8.0.4" else toolchain_info["sdp_version"] # FIXME: currently we do not support constraint "8.0.4".
301+
identifier = _IDENTIFIER_SDP
302+
version = toolchain_info["sdp_version"]
303+
304+
# Apply version mapping for constraint compatibility
305+
version = _SDP_VERSION_MAPPING.get(version, version)
276306

277307
return "{}_{}".format(identifier, version)
278308

279309
def _get_info(mctx):
280-
"""Gets raw info from module ctx about toolchain properties.
310+
"""Extracts and validates toolchain and package information from module configuration.
311+
312+
Ensures that only the root module uses this extension and processes all tags.
281313
282314
Args:
283-
mctx: A bazel object holding module information.
315+
mctx: ModuleContext object holding module information.
284316
285317
Returns:
286-
list: A list of dictionaries with toolchain and archive information.
318+
tuple: (toolchains, packages) where:
319+
- toolchains: list of dict with normalized toolchain configuration
320+
- packages: list of dict with archive information
321+
322+
Fails:
323+
If a non-root module attempts to use the gcc extension.
287324
"""
288325
root = None
289326
for mod in mctx.modules:
@@ -311,11 +348,13 @@ def _get_info(mctx):
311348
return toolchains, packages
312349

313350
def _impl(mctx):
314-
"""Extracts information about toolchain and instantiates nessesary rules for toolchain declaration.
351+
"""Implementation of the gcc module extension.
315352
316-
Args:
317-
mctx: A bazel object holding module information.
353+
Processes toolchain and package configurations, instantiates http_archive rules
354+
for dependencies, and creates gcc_toolchain rules with proper configuration.
318355
356+
Args:
357+
mctx: ModuleContext object holding module information.
319358
"""
320359
toolchains, archives = _get_info(mctx)
321360
for archive_info in archives:

0 commit comments

Comments
 (0)