Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a new CSV/sdkconfig-driven Relinker package and integrates it into the ESP-IDF PlatformIO build: prebuilds Ninja-generated sources, post-processes ldgen-generated Changes
Sequence DiagramsequenceDiagram
participant Env as Build Env
participant Ninja as Ninja
participant SCons as SCons
participant LDGen as ldgen (sections.ld)
participant Relinker as Relinker (Python)
participant Linker as Final Linker
Env->>Ninja: parse build.ninja for CUSTOM_COMMAND outputs
Env->>Ninja: run `ninja -C <build_dir> <missing_targets>`
Ninja-->>Env: generate missing sources
SCons->>LDGen: run ldgen -> produce `sections.ld`
SCons->>Relinker: if `custom_relinker_*` fully set, invoke relinker with `sections.ld`
Relinker->>Relinker: read CSVs + sdkconfig, run objdump(s), compute include/exclude sections
Relinker-->>Env: write `sections.ld.relinked` (stamp/target)
SCons->>Linker: final link depends on `sections.ld.relinked` when present
Linker-->>Env: produce final ELF/firmware
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
builder/relinker/configuration.py (5)
70-81: Improve None comparison style.Line 92 uses
!= Nonewhich should beis not Noneper Python style conventions. Additionally, consider usingis not Noneconsistently throughout.♻️ Suggested fix for line 92
def append(self, func): section = self.get_func_section(self.dumps, func) - if section != None: + if section is not None: self.funcs[func] = section🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/configuration.py` around lines 70 - 81, Replace any comparisons using "!= None" with "is not None" in get_func_section and nearby code to follow Python style; specifically change checks like "if m != None" to "if m is not None" and use "is not None" consistently for variables such as m and the result of regex groups, ensuring other None checks (e.g., espidf_missing_function_info) already use proper identity comparisons.
56-68: Debug print statement and resource management issues.
- Line 61: Debug
print('pathes:', pathes)should be removed or converted to logging for production code.- Line 57, 62, 87, 88: Typo
pathesshould bepaths.- Line 67: Use exception chaining with
raise ... from eto preserve the original traceback.♻️ Suggested improvements
class object_c: - def read_dump_info(self, pathes): + def read_dump_info(self, paths): new_env = os.environ.copy() new_env['LC_ALL'] = 'C' dumps = list() - print('pathes:', pathes) - for path in pathes: + for path in paths: try: dump = StringIO(subprocess.check_output([espidf_objdump, '-t', path], env=new_env).decode()) dumps.append(dump.readlines()) except subprocess.CalledProcessError as e: - raise RuntimeError('cmd:%s result:%s'%(e.cmd, e.returncode)) + raise RuntimeError('cmd:%s result:%s'%(e.cmd, e.returncode)) from e return dumps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/configuration.py` around lines 56 - 68, In object_c.read_dump_info replace the debug print and typo'd variable name: rename parameter and all uses of "pathes" to "paths", remove the plain print('pathes:', pathes) and log with the module logger at debug level instead (or remove entirely), and when re-raising the RuntimeError include exception chaining (use "raise RuntimeError(... ) from e") so the original CalledProcessError traceback is preserved; ensure you update the local variable names (e.g., for path in paths) and the error message construction in the except block to reference the corrected names.
83-88: Typo in variable and attribute names.
librayshould belibraryfor clarity and consistency.♻️ Suggested fix
- def __init__(self, name, pathes, libray): + def __init__(self, name, paths, library): self.name = name - self.libray = libray + self.library = library self.funcs = dict() - self.pathes = pathes - self.dumps = self.read_dump_info(pathes) + self.paths = paths + self.dumps = self.read_dump_info(paths)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/configuration.py` around lines 83 - 88, The constructor __init__ uses a misspelled parameter/attribute "libray" — rename the parameter and attribute to "library" (update the signature parameter 'libray' and all uses of self.libray to self.library) to keep naming consistent; ensure any other places in this class that reference the old symbol (e.g., other methods or initializations) are updated accordingly so there are no unresolved references to "libray".
160-180: Use context managers for CSV file operations and fix comparison style.Multiple files are opened without context managers, and line 172 uses
== Falseinstead of the idiomaticnotoperator.♻️ Suggested improvements
sdkconfig = sdkconfig_c(sdkconfig_file) lib_paths = paths_c() - for p in csv.DictReader(open(library_file, 'r')): - lib_paths.append(p['library'], '*', p['path']) + with open(library_file, 'r', encoding='utf-8') as f: + for p in csv.DictReader(f): + lib_paths.append(p['library'], '*', p['path']) obj_paths = paths_c() - for p in csv.DictReader(open(object_file, 'r')): - obj_paths.append(p['library'], p['object'], p['path']) + with open(object_file, 'r', encoding='utf-8') as f: + for p in csv.DictReader(f): + obj_paths.append(p['library'], p['object'], p['path']) libraries = libraries_c() - for d in csv.DictReader(open(function_file, 'r')): - if d['option'] and sdkconfig.check(d['option']) == False: + with open(function_file, 'r', encoding='utf-8') as f: + for d in csv.DictReader(f): + if d['option'] and not sdkconfig.check(d['option']):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/configuration.py` around lines 160 - 180, Replace plain open(...) calls with context managers and fix the boolean comparison: open library_file, object_file, and function_file using with open(..., 'r') as f and pass f into csv.DictReader to ensure files are closed (change both loops using csv.DictReader(open(..., 'r')) to with blocks). Also change the condition if d['option'] and sdkconfig.check(d['option']) == False: to use the idiomatic check if d['option'] and not sdkconfig.check(d['option']):; keep existing uses of sdkconfig_c, paths_c, libraries_c, lib_paths.append, obj_paths.append, libraries.append, lib_paths.index and obj_paths.index when locating paths.
20-29: Use context manager for file operations.Opening files without a context manager can lead to resource leaks if an exception occurs before the file is closed.
♻️ Suggested fix
class sdkconfig_c: def __init__(self, path): - lines = open(path).read().splitlines() + with open(path, encoding='utf-8') as f: + lines = f.read().splitlines() config = dict() for l in lines:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/configuration.py` around lines 20 - 29, The __init__ of class sdkconfig_c currently opens the file with open(path).read() without a context manager; change it to use a with-statement (e.g., with open(path, 'r', encoding='utf-8') as f: lines = f.read().splitlines()) so the file is always closed even on exceptions, then proceed to parse lines into the config dict as before (references: class sdkconfig_c, __init__, path, lines, config).builder/frameworks/espidf.py (2)
1424-1472: Relinker integration looks good with one minor resource concern.The relinker integration is well-structured. However, the lambda at lines 1465-1467 opens a file but relies on garbage collection to close it.
♻️ Suggested improvement for proper file handling
relinker_step = env.Command( str(Path("$BUILD_DIR") / "sections.ld.relinked"), str(Path("$BUILD_DIR") / "sections.ld"), [ env.VerboseAction(_relinker_cmd, "Running relinker to optimize IRAM usage"), # Touch a stamp file so SCons tracks the dependency env.VerboseAction( - lambda target, source, env: open(str(target[0]), 'w').write('done'), + lambda target, source, env: Path(str(target[0])).write_text('done'), "" ), ], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/frameworks/espidf.py` around lines 1424 - 1472, The lambda used in the env.VerboseAction for relinker_step writes to the stamp file without closing it; replace that inline lambda with a small named action/function (e.g., write_relinker_stamp) that takes (target, source, env) and uses a with open(str(target[0]), 'w') as f: f.write('done') pattern to ensure the file is closed, then pass that function to env.VerboseAction instead of the lambda so relinker_step (sections.ld.relinked) reliably creates a closed stamp file.
2531-2536: Relinker ELF dependency logic is correct but could use a comment.The condition checks both file existence and config option because:
- On subsequent builds, the stamp file will exist
- On first build, we need the config option to create the dependency
This is correct behavior but might be confusing to future maintainers.
📝 Suggested documentation improvement
# If relinker is configured, ensure the ELF depends on the relinked stamp +# Check both: (1) stamp exists from previous build, or (2) config option is set for first build _relinker_stamp = str(Path(BUILD_DIR) / "sections.ld.relinked") if os.path.exists(_relinker_stamp) or config.get( "env:" + env["PIOENV"], "custom_relinker_function", "" ): env.Depends("$BUILD_DIR/$PROGNAME$PROGSUFFIX", _relinker_stamp)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/frameworks/espidf.py` around lines 2531 - 2536, Add a short clarifying comment above the relinker dependency check explaining why both the stamp file existence and the config option are evaluated: on subsequent builds the stamp file (_relinker_stamp) will exist and should create the Depends, while on the very first build the stamp won’t exist so we must check the config key ("env:" + env["PIOENV"], "custom_relinker_function") to ensure the relinker dependency is registered; keep the existing logic using BUILD_DIR, env and env.Depends("$BUILD_DIR/$PROGNAME$PROGSUFFIX", _relinker_stamp) unchanged.builder/relinker/relinker.py (4)
127-128: Parameter name shadows Python builtin.The parameter
inputshadows the Python builtin function. Consider renaming toinput_fileorlinker_inputfor clarity.♻️ Suggested fix
class relink_c: - def __init__(self, input, library_file, object_file, function_file, sdkconfig_file, missing_function_info): - self.filter = filter_c(input) + def __init__(self, input_file, library_file, object_file, function_file, sdkconfig_file, missing_function_info): + self.filter = filter_c(input_file)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/relinker.py` around lines 127 - 128, The __init__ of class relink_c currently defines a parameter named input which shadows Python's builtin; rename that parameter (for example to input_file or linker_input) throughout the relink_c.__init__ signature and all uses inside the class (constructor body and any assignments or references) to avoid builtin shadowing and improve clarity; ensure any external callers that instantiate relink_c are updated to pass the new parameter name if keyword args are used.
249-252: Use context managers and avoid shadowing builtins.The
savemethod opens files without context managers and usesinputas a parameter name.♻️ Suggested fix
- def save(self, input, output): - lines = open(input).read().splitlines() + def save(self, input_file, output_file): + with open(input_file, encoding='utf-8') as f: + lines = f.read().splitlines() lines = self.__replace__(lines) - open(output, 'w+').write('\n'.join(lines)) + with open(output_file, 'w', encoding='utf-8') as f: + f.write('\n'.join(lines))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/relinker.py` around lines 249 - 252, In save, avoid shadowing the builtin and ensure files are opened with context managers: rename the parameters (e.g., input_path, output_path) and use with open(input_path, 'r', encoding='utf-8') as f: lines = f.read().splitlines(), then lines = self.__replace__(lines), and write with with open(output_path, 'w', encoding='utf-8') as f: f.write('\n'.join(lines)); update any callers if needed.
206-247: Inconsistent return type in_replace_func.The method returns either a modified string,
' '(space string), orFalse. ReturningFalsefrom a function that otherwise returns strings is confusing and can lead to bugs. Consider returningNoneinstead.♻️ Suggested fix
- return False + return NoneAnd update the caller at line 203-204:
new_l = self._replace_func(l) - if new_l: + if new_l is not None: lines[i] = new_l🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/relinker.py` around lines 206 - 247, _replace_func currently returns strings, a space ' ', or the boolean False; change it to return None instead of False so its return type is either str or None. Update all call sites that test the result of _replace_func (the caller in the same module that currently checks for False) to treat None as "no replacement" (use "is None" or "if result is not None") and avoid relying on truthiness; keep the existing behavior for returning ' ' and other strings unchanged.
84-86: Use context manager for file operations.Opening files without a context manager can lead to resource leaks.
♻️ Suggested fix
class filter_c: def __init__(self, file): - lines = open(file).read().splitlines() + with open(file, encoding='utf-8') as f: + lines = f.read().splitlines() self.libs_desc = ''🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/relinker.py` around lines 84 - 86, The constructor of class filter_c currently opens the file with open(file) without a context manager (see filter_c.__init__), which can leak file descriptors; change it to use a with statement: open the file with "with open(file, 'r', encoding='utf-8') as f:" (or appropriate encoding), read lines via f.read().splitlines() (or f.readlines() if preferred) and assign to lines so the file is automatically closed even on exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/relinker/configuration.py`:
- Around line 205-208: The generator() call in main() is missing the required
missing_function_info parameter; update the call to pass the CLI value (e.g.,
args.missing_function_info) when invoking generator(args.library, args.object,
args.function, args.sdkconfig) and ensure the ArgumentParser defines/uses the
same argument name so missing_function_info is available on args before calling
generator().
In `@builder/relinker/relinker.py`:
- Around line 89-94: The regex string assigned to desc uses backslashes in a
normal string which creates invalid escape sequences; change desc to a raw
string (e.g. desc = r'\(EXCLUDE_FILE\((.*)\) .iram1 ') to avoid escape warnings
and ensure the pattern is interpreted literally, then keep the existing
re.search(desc, l) usage and assignment to self.libs_desc/self.libs unchanged
(optionally check the match is not None before indexing).
- Around line 121-125: In the __str__ method of the ReLinker class, fix the
invalid escape sequence `\l` inside the format string (currently in the string
assigned to s) by replacing it with the intended newline escape `\n` (or use a
multi-line string or join to build the output) so the format becomes valid and
prints each field on its own line; ensure the format string still references
self.lib, self.file, self.lib_path, self.desc, self.secs, self.fsecs, and
self.isecs as before.
- Around line 298-302: The CLI flag defined with
argparser.add_argument('--missing_function_info', default=False, type=bool) is
broken because argparse with type=bool treats string values incorrectly; change
the argument to use action='store_true' (so presence sets it True, default stays
False) for the '--missing_function_info' option in the argparser.add_argument
call (or alternatively implement a custom type/parser), and update any code that
reads the missing_function_info variable to expect a boolean produced by the new
action.
---
Nitpick comments:
In `@builder/frameworks/espidf.py`:
- Around line 1424-1472: The lambda used in the env.VerboseAction for
relinker_step writes to the stamp file without closing it; replace that inline
lambda with a small named action/function (e.g., write_relinker_stamp) that
takes (target, source, env) and uses a with open(str(target[0]), 'w') as f:
f.write('done') pattern to ensure the file is closed, then pass that function to
env.VerboseAction instead of the lambda so relinker_step (sections.ld.relinked)
reliably creates a closed stamp file.
- Around line 2531-2536: Add a short clarifying comment above the relinker
dependency check explaining why both the stamp file existence and the config
option are evaluated: on subsequent builds the stamp file (_relinker_stamp) will
exist and should create the Depends, while on the very first build the stamp
won’t exist so we must check the config key ("env:" + env["PIOENV"],
"custom_relinker_function") to ensure the relinker dependency is registered;
keep the existing logic using BUILD_DIR, env and
env.Depends("$BUILD_DIR/$PROGNAME$PROGSUFFIX", _relinker_stamp) unchanged.
In `@builder/relinker/configuration.py`:
- Around line 70-81: Replace any comparisons using "!= None" with "is not None"
in get_func_section and nearby code to follow Python style; specifically change
checks like "if m != None" to "if m is not None" and use "is not None"
consistently for variables such as m and the result of regex groups, ensuring
other None checks (e.g., espidf_missing_function_info) already use proper
identity comparisons.
- Around line 56-68: In object_c.read_dump_info replace the debug print and
typo'd variable name: rename parameter and all uses of "pathes" to "paths",
remove the plain print('pathes:', pathes) and log with the module logger at
debug level instead (or remove entirely), and when re-raising the RuntimeError
include exception chaining (use "raise RuntimeError(... ) from e") so the
original CalledProcessError traceback is preserved; ensure you update the local
variable names (e.g., for path in paths) and the error message construction in
the except block to reference the corrected names.
- Around line 83-88: The constructor __init__ uses a misspelled
parameter/attribute "libray" — rename the parameter and attribute to "library"
(update the signature parameter 'libray' and all uses of self.libray to
self.library) to keep naming consistent; ensure any other places in this class
that reference the old symbol (e.g., other methods or initializations) are
updated accordingly so there are no unresolved references to "libray".
- Around line 160-180: Replace plain open(...) calls with context managers and
fix the boolean comparison: open library_file, object_file, and function_file
using with open(..., 'r') as f and pass f into csv.DictReader to ensure files
are closed (change both loops using csv.DictReader(open(..., 'r')) to with
blocks). Also change the condition if d['option'] and
sdkconfig.check(d['option']) == False: to use the idiomatic check if d['option']
and not sdkconfig.check(d['option']):; keep existing uses of sdkconfig_c,
paths_c, libraries_c, lib_paths.append, obj_paths.append, libraries.append,
lib_paths.index and obj_paths.index when locating paths.
- Around line 20-29: The __init__ of class sdkconfig_c currently opens the file
with open(path).read() without a context manager; change it to use a
with-statement (e.g., with open(path, 'r', encoding='utf-8') as f: lines =
f.read().splitlines()) so the file is always closed even on exceptions, then
proceed to parse lines into the config dict as before (references: class
sdkconfig_c, __init__, path, lines, config).
In `@builder/relinker/relinker.py`:
- Around line 127-128: The __init__ of class relink_c currently defines a
parameter named input which shadows Python's builtin; rename that parameter (for
example to input_file or linker_input) throughout the relink_c.__init__
signature and all uses inside the class (constructor body and any assignments or
references) to avoid builtin shadowing and improve clarity; ensure any external
callers that instantiate relink_c are updated to pass the new parameter name if
keyword args are used.
- Around line 249-252: In save, avoid shadowing the builtin and ensure files are
opened with context managers: rename the parameters (e.g., input_path,
output_path) and use with open(input_path, 'r', encoding='utf-8') as f: lines =
f.read().splitlines(), then lines = self.__replace__(lines), and write with with
open(output_path, 'w', encoding='utf-8') as f: f.write('\n'.join(lines)); update
any callers if needed.
- Around line 206-247: _replace_func currently returns strings, a space ' ', or
the boolean False; change it to return None instead of False so its return type
is either str or None. Update all call sites that test the result of
_replace_func (the caller in the same module that currently checks for False) to
treat None as "no replacement" (use "is None" or "if result is not None") and
avoid relying on truthiness; keep the existing behavior for returning ' ' and
other strings unchanged.
- Around line 84-86: The constructor of class filter_c currently opens the file
with open(file) without a context manager (see filter_c.__init__), which can
leak file descriptors; change it to use a with statement: open the file with
"with open(file, 'r', encoding='utf-8') as f:" (or appropriate encoding), read
lines via f.read().splitlines() (or f.readlines() if preferred) and assign to
lines so the file is automatically closed even on exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f69ea903-f276-44ce-8c5f-a8d76d2c4567
⛔ Files ignored due to path filters (3)
builder/relinker/examples/esp32c2/function.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/library.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/object.csvis excluded by!**/*.csv
📒 Files selected for processing (4)
builder/frameworks/espidf.pybuilder/relinker/__init__.pybuilder/relinker/configuration.pybuilder/relinker/relinker.py
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/frameworks/espidf.py`:
- Around line 2532-2538: The dependency on the relinker stamp (_relinker_stamp)
is being added whenever only custom_relinker_function is set, but
generate_project_ld_script() only creates sections.ld.relinked when all relinker
options are provided; update the conditional around
env.Depends("$BUILD_DIR/$PROGNAME$PROGSUFFIX", _relinker_stamp) to mirror
generate_project_ld_script() by requiring either the stamp file exists OR all
three config keys ("custom_relinker_function", "custom_relinker_library",
"custom_relinker_object") are truthy via config.get; this ensures the stamp
dependency is only added when the relinker step is actually configured (use the
same config.get(...) checks and _relinker_stamp variable).
- Around line 1431-1472: The relinker action (relinker_step) only lists
sections.ld as its source, so changes to the relinker script, configuration
files, the three CSV inputs (library/object/function), SDKCONFIG_PATH or any
files under FRAMEWORK_DIR do not trigger rebuilds; update the env.Command call
that creates relinker_step to include the actual file dependencies: the relinker
script (_relinker_script), the relinker module(s) (e.g., configuration.py), the
three CSV files referenced by relinker_library/relinker_object/relinker_function
(resolve their paths and pass them as additional source entries), and
SDKCONFIG_PATH (and any FRAMEWORK_DIR files the relinker reads), or
alternatively call env.Depends(relinker_step, [list_of_these_paths]) so SCons
knows to rebuild sections.ld.relinked when any of those files change.
In `@builder/relinker/configuration.py`:
- Around line 148-153: The index method currently raises KeyError when a library
exists in self.paths but the specific object key is missing; change index (in
builder/relinker/configuration.py) to explicitly check for the object key after
handling the '*' wildcard and return None when obj is not present (i.e., if lib
in self.paths and '*' not in self.paths[lib] and obj not in self.paths[lib] then
return None) so callers can fall back to the library-level path instead of
aborting.
In `@builder/relinker/relinker.py`:
- Around line 21-35: The top-level import "from entity import EntityDB" runs
before CLI parsing so --idf-path can't affect import resolution; move that
import (and any other imports that rely on IDF_PATH) to after parsing in
main()/run_relinker(), call _setup_ldgen_imports(idf_path) with the parsed
idf_path before importing, and set os.environ['IDF_PATH'] = idf_path (do not use
setdefault) so configuration reads the explicit value; update
run_relinker()/main() to pass the parsed idf_path into _setup_ldgen_imports and
only then import EntityDB.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 507a3557-3a40-434f-b951-96957434e570
⛔ Files ignored due to path filters (3)
builder/relinker/examples/esp32c2/function.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/library.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/object.csvis excluded by!**/*.csv
📒 Files selected for processing (4)
builder/frameworks/espidf.pybuilder/relinker/__init__.pybuilder/relinker/configuration.pybuilder/relinker/relinker.py
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
builder/frameworks/espidf.py (1)
1595-1623: Skip already-generated outputs before spawning Ninja.
compile_source_files()calls this helper per component, but this loop adds every matching generated target even whenabs_pathalready exists. That keeps incremental builds paying a separateninja -C ...startup cost for no-op targets.♻️ Proposed fix
for source in generated_sources: src_path = source["path"] if not os.path.isabs(src_path): abs_path = str(Path(project_src_dir) / src_path) else: abs_path = src_path + if os.path.exists(abs_path): + continue # Ninja targets are relative to build_dir, not project_src_dir try: ninja_target = fs.to_unix_path( str(Path(abs_path).resolve().relative_to(Path(build_dir).resolve())) ).lstrip("./")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/frameworks/espidf.py` around lines 1595 - 1623, The loop collects generated targets even when the generated output file already exists, causing unnecessary ninja invocations; in the loop inside compile_source_files() that iterates generated_sources (and builds ninja_target and appends to generated_targets), check whether the resolved abs_path (or its expected generated output) already exists on disk (e.g., Path(abs_path).exists()) and skip adding that target to generated_targets if so; leave the rest (ninja_custom_targets check, populate_idf_env_vars, exec_command) unchanged so only missing outputs are passed to exec_command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/frameworks/espidf.py`:
- Around line 1439-1449: The command string in builder/frameworks/espidf.py
unconditionally appends the relinker flag --missing_function_info (see
_relinker_cmd) which forces "print-and-drop" behavior; change it so the flag is
only added when an explicit opt-in is set (e.g., a new boolean config/option
like relinker_allow_missing_function_info or via existing relinker config), and
consult builder/relinker/configuration.py to honor that setting; update the code
that constructs _relinker_cmd to conditionally append '--missing_function_info'
only if that opt-in flag is true, and ensure tests or callers pass the
new/configured option.
In `@builder/relinker/relinker.py`:
- Around line 204-208: In relinker.py, when you detect '(.stub .gnu.warning' and
insert the flash block into lines[i], set flash_done = True inside that
insertion branch (immediately after the lines[i] = '%s\n\n%s' %
(self.flash_include, l) assignment) so subsequent iterations see that flash has
been handled; this ensures the later elif self.flash_include in l check doesn't
cause duplicate inserts when ldgen emits multiple '(.stub .gnu.warning' markers.
In `@RELINKER_INTEGRATION.md`:
- Around line 18-33: The unlabeled fenced code blocks in RELINKER_INTEGRATION.md
(e.g., the ASCII diagram block starting with the box diagram containing
"sections.ld → ldgen → sections.ld → relinker" and the other blocks around the
comment references) should have explicit language identifiers to satisfy
markdownlint; update each triple-backtick fence to include a language token such
as text, bash, or ini (for example, change ``` to ```text) for the blocks at the
noted locations so the file is lint-clean while keeping the block contents
unchanged.
- Around line 35-39: The docs currently describe function.csv backwards:
function.csv lists functions that are relocated to flash (not kept in IRAM);
builder/relinker/configuration.py only retains rows whose option evaluates to
true and builder/relinker/relinker.py writes those selected rows into
flash_include, so update the text to state that included/true rows go to flash
and omitted/false rows remain in IRAM, and correct the option table meanings for
CONFIG_*, !CONFIG_*, and FALSE accordingly (also apply the same fix in the other
two affected sections around lines 156–165 and 190–197).
---
Nitpick comments:
In `@builder/frameworks/espidf.py`:
- Around line 1595-1623: The loop collects generated targets even when the
generated output file already exists, causing unnecessary ninja invocations; in
the loop inside compile_source_files() that iterates generated_sources (and
builds ninja_target and appends to generated_targets), check whether the
resolved abs_path (or its expected generated output) already exists on disk
(e.g., Path(abs_path).exists()) and skip adding that target to generated_targets
if so; leave the rest (ninja_custom_targets check, populate_idf_env_vars,
exec_command) unchanged so only missing outputs are passed to exec_command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e2eabf5-8a4c-45b4-b0c8-65af02a0fa53
⛔ Files ignored due to path filters (3)
builder/relinker/examples/esp32c2/function.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/library.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/object.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
RELINKER_INTEGRATION.mdbuilder/frameworks/espidf.pybuilder/relinker/__init__.pybuilder/relinker/configuration.pybuilder/relinker/relinker.py
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
builder/frameworks/espidf.py (1)
1439-1462:⚠️ Potential issue | 🟠 MajorKeep missing-symbol tolerance opt-in.
builder/relinker/configuration.pydrops rows when section lookup fails in this mode, so defaulting this flag to"yes"lets typos or IDF drift silently produce a partially relinkedsections.ld. Please default it to off and only append--missing_function_infowhen the user explicitly enables it.🛠️ Proposed fix
_relinker_missing = config.get( - "env:" + env["PIOENV"], "custom_relinker_missing_function_info", "yes" + "env:" + env["PIOENV"], "custom_relinker_missing_function_info", "no" ).strip().lower() in ("yes", "true", "1")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/frameworks/espidf.py` around lines 1439 - 1462, The code currently defaults _relinker_missing to true by using config.get(..., "yes"); change the default to off so missing-symbol tolerance is opt-in: update the config.get call for "custom_relinker_missing_function_info" to default to "no"/"false"/"0" (or otherwise parse absence as False) and keep the existing logic that only appends ' --missing_function_info' to _relinker_cmd when _relinker_missing is True; modify the _relinker_missing assignment (the variable and the config key "custom_relinker_missing_function_info") so typos/IDF drift do not silently enable the flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@RELINKER_INTEGRATION.md`:
- Around line 277-288: The guidance is too broad—change the ISR wording to match
the earlier minimal examples by limiting "must stay in IRAM" to code paths that
run with flash cache disabled or that are explicitly IRAM-safe (e.g., early-boot
handlers, flash callbacks like spi_flash_guard_* / spi_flash_os_*, cache error
handlers, RTC/sleep callbacks, and functions marked IRAM_ATTR) and avoid blanket
statements that every function "called from ISR" must be in IRAM; explicitly
call out exceptions such as FreeRTOS ISR helpers that are IRAM-safe (e.g.,
xQueueGiveFromISR) and add a short sentence instructing authors to verify
whether their interrupt runs with flash cache disabled before moving functions
to flash.
---
Duplicate comments:
In `@builder/frameworks/espidf.py`:
- Around line 1439-1462: The code currently defaults _relinker_missing to true
by using config.get(..., "yes"); change the default to off so missing-symbol
tolerance is opt-in: update the config.get call for
"custom_relinker_missing_function_info" to default to "no"/"false"/"0" (or
otherwise parse absence as False) and keep the existing logic that only appends
' --missing_function_info' to _relinker_cmd when _relinker_missing is True;
modify the _relinker_missing assignment (the variable and the config key
"custom_relinker_missing_function_info") so typos/IDF drift do not silently
enable the flag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07b916cc-78c0-49ee-9bba-3e42d3c76c0e
⛔ Files ignored due to path filters (3)
builder/relinker/examples/esp32c2/function.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/library.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/object.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
RELINKER_INTEGRATION.mdbuilder/frameworks/espidf.pybuilder/relinker/__init__.pybuilder/relinker/configuration.pybuilder/relinker/relinker.py
Added build_dir parameter to paths_c and generator functions to support custom build directories.
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
builder/relinker/configuration.py (1)
153-160:⚠️ Potential issue | 🟠 MajorPrefer the explicit object mapping before falling back to
*.If a library has both an object-specific path and a wildcard path, this method always rewrites the lookup to
*, so the override is unreachable. That makes mixed wildcard/override CSVs resolve the wrong file.🐛 Proposed fix
def index(self, lib, obj): if lib not in self.paths: return None - if '*' in self.paths[lib]: - obj = '*' - if obj not in self.paths[lib]: - return None - return self.paths[lib][obj] + if obj in self.paths[lib]: + return self.paths[lib][obj] + if '*' in self.paths[lib]: + return self.paths[lib]['*'] + return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/configuration.py` around lines 153 - 160, The index method currently prefers the wildcard mapping ('*') before checking for an explicit object mapping, causing explicit overrides to be ignored; change the lookup order in index(lib, obj) to first check if lib exists in self.paths, then if obj is explicitly present in self.paths[lib] and return that, and only if the explicit obj is missing fall back to checking for '*' in self.paths[lib] and return the wildcard entry; ensure the existing None returns remain for missing lib or missing both explicit and wildcard entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/frameworks/espidf.py`:
- Around line 1599-1629: In _ensure_generated_sources(), stop skipping generated
sources that already exist: remove the existence check (the if
os.path.exists(abs_path): continue) so that all entries from generated_sources
that map to ninja_custom_targets are appended to generated_targets every run;
this ensures all ninja targets in all_targets are passed to exec_command (the
ninja invocation) so Ninja can decide whether to rebuild based on its inputs/dep
tracking rather than SCons short-circuiting. Keep the surrounding logic that
computes ninja_target and checks ninja_custom_targets and ensure
generated_targets is still used to build all_targets for exec_command.
In `@builder/relinker/relinker.py`:
- Around line 346-348: The run_relinker function's default for
missing_function_info (currently True) causes silent dropping of missing symbols
and diverges from the CLI's fail-fast behavior; change the default parameter in
run_relinker(...) from missing_function_info=True to missing_function_info=False
so the API fails fast like main(), and verify callers that relied on the old
default explicitly pass True if they need the prior silent behavior (look for
references to run_relinker to update callers as needed).
- Around line 137-140: The call to configuration.generator in relinker.__init__
omits the real build directory so relative CSV paths are resolved incorrectly;
update relinker.__init__ (constructor) to accept a build_dir argument (or obtain
the real build dir from the caller/context) and pass that build_dir into
configuration.generator when calling it (preserving existing params:
library_file, object_file, function_file, sdkconfig_file, missing_function_info,
espidf_objdump), and then update any instantiations of the Relinker class to
supply the real build_dir so configuration.generator can resolve CSV-relative
paths against BUILD_DIR.
In `@RELINKER_INTEGRATION.md`:
- Around line 47-59: The example uses POSIX-only paths and shell commands;
update RELINKER_INTEGRATION.md to be OS-neutral by either (A) prefixing the
existing snippet with a clear "POSIX (Linux/macOS) example" label, or (B) adding
a Windows PowerShell alternative that uses the user's profile path (e.g.,
%USERPROFILE% or $env:USERPROFILE) and the PowerShell equivalents of creating a
directory and copying files (New-Item / Copy-Item), and explicitly reference the
same platformio example directory under the user's platform folder so Windows
users know where to copy from; ensure both options mention the relinker/ target
directory and the example source path.
---
Duplicate comments:
In `@builder/relinker/configuration.py`:
- Around line 153-160: The index method currently prefers the wildcard mapping
('*') before checking for an explicit object mapping, causing explicit overrides
to be ignored; change the lookup order in index(lib, obj) to first check if lib
exists in self.paths, then if obj is explicitly present in self.paths[lib] and
return that, and only if the explicit obj is missing fall back to checking for
'*' in self.paths[lib] and return the wildcard entry; ensure the existing None
returns remain for missing lib or missing both explicit and wildcard entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43110e62-d887-423c-89f9-419cfd5f377d
⛔ Files ignored due to path filters (3)
builder/relinker/examples/esp32c2/function.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/library.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/object.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
RELINKER_INTEGRATION.mdbuilder/frameworks/espidf.pybuilder/relinker/__init__.pybuilder/relinker/configuration.pybuilder/relinker/relinker.py
Remove check for existing absolute path before appending generated targets.
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
test/TESTING.md (1)
224-227:⚠️ Potential issue | 🟡 MinorCleanup instructions don’t match the temp directory pattern shown in the same doc.
tempfile.mkdtemp()at Line 226 typically creates/tmp/tmp*, but Line 333 suggests/tmp/tmp*relinker*, which usually won’t match anything. Either use a dedicated prefix in examples or adjust cleanup guidance to exact printed temp paths.🛠️ Suggested doc alignment
- self.temp_dir = tempfile.mkdtemp() + self.temp_dir = tempfile.mkdtemp(prefix="relinker_test_") ... -rm -rf /tmp/tmp*relinker* +rm -rf /tmp/relinker_test_*Also applies to: 325-334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/TESTING.md` around lines 224 - 227, The cleanup guidance and example temp paths don't match how the temp dir is created in setUp: tempfile.mkdtemp() produces names like /tmp/tmp* but the doc's cleanup examples expect /tmp/tmp*relinker*; to fix, either create the temp dir with a deterministic prefix (e.g., call tempfile.mkdtemp(prefix="relinker_") where setUp assigns self.temp_dir) or update the cleanup instructions/examples to reference the actual pattern produced by tempfile.mkdtemp() (e.g., /tmp/tmp* or use self.temp_dir directly) so the documented cleanup matches the code.test/test_relinker_functionality.py (1)
54-92:⚠️ Potential issue | 🟡 MinorRestore
IDF_PATHfrom the outer cleanup.Any failure before the inner
finallyleaks the modified process environment into later tests. Move the restore alongsideshutil.rmtree()so it always runs.♻️ Safer cleanup
def test_path_normalization(self): """Test path normalization and resolution.""" temp_dir = tempfile.mkdtemp() + original_idf = os.environ.get('IDF_PATH') try: - # Set IDF_PATH - original_idf = os.environ.get('IDF_PATH') os.environ['IDF_PATH'] = '/test/esp-idf' @@ - finally: - # Restore IDF_PATH - if original_idf: - os.environ['IDF_PATH'] = original_idf finally: + if original_idf is not None: + os.environ['IDF_PATH'] = original_idf + else: + os.environ.pop('IDF_PATH', None) shutil.rmtree(temp_dir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_relinker_functionality.py` around lines 54 - 92, The test modifies the environment variable IDF_PATH inside test_path_normalization but only restores it in an inner finally block, which can leak into other tests; move the restoration of the original_idf so it always executes in the outer finally (the same block that calls shutil.rmtree(temp_dir)) to guarantee IDF_PATH is restored even if an earlier assertion or exception occurs; update the cleanup logic in test_path_normalization to set os.environ['IDF_PATH'] back to original_idf (or delete it if original_idf was None) inside the outer finally that runs after tempfile.mkdtemp()/shutil.rmtree.test/test_configuration.py (1)
209-217:⚠️ Potential issue | 🟡 MinorPin
espidf_missing_function_infoinside this test.
object_c.append()only returnsFalsein warning-only mode, andconfiguration.generator()mutates that module-global. If another test flips it toFalsefirst, this starts raisingRuntimeErrorinstead of returningFalse.🧪 Stabilize the test
- with mock.patch.object(object_c, 'read_dump_info', return_value=[['mock dump data']]), \ + with mock.patch('configuration.espidf_missing_function_info', True), \ + mock.patch.object(object_c, 'read_dump_info', return_value=[['mock dump data']]), \ mock.patch.object(object_c, 'get_func_section', return_value=None): obj = object_c('test.c.obj', [], 'libtest.a') result = obj.append('nonexistent_function')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_configuration.py` around lines 209 - 217, The test can be flaky because object_c.append() behavior depends on the module-global espidf_missing_function_info mutated by configuration.generator(); to stabilize, explicitly pin espidf_missing_function_info to the warning-only value inside the test (e.g. use mock.patch.object or monkeypatch to set configuration.espidf_missing_function_info = True) before constructing object_c and calling obj.append('nonexistent_function'), so the test always exercises the "append returns False" branch regardless of other tests mutating configuration.generator().builder/frameworks/espidf.py (1)
2357-2369:⚠️ Potential issue | 🟡 MinorTreat a non-runnable venv Python as broken too.
The new validation only checks that the interpreter file exists. A corrupted or non-executable binary still passes
_is_venv_interpreter_valid(), then_get_idf_venv_python_version()raisesOSErrorand the build aborts instead of recreating the IDF venv.🛠️ Harden the validation path
def _get_idf_venv_python_version(): try: version = subprocess.check_output( [ get_python_exe(), "-c", "import sys;print('{0}.{1}.{2}-{3}.{4}'.format(*list(sys.version_info)))" ], text=True ) return version.strip() - except subprocess.CalledProcessError as e: + except (subprocess.CalledProcessError, OSError) as e: print("Failed to extract Python version from IDF virtual env!") return None @@ def _is_venv_interpreter_valid(venv_dir): python_path = get_executable_path(venv_dir, "python") - return os.path.isfile(python_path) + if not os.path.isfile(python_path): + return False + try: + subprocess.check_output( + [python_path, "-c", "import sys"], + stderr=subprocess.STDOUT, + text=True, + ) + return True + except (subprocess.CalledProcessError, OSError): + return FalseAlso applies to: 2431-2457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/frameworks/espidf.py` around lines 2357 - 2369, The validation currently only checks for the interpreter file's existence; update _is_venv_interpreter_valid() to ensure the venv python is runnable (executable and actually launches) by attempting to execute a lightweight command (e.g., get_python_exe() with "-c 'import sys; print(1)'" or equivalent) and treat any OSError/CalledProcessError/ non-zero exit as invalid so the code path will recreate the IDF venv instead of letting _get_idf_venv_python_version() raise; ensure error cases are handled consistently and that _get_idf_venv_python_version() remains tolerant (or its call guarded) to avoid unhandled OSError if the interpreter is invalid.
🧹 Nitpick comments (2)
test/test_integration.py (1)
302-321: Test class validates fixture setup rather than relinker behavior.These tests verify that the
setUp()fixture is written correctly, but don't exercise the actual linker script modification code. The relinker transformation testing is handled byTestIdempotency, so this isn't a gap in coverage, but the class nameTestLinkerScriptModificationis misleading.Consider renaming to
TestLinkerScriptFixtureor adding actual relink transformation assertions here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_integration.py` around lines 302 - 321, The test class currently named TestLinkerScriptModification only exercises the setUp fixture (methods test_linker_script_exists, test_linker_script_has_iram_section, test_linker_script_has_flash_section) so rename the class to TestLinkerScriptFixture to reflect that it validates fixture setup (or alternatively add assertions that call the actual relinker/transformation code used by TestIdempotency if you prefer to keep the original name); update the class declaration from TestLinkerScriptModification to TestLinkerScriptFixture and adjust any references/imports or docstrings accordingly.builder/relinker/relinker.py (1)
220-252: Pattern-based detection may still match non-relinker lines in edge cases.The
_is_relinker_iram_include()and_is_relinker_flash_include()predicates use heuristic pattern matching rather than comparing against exact generated lines. While the patterns are reasonably specific (requiring*,:,.*(in combination), they could theoretically match hand-written ldgen entries.However, based on learnings, PlatformIO regenerates
sections.ldwhen relinker settings change, so this is unlikely to cause issues in practice. Consider this a known limitation rather than a blocking issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/relinker.py` around lines 220 - 252, The current heuristics in _is_relinker_iram_include and _is_relinker_flash_include can false-match hand-written lines; tighten detection by switching to anchored, specific regex checks: in _is_relinker_iram_include require the line to start with '*' then a lib:obj token (no spaces), include the literal substring '.*(' immediately followed by section tokens containing '.iram1.' and a closing ')' (i.e. match the full pattern rather than substrings); similarly in _is_relinker_flash_include require an anchored pattern that contains explicit section names ('.literal.' or '.text.' or '.iram1.') inside the parentheses and no extraneous characters outside the expected lib:obj.*(...) form; update these two functions to use those stricter matches so only relinker-generated lines are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/test_integration.py`:
- Around line 464-465: The test is calling the relink_c constructor with
arguments in the wrong order; change the call to pass input_script (the linker
script) as the first argument followed by library_csv, object_csv, function_csv,
sdkconfig (and keep missing_function_info=True) so the invocation matches
relink_c.__init__(self, input, library_file, object_file, function_file,
sdkconfig_file, ...); update the line that creates relink1 to use input_script
as the first parameter and the other CSV variables in the original order.
- Around line 474-475: The second call to relink_c uses the wrong argument
order; update the invocation of relink_c (the call assigning relink2) to match
the function's expected parameter order by rearranging the passed variables
(library_csv, object_csv, function_csv, output1, sdkconfig,
missing_function_info=True) into the correct sequence used by the relink_c
definition so that the positional arguments align with the function signature
rather than the current incorrect order.
---
Duplicate comments:
In `@builder/frameworks/espidf.py`:
- Around line 2357-2369: The validation currently only checks for the
interpreter file's existence; update _is_venv_interpreter_valid() to ensure the
venv python is runnable (executable and actually launches) by attempting to
execute a lightweight command (e.g., get_python_exe() with "-c 'import sys;
print(1)'" or equivalent) and treat any OSError/CalledProcessError/ non-zero
exit as invalid so the code path will recreate the IDF venv instead of letting
_get_idf_venv_python_version() raise; ensure error cases are handled
consistently and that _get_idf_venv_python_version() remains tolerant (or its
call guarded) to avoid unhandled OSError if the interpreter is invalid.
In `@test/test_configuration.py`:
- Around line 209-217: The test can be flaky because object_c.append() behavior
depends on the module-global espidf_missing_function_info mutated by
configuration.generator(); to stabilize, explicitly pin
espidf_missing_function_info to the warning-only value inside the test (e.g. use
mock.patch.object or monkeypatch to set
configuration.espidf_missing_function_info = True) before constructing object_c
and calling obj.append('nonexistent_function'), so the test always exercises the
"append returns False" branch regardless of other tests mutating
configuration.generator().
In `@test/test_relinker_functionality.py`:
- Around line 54-92: The test modifies the environment variable IDF_PATH inside
test_path_normalization but only restores it in an inner finally block, which
can leak into other tests; move the restoration of the original_idf so it always
executes in the outer finally (the same block that calls
shutil.rmtree(temp_dir)) to guarantee IDF_PATH is restored even if an earlier
assertion or exception occurs; update the cleanup logic in
test_path_normalization to set os.environ['IDF_PATH'] back to original_idf (or
delete it if original_idf was None) inside the outer finally that runs after
tempfile.mkdtemp()/shutil.rmtree.
In `@test/TESTING.md`:
- Around line 224-227: The cleanup guidance and example temp paths don't match
how the temp dir is created in setUp: tempfile.mkdtemp() produces names like
/tmp/tmp* but the doc's cleanup examples expect /tmp/tmp*relinker*; to fix,
either create the temp dir with a deterministic prefix (e.g., call
tempfile.mkdtemp(prefix="relinker_") where setUp assigns self.temp_dir) or
update the cleanup instructions/examples to reference the actual pattern
produced by tempfile.mkdtemp() (e.g., /tmp/tmp* or use self.temp_dir directly)
so the documented cleanup matches the code.
---
Nitpick comments:
In `@builder/relinker/relinker.py`:
- Around line 220-252: The current heuristics in _is_relinker_iram_include and
_is_relinker_flash_include can false-match hand-written lines; tighten detection
by switching to anchored, specific regex checks: in _is_relinker_iram_include
require the line to start with '*' then a lib:obj token (no spaces), include the
literal substring '.*(' immediately followed by section tokens containing
'.iram1.' and a closing ')' (i.e. match the full pattern rather than
substrings); similarly in _is_relinker_flash_include require an anchored pattern
that contains explicit section names ('.literal.' or '.text.' or '.iram1.')
inside the parentheses and no extraneous characters outside the expected
lib:obj.*(...) form; update these two functions to use those stricter matches so
only relinker-generated lines are detected.
In `@test/test_integration.py`:
- Around line 302-321: The test class currently named
TestLinkerScriptModification only exercises the setUp fixture (methods
test_linker_script_exists, test_linker_script_has_iram_section,
test_linker_script_has_flash_section) so rename the class to
TestLinkerScriptFixture to reflect that it validates fixture setup (or
alternatively add assertions that call the actual relinker/transformation code
used by TestIdempotency if you prefer to keep the original name); update the
class declaration from TestLinkerScriptModification to TestLinkerScriptFixture
and adjust any references/imports or docstrings accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b5651aa-db43-466c-87f0-f69b052c9f06
⛔ Files ignored due to path filters (3)
builder/relinker/examples/esp32c2/function.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/library.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/object.csvis excluded by!**/*.csv
📒 Files selected for processing (14)
RELINKER_INTEGRATION.mdbuilder/frameworks/espidf.pybuilder/relinker/__init__.pybuilder/relinker/configuration.pybuilder/relinker/relinker.pytest/.gitignoretest/README.mdtest/TESTING.mdtest/VALIDATION_REPORT.mdtest/run_tests.pytest/test_configuration.pytest/test_integration.pytest/test_relinker.pytest/test_relinker_functionality.py
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
builder/relinker/configuration.py (1)
109-127:⚠️ Potential issue | 🟠 MajorStrict mode still fails open here.
--missing_function_infois documented as “print missing function info instead of raising”, but both failure branches returnFalseeven when that flag is off. In the defaultbuilder/frameworks/espidf.pyintegration path, that silently drops relink targets instead of failing the build.Suggested fix
def append(self, func): if func in self._WILDCARD_SECTIONS: self.funcs[func] = self._WILDCARD_SECTIONS[func] self.section_all = True return True if not self.dumps: - # Return False silently regardless of espidf_missing_function_info setting - # Only print warning if espidf_missing_function_info is True if espidf_missing_function_info: print("Warning: Failed to load symbol data for %s:%s" % (self.library, self.name)) - return False + return False + raise RuntimeError( + "Failed to load symbol data for %s:%s" % (self.library, self.name) + ) section = self.get_func_section(self.dumps, func) if section is None: - # Return False silently regardless of espidf_missing_function_info setting - # Only print warning if espidf_missing_function_info is True if espidf_missing_function_info: print("Warning: Failed to find section for function '%s' in %s:%s" % (func, self.library, self.name)) - return False + return False + raise RuntimeError( + "Failed to find section for function '%s' in %s:%s" + % (func, self.library, self.name) + ) self.funcs[func] = section return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/configuration.py` around lines 109 - 127, The append method in configuration.py incorrectly "fails open": when espidf_missing_function_info is False it still returns False on both missing dumps and missing section instead of raising, which causes silent drops; update append (function name append, checks of self.dumps and get_func_section) to raise a clear exception (e.g., RuntimeError or a custom exception) with contextual message including self.library, self.name and func when espidf_missing_function_info is False, while keeping the current print-and-return behavior when espidf_missing_function_info is True so callers see warnings in permissive mode.test/test_relinker_functionality.py (1)
158-229:⚠️ Potential issue | 🟠 MajorThis generator test never reaches the objdump/symbol path.
builder/relinker/configuration.py:object_c.read_dump_info()returns before callingsubprocess.check_output()unless the object file exists. Because this fixture never createstemp_dir/esp-idf/test.c.obj, the mock stays unused and the test still passes even if section extraction regresses.Suggested fix
sdkconfig = os.path.join(temp_dir, 'sdkconfig') with open(sdkconfig, 'w') as f: f.write('CONFIG_TEST=y\n') + + obj_path = os.path.join(temp_dir, 'esp-idf', 'test.c.obj') + os.makedirs(os.path.dirname(obj_path), exist_ok=True) + with open(obj_path, 'wb'): + pass @@ with patch('configuration.subprocess.check_output') as mock_subprocess: # Mock objdump output for symbol table mock_subprocess.return_value = b'00000000 g F .text.test_func 00000010 test_func\n' @@ - # Verify the object was processed (may or may not be present depending on file existence) + mock_subprocess.assert_called_once() lib = libraries.libs['libtest.a'] self.assertEqual(lib.name, 'libtest.a', "Library name should match") self.assertTrue(hasattr(lib, 'objs'), "Library should have objs attribute") + self.assertIn('test.c.obj', lib.objs) + self.assertEqual( + lib.objs['test.c.obj'].funcs['test_func'], + 'test_func', + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_relinker_functionality.py` around lines 158 - 229, The test never exercises object_c.read_dump_info's call to subprocess.check_output because the object file path (temp_dir/esp-idf/test.c.obj) doesn't exist; modify test_csv_processing so before calling generator it either creates the expected object file at esp-idf/test.c.obj (e.g., mkdir esp-idf and touch a minimal file) or patches os.path.exists (or object_c.read_dump_info) to return True so that subprocess.check_output (patched as configuration.subprocess.check_output) is actually invoked and the objdump/symbol path is exercised.builder/relinker/relinker.py (2)
349-357:⚠️ Potential issue | 🔴 CriticalDon't erase the bare
.iram1catch-all on flash-only relinks.Line 357 sets
self.iram1_excludeto''when there is nothing to exclude, but Lines 391-395 still replace the original*(.iram1 .iram1.*)descriptor with that empty value. For the common case wherefunc2sect()only produced.literal/.texttargets, this drops the global.iram1mapping and can leave unrelated IRAM sections unmapped.🛠️ Minimal fix
self.iram1_exclude = ( ' *(EXCLUDE_FILE(%s) .iram1.*)\n' ' *(EXCLUDE_FILE(%s) .iram1)' - ) % (exclude_tokens, exclude_tokens) if exclude_tokens else '' + ) % (exclude_tokens, exclude_tokens) if exclude_tokens else ' *(.iram1 .iram1.*)'Also applies to: 388-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/relinker.py` around lines 349 - 357, The current code sets self.iram1_exclude to '' when exclude_tokens is empty, which causes the original catch-all mapping for .iram1 to be removed during the later replacement; instead, when exclude_tokens is empty set self.iram1_exclude to the original catch-all mapping string (e.g. ' *(.iram1 .iram1.*)') or alter the replacement logic so that when exclude_tokens == '' you skip replacing the original '*(.iram1 .iram1.*)' entry; update the code that builds self.iram1_exclude (the expression using exclude_tokens, self.filter.add(), and iram1_exclude) so it preserves the global .iram1 mapping when there are no exclude tokens.
220-251:⚠️ Potential issue | 🟠 MajorThe cleanup pass can still strip ldgen-owned include lines.
Lines 220-251 treat any
*lib:obj.*(...)mapping as relinker-owned, and Lines 402-417 delete those matches wholesale around the IRAM/flash markers. ldgen emits the same shape for its own object-specific placements, so a first relink pass can remove legitimate mappings before inserting the new block.🛠️ Safer cleanup approach
+ relinker_iram_lines = { + line.strip() for line in self.iram1_include.splitlines() if line.strip() + } + relinker_flash_lines = { + line.strip() for line in self.flash_include.splitlines() if line.strip() + } ... - while j < len(lines) and _is_relinker_iram_include(lines[j]): + while j < len(lines) and lines[j].strip() in relinker_iram_lines: lines.pop(j) ... - while j >= 0 and (_is_relinker_flash_include(lines[j]) or not lines[j].strip()): - if _is_relinker_flash_include(lines[j]): + while j >= 0 and (lines[j].strip() in relinker_flash_lines or not lines[j].strip()): + if lines[j].strip() in relinker_flash_lines: lines.pop(j)Also applies to: 396-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/relinker.py` around lines 220 - 251, The cleanup functions _is_relinker_iram_include and _is_relinker_flash_include are too permissive and match ldgen's object-specific includes; tighten detection to only target relinker-owned lines by requiring a unique relinker marker/token in the line (e.g. "/* RELINKER */" or another deterministic token) before returning True, and update the relinker code that emits its include lines to append that token; ensure the deletion logic that calls these predicates (the cleanup around the IRAM/flash markers referenced near the existing deletion blocks) uses the new stricter checks so ldgen-owned "*lib:obj.*(...)" entries are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/frameworks/espidf.py`:
- Around line 2435-2457: The current flow only calls install_python_deps inside
_recreate_and_save, so when a venv exists and _is_venv_outdated returns False
the code skips validating/installing packages; change the steady-state path to
still call install_python_deps(deps) to ensure packages are present even when
the metadata matches. Concretely, after computing deps = _get_python_deps() and
verifying venv_data_file exists and
_is_venv_interpreter_valid(get_idf_venv_dir()), call install_python_deps(deps)
(but do not recreate the venv) before exiting; keep _recreate_and_save(venv_dir,
deps, venv_data_file) for the recreate cases and remove the early-return that
skips package installation. Ensure references to _recreate_and_save,
install_python_deps, _get_python_deps, _is_venv_interpreter_valid, and
_is_venv_outdated are updated accordingly.
In `@builder/relinker/relinker.py`:
- Around line 153-168: The match method currently compares full descriptors like
"libfreertos.a:tasks.*" against entries that only contain whole-library names,
so the check is always false; update match(self, desc) to extract the library
token first (base = desc.split(':', 1)[0].lstrip('*')) and then test if base in
self.entries, preserving the original printed descriptor when removing; apply
the identical change to the similar method/block around lines 271-278 to ensure
library-level exclusions are honored.
---
Duplicate comments:
In `@builder/relinker/configuration.py`:
- Around line 109-127: The append method in configuration.py incorrectly "fails
open": when espidf_missing_function_info is False it still returns False on both
missing dumps and missing section instead of raising, which causes silent drops;
update append (function name append, checks of self.dumps and get_func_section)
to raise a clear exception (e.g., RuntimeError or a custom exception) with
contextual message including self.library, self.name and func when
espidf_missing_function_info is False, while keeping the current
print-and-return behavior when espidf_missing_function_info is True so callers
see warnings in permissive mode.
In `@builder/relinker/relinker.py`:
- Around line 349-357: The current code sets self.iram1_exclude to '' when
exclude_tokens is empty, which causes the original catch-all mapping for .iram1
to be removed during the later replacement; instead, when exclude_tokens is
empty set self.iram1_exclude to the original catch-all mapping string (e.g. '
*(.iram1 .iram1.*)') or alter the replacement logic so that when exclude_tokens
== '' you skip replacing the original '*(.iram1 .iram1.*)' entry; update the
code that builds self.iram1_exclude (the expression using exclude_tokens,
self.filter.add(), and iram1_exclude) so it preserves the global .iram1 mapping
when there are no exclude tokens.
- Around line 220-251: The cleanup functions _is_relinker_iram_include and
_is_relinker_flash_include are too permissive and match ldgen's object-specific
includes; tighten detection to only target relinker-owned lines by requiring a
unique relinker marker/token in the line (e.g. "/* RELINKER */" or another
deterministic token) before returning True, and update the relinker code that
emits its include lines to append that token; ensure the deletion logic that
calls these predicates (the cleanup around the IRAM/flash markers referenced
near the existing deletion blocks) uses the new stricter checks so ldgen-owned
"*lib:obj.*(...)" entries are preserved.
In `@test/test_relinker_functionality.py`:
- Around line 158-229: The test never exercises object_c.read_dump_info's call
to subprocess.check_output because the object file path
(temp_dir/esp-idf/test.c.obj) doesn't exist; modify test_csv_processing so
before calling generator it either creates the expected object file at
esp-idf/test.c.obj (e.g., mkdir esp-idf and touch a minimal file) or patches
os.path.exists (or object_c.read_dump_info) to return True so that
subprocess.check_output (patched as configuration.subprocess.check_output) is
actually invoked and the objdump/symbol path is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7eb50f39-d43e-4658-8da6-49afed1b5a18
⛔ Files ignored due to path filters (3)
builder/relinker/examples/esp32c2/function.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/library.csvis excluded by!**/*.csvbuilder/relinker/examples/esp32c2/object.csvis excluded by!**/*.csv
📒 Files selected for processing (14)
RELINKER_INTEGRATION.mdbuilder/frameworks/espidf.pybuilder/relinker/__init__.pybuilder/relinker/configuration.pybuilder/relinker/relinker.pytest/.gitignoretest/README.mdtest/TESTING.mdtest/VALIDATION_REPORT.mdtest/run_tests.pytest/test_configuration.pytest/test_integration.pytest/test_relinker.pytest/test_relinker_functionality.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
builder/frameworks/espidf.py (1)
2446-2452:⚠️ Potential issue | 🟠 MajorSteady-state IDF venvs never reconcile missing packages.
If the metadata still matches, this path exits without calling
install_python_deps(). A partially broken venv then survives until a later import/build failure, even thoughinstall_python_deps()is already idempotent and only repairs missing/mismatched packages.🐛 Proposed fix
- if not os.path.isfile(venv_data_file): - _recreate_and_save(venv_dir, deps, venv_data_file) - elif not _is_venv_interpreter_valid(venv_dir): + recreate = False + if not os.path.isfile(venv_data_file): + recreate = True + elif not _is_venv_interpreter_valid(venv_dir): print("Warning! Python interpreter in the IDF virtual environment is missing. Recreating...") - _recreate_and_save(venv_dir, deps, venv_data_file) + recreate = True elif _is_venv_outdated(venv_data_file, deps): - _recreate_and_save(venv_dir, deps, venv_data_file) + recreate = True + + if recreate: + _recreate_and_save(venv_dir, deps, venv_data_file) + else: + install_python_deps(deps)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/frameworks/espidf.py` around lines 2446 - 2452, The current flow skips repair when venv metadata matches, allowing missing packages to persist; update the logic in the block that checks venv_data_file/_is_venv_interpreter_valid/_is_venv_outdated so that when the metadata matches (i.e., none of those conditions triggered) you still call install_python_deps(venv_dir, deps) to reconcile/install any missing or mismatched packages instead of exiting early; specifically, locate the conditional around venv_data_file/venv_dir and after the final elif branch add a call to install_python_deps(venv_dir, deps) (or replace the no-op path with that call) while keeping existing calls to _recreate_and_save(...) when interpreter is missing or venv is outdated.builder/relinker/configuration.py (1)
113-126:⚠️ Potential issue | 🟠 MajorStrict mode still drops missing relinker targets silently.
Line 279 says
--missing_function_infoshould switch from raising to printing, but these branches returnFalsein both modes. That makeslibrary_c.append()quietly omit stale/missing entries in the default strict path instead of failing fast.🐛 Proposed fix
if not self.dumps: - # Return False silently regardless of espidf_missing_function_info setting - # Only print warning if espidf_missing_function_info is True if espidf_missing_function_info: print("Warning: Failed to load symbol data for %s:%s" % (self.library, self.name)) - return False + return False + raise RuntimeError( + "Failed to load symbol data for %s:%s" % (self.library, self.name) + ) section = self.get_func_section(self.dumps, func) if section is None: - # Return False silently regardless of espidf_missing_function_info setting - # Only print warning if espidf_missing_function_info is True if espidf_missing_function_info: print("Warning: Failed to find section for function '%s' in %s:%s" % (func, self.library, self.name)) - return False + return False + raise RuntimeError( + "Failed to find section for function '%s' in %s:%s" + % (func, self.library, self.name) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/relinker/configuration.py` around lines 113 - 126, The current branches in the method that check self.dumps and get_func_section(self.dumps, func) always return False, which silently omits missing relinker targets even in strict mode; instead, when espidf_missing_function_info is False (strict/default), raise an exception (e.g., RuntimeError or a specific ReLinkerError) with a clear message including self.library, self.name and func so callers like library_c.append() will fail fast, and only print the warning and return False when espidf_missing_function_info is True; update both branches that currently return False to implement this raise-vs-print behavior accordingly.
🧹 Nitpick comments (1)
test/test_integration.py (1)
137-139: The default strict relinker path still isn't exercised.Every end-to-end call opts into
missing_function_info=True, butbuilder/frameworks/espidf.pydefaultscustom_relinker_missing_function_infoto"no". Please add one negative case withFalseand assertgenerator()/relink_c()raises on a missing symbol so the default integration behavior is covered.Also applies to: 387-387, 457-457, 467-467
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_integration.py` around lines 137 - 139, Add a negative integration test that sets missing_function_info=False (so it uses the framework default custom_relinker_missing_function_info="no") and call the same flow that currently uses missing_function_info=True, then assert that generator() or relink_c() raises on the missing symbol; this will exercise the strict relinker path. Repeat the same negative assertion for the other test locations that currently pass True (the two other call sites mentioned) so the default-integration behavior is covered for generator()/relink_c() when a symbol is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@builder/frameworks/espidf.py`:
- Around line 2446-2452: The current flow skips repair when venv metadata
matches, allowing missing packages to persist; update the logic in the block
that checks venv_data_file/_is_venv_interpreter_valid/_is_venv_outdated so that
when the metadata matches (i.e., none of those conditions triggered) you still
call install_python_deps(venv_dir, deps) to reconcile/install any missing or
mismatched packages instead of exiting early; specifically, locate the
conditional around venv_data_file/venv_dir and after the final elif branch add a
call to install_python_deps(venv_dir, deps) (or replace the no-op path with that
call) while keeping existing calls to _recreate_and_save(...) when interpreter
is missing or venv is outdated.
In `@builder/relinker/configuration.py`:
- Around line 113-126: The current branches in the method that check self.dumps
and get_func_section(self.dumps, func) always return False, which silently omits
missing relinker targets even in strict mode; instead, when
espidf_missing_function_info is False (strict/default), raise an exception
(e.g., RuntimeError or a specific ReLinkerError) with a clear message including
self.library, self.name and func so callers like library_c.append() will fail
fast, and only print the warning and return False when
espidf_missing_function_info is True; update both branches that currently return
False to implement this raise-vs-print behavior accordingly.
---
Nitpick comments:
In `@test/test_integration.py`:
- Around line 137-139: Add a negative integration test that sets
missing_function_info=False (so it uses the framework default
custom_relinker_missing_function_info="no") and call the same flow that
currently uses missing_function_info=True, then assert that generator() or
relink_c() raises on the missing symbol; this will exercise the strict relinker
path. Repeat the same negative assertion for the other test locations that
currently pass True (the two other call sites mentioned) so the
default-integration behavior is covered for generator()/relink_c() when a symbol
is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31fb1ca1-579d-4868-974c-b627dccef322
📒 Files selected for processing (3)
builder/frameworks/espidf.pybuilder/relinker/configuration.pytest/test_integration.py
…multiple patterns
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Checklist: