feat(tarball): Add tarball/source build helper (Issue #452) #558
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a Tarball/Source Build Helper module and CLI integration: new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as cortex/cli.py
participant Helper as TarballHelper
participant FS as Source Files
participant Sys as pkg-config/apt/dpkg
User->>CLI: tarball analyze <source_dir>
CLI->>Helper: run_analyze_command(source_dir)
Helper->>FS: read manifests (CMakeLists.txt / configure.ac / meson.build / setup.py)
FS-->>Helper: file contents
Helper->>Helper: detect build system, parse dependencies
Helper->>Sys: check_installed(dependencies)
Sys-->>Helper: installed status
Helper-->>CLI: analysis results
CLI-->>User: display analysis
sequenceDiagram
participant User
participant CLI as cortex/cli.py
participant Helper as TarballHelper
participant PkgMgr as apt / apt-cache
participant History as ~/.cortex/manual_builds.json
User->>CLI: tarball install-deps <source_dir> [--dry-run]
CLI->>Helper: run_install_deps_command(source_dir, dry_run)
Helper->>Helper: determine missing packages
Helper->>PkgMgr: apt-get install (or simulate)
PkgMgr-->>Helper: result
Helper-->>CLI: report install outcome
User->>CLI: tarball track <name> <source_dir> [--packages ...]
CLI->>Helper: run_track_command(name, source_dir, packages)
Helper->>History: _load_history()
Helper->>History: _save_history(updated)
Helper-->>CLI: confirm tracked
User->>CLI: tarball cleanup <name> [--dry-run]
CLI->>Helper: run_cleanup_command(name, dry_run)
Helper->>History: _load_history()
Helper->>PkgMgr: remove packages (or simulate)
Helper->>History: _save_history(updated)
Helper-->>CLI: cleanup result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
CLA Verification PassedAll contributors have signed the CLA.
|
Summary of ChangesHello @jeremylongshore, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a powerful new cortex tarball command-line utility designed to simplify the often complex process of building software from source code. It provides intelligent analysis of various build systems to identify dependencies, automates the installation of missing development packages, and offers robust tracking and cleanup mechanisms for manual installations. This feature aims to significantly reduce the friction and manual effort involved in managing custom-built software. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a powerful tarball helper to analyze and manage dependencies for source builds. The implementation is well-structured with a dedicated tarball_helper module and comprehensive test coverage. The code is clear and the feature is a great addition to the CLI.
My review focuses on improving the robustness of the implementation. I've identified some areas for improvement:
- The dependency parsing logic in
tarball_helper.pyuses regular expressions that are too simple for complex, real-world build files, which could lead to missed dependencies. - The test suite for this parsing logic could be expanded to cover these more complex cases.
- In
cli.py, the exception handling is overly broad, and a minor logic improvement can make package name parsing more robust.
Overall, this is a very strong contribution. Addressing these points will make the new feature even more reliable.
| content = cmake_file.read_text(encoding="utf-8", errors="ignore") | ||
|
|
||
| # Find find_package() calls | ||
| for match in re.finditer(r"find_package\s*\(\s*(\w+)", content, re.IGNORECASE): |
There was a problem hiding this comment.
The regular expression used for find_package is a bit too simple and may fail on common CMake syntax, leading to missed dependencies. For example, it won't match package names containing hyphens or numbers if not at the end (e.g., Qt5-Core), and it doesn't correctly handle arguments that follow the package name.
A more robust regex that captures the first argument would handle more cases. A similar issue exists with the pkg_check_modules regex on line 261, which can fail to find all packages if multiple are listed.
| for match in re.finditer(r"find_package\s*\(\s*(\w+)", content, re.IGNORECASE): | |
| for match in re.finditer(r"find_package\s*\(\s*([^\s\)]+)", content, re.IGNORECASE): |
| if action == "analyze": | ||
| try: | ||
| run_analyze_command(args.source_dir) | ||
| return 0 | ||
| except Exception as e: | ||
| console.print(f"[red]Error: {e}[/red]") | ||
| return 1 | ||
|
|
||
| elif action == "install-deps": | ||
| try: | ||
| run_install_deps_command( | ||
| args.source_dir, | ||
| dry_run=getattr(args, "dry_run", False), | ||
| ) | ||
| return 0 | ||
| except Exception as e: | ||
| console.print(f"[red]Error: {e}[/red]") | ||
| return 1 | ||
|
|
||
| elif action == "track": | ||
| try: | ||
| packages = [] | ||
| if hasattr(args, "packages") and args.packages: | ||
| packages = args.packages.split(",") | ||
| run_track_command(args.name, args.source_dir, packages) | ||
| return 0 | ||
| except Exception as e: | ||
| console.print(f"[red]Error: {e}[/red]") | ||
| return 1 | ||
|
|
||
| elif action == "list": | ||
| try: | ||
| run_list_command() | ||
| return 0 | ||
| except Exception as e: | ||
| console.print(f"[red]Error: {e}[/red]") | ||
| return 1 | ||
|
|
||
| elif action == "cleanup": | ||
| try: | ||
| run_cleanup_command( | ||
| args.name, | ||
| dry_run=getattr(args, "dry_run", False), | ||
| ) | ||
| return 0 | ||
| except Exception as e: | ||
| console.print(f"[red]Error: {e}[/red]") | ||
| return 1 | ||
|
|
There was a problem hiding this comment.
The repeated use of except Exception as e is too broad. It can catch and hide unexpected errors, including SystemExit or KeyboardInterrupt, making debugging difficult. It's better to catch more specific exceptions that you expect from the tarball_helper functions (like ValueError or FileNotFoundError) and let other exceptions propagate to a higher-level handler. If you must catch a broad exception, consider logging the full traceback when in verbose mode to aid debugging.
| try: | ||
| packages = [] | ||
| if hasattr(args, "packages") and args.packages: | ||
| packages = args.packages.split(",") |
There was a problem hiding this comment.
When splitting the packages string, it's a good practice to strip whitespace from each package name. This makes the input more robust against variations like "pkg1, pkg2" (with a space).
| packages = args.packages.split(",") | |
| packages = [p.strip() for p in args.packages.split(",")] |
| class TestAnalyzeCMake: | ||
| """Test CMake analysis.""" | ||
|
|
||
| def test_analyze_cmake_find_package(self, tmp_path): | ||
| cmake_content = """ | ||
| cmake_minimum_required(VERSION 3.10) | ||
| project(MyProject) | ||
| find_package(OpenSSL REQUIRED) | ||
| find_package(ZLIB) | ||
| """ | ||
| (tmp_path / "CMakeLists.txt").write_text(cmake_content) | ||
|
|
||
| helper = TarballHelper() | ||
| with patch.object(helper, "_check_installed"): | ||
| analysis = helper.analyze(tmp_path) | ||
|
|
||
| assert analysis.build_system == BuildSystem.CMAKE | ||
| pkg_names = [d.name for d in analysis.dependencies] | ||
| assert "openssl" in pkg_names | ||
| assert "zlib" in pkg_names | ||
|
|
||
| def test_analyze_cmake_pkg_check_modules(self, tmp_path): | ||
| cmake_content = """ | ||
| pkg_check_modules(GLIB REQUIRED glib-2.0) | ||
| """ | ||
| (tmp_path / "CMakeLists.txt").write_text(cmake_content) | ||
|
|
||
| helper = TarballHelper() | ||
| with patch.object(helper, "_check_installed"): | ||
| analysis = helper.analyze(tmp_path) | ||
|
|
||
| pkg_names = [d.name for d in analysis.dependencies] | ||
| assert "glib-2.0" in pkg_names | ||
|
|
There was a problem hiding this comment.
The tests for _analyze_cmake are good, but they only cover simple cases that the current regular expressions can handle. To make the test suite more robust and catch the fragility of the regex-based parsing, consider adding test cases with more complex, real-world CMakeLists.txt syntax.
Examples of valuable test cases to add:
find_packagewith package names containing numbers and hyphens (e.g.,find_package(Qt5-Core)).pkg_check_moduleswith multiple packages listed (e.g.,pkg_check_modules(GSTREAMER gstreamer-1.0 gstreamer-video-1.0)).find_packagewith version numbers and components (e.g.,find_package(Boost 1.71.0 REQUIRED COMPONENTS program_options)).
Adding these would help ensure the dependency analysis is reliable across a wider range of projects.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/test_tarball_helper.py (2)
264-277: Consider adding assertion for the subprocess command.The success and failure tests mock
subprocess.runbut don't verify the command arguments. Consider adding an assertion to confirm the correct apt-get command is constructed.💡 Optional enhancement
def test_install_deps_success(self): helper = TarballHelper() mock_result = MagicMock(returncode=0) - with patch("subprocess.run", return_value=mock_result): + with patch("subprocess.run", return_value=mock_result) as mock_run: result = helper.install_dependencies(["zlib1g-dev"]) + mock_run.assert_called_once() + assert "apt-get" in mock_run.call_args[0][0] assert result is True
375-410: Run command tests verify no exceptions are raised but lack output assertions.These tests confirm the commands execute without error, which is valuable. For higher confidence, consider capturing console output and verifying expected content is printed.
cortex/tarball_helper.py (5)
317-324: AC_CHECK_LIB mapping may not always find apt packages.The code tries to find apt package as
lib{lib_name}inPKGCONFIG_PACKAGES, but many libraries (e.g.,z→zlib,crypto→libssl-dev) don't follow this pattern. The mapping may miss some packages.This is a minor limitation since the tool still identifies the dependency; apt package suggestion just won't always work.
Consider expanding the library-to-package mapping for common cases like
z→zlib1g-dev,crypto→libssl-dev.
374-386: Consider adding timeout to subprocess calls.The
subprocess.runcalls forpkg-configanddpkg-querydon't specify a timeout. While these are typically fast, in edge cases (broken dpkg, stale locks) they could hang indefinitely.💡 Add timeout to prevent potential hangs
elif dep.dep_type == "pkg-config" and dep.apt_package: # Check via pkg-config result = subprocess.run( ["pkg-config", "--exists", dep.name], capture_output=True, + timeout=5, ) dep.found = result.returncode == 0 elif dep.apt_package: # Check if apt package is installed result = subprocess.run( ["dpkg-query", "-W", "-f=${Status}", dep.apt_package], capture_output=True, text=True, + timeout=5, )
448-451: Consider providing error feedback on installation failure.When
apt-get installfails, the method returnsFalsebut doesn't provide any feedback to the user about what went wrong. The apt output would have been shown, but a follow-up message might help.💡 Optional: Add failure message
result = subprocess.run(cmd) - return result.returncode == 0 + if result.returncode != 0: + console.print("[red]Package installation failed. Check the output above for details.[/red]") + return False + return True
552-565: History removal happens before package removal confirmation.The installation is removed from history (line 553-554) before the package removal prompt. If the user declines package removal or if it fails, the history is still lost.
Consider: Should the tracking entry be preserved if the user declines to remove packages?
💡 Optional: Delay history removal
- # Remove from history - del history[name] - self._save_history(history) - - console.print(f"[green]Removed '{name}' from tracking.[/green]") - if packages: remove_pkgs = Confirm.ask( f"Remove {len(packages)} packages that were installed for this build?" ) if remove_pkgs: cmd = ["sudo", "apt-get", "remove", "-y"] + packages subprocess.run(cmd) + # Remove from history after user decisions + del history[name] + self._save_history(history) + console.print(f"[green]Removed '{name}' from tracking.[/green]") + return True
577-580: Consider specifying encoding for write_text.
_load_historydoesn't specify encoding forread_text(), and_save_historydoesn't specify encoding forwrite_text(). While Python 3 defaults to UTF-8 on most systems, explicit encoding ensures consistency.💡 Optional: Explicit encoding
def _load_history(self) -> dict: """Load installation history from file.""" if not self.history_file.exists(): return {} try: - return json.loads(self.history_file.read_text()) + return json.loads(self.history_file.read_text(encoding="utf-8")) except (json.JSONDecodeError, OSError): return {} def _save_history(self, history: dict) -> None: """Save installation history to file.""" - self.history_file.write_text(json.dumps(history, indent=2)) + self.history_file.write_text(json.dumps(history, indent=2), encoding="utf-8")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/cli.pycortex/tarball_helper.pytests/test_tarball_helper.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.pytests/test_tarball_helper.pycortex/tarball_helper.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_tarball_helper.py
🧬 Code graph analysis (2)
cortex/cli.py (1)
cortex/tarball_helper.py (5)
run_analyze_command(582-647)run_cleanup_command(699-707)run_install_deps_command(650-670)run_list_command(673-696)run_track_command(710-726)
cortex/tarball_helper.py (1)
cortex/branding.py (1)
cx_header(82-88)
🔇 Additional comments (18)
cortex/cli.py (3)
604-685: LGTM! Clean implementation following existing patterns.The
tarball()method correctly:
- Uses lazy imports to avoid loading
tarball_helperunless needed- Handles each subcommand with try/except for error resilience
- Returns appropriate exit codes (0 for success, 1 for error)
- Provides helpful usage info when no action is specified
The structure is consistent with existing command handlers like
sandbox()andnotify().
2357-2399: Argument parser definitions are well-structured.The tarball subcommands are defined consistently with other CLI subcommands. The
--packagesargument fortrackuses comma-separated values which is handled in thetarball()method viaargs.packages.split(",").
2662-2663: Command routing is correct.tests/test_tarball_helper.py (6)
1-25: Well-organized test file with comprehensive imports.The test file follows good practices:
- Clear module docstring explaining purpose
- All necessary imports from the module under test
- Proper use of pytest and unittest.mock
27-74: Good dataclass and enum tests.These tests verify the basic data structures work correctly with default values and full initialization.
85-131: Comprehensive build system detection tests.All six build system types are tested, plus the error case for invalid paths. The tests correctly use
tmp_pathfor isolation.
133-218: Analysis tests cover the main parsing patterns well.The tests mock
_check_installedappropriately to avoid external tool calls during testing, while still verifying the parsing logic extracts dependencies correctly.
451-827: Excellent edge case coverage.The advanced test classes thoroughly cover:
- Empty build files
- Missing configuration files
- Corrupt JSON history
- Command-line scenarios with missing directories
- Various fallback behaviors
This provides good confidence in error handling paths.
829-830: Good test entry point.cortex/tarball_helper.py (9)
1-24: Well-structured module with clear imports and organization.The module docstring explains the purpose, and imports are organized logically. Using
richfor console output aligns with the rest of the codebase.
26-94: Well-designed data models with proper type hints and docstrings.The dataclasses correctly:
- Use
field(default_factory=list)for mutable default arguments- Include comprehensive docstrings as required by the coding guidelines
- Use
Optional[str]for nullable fields
96-171: Good class design with comprehensive package mappings.The class follows good patterns:
- Clear docstring explaining purpose
- Class-level constants for mappings (26 common packages)
- Safe directory creation in
__init__
172-199: Clean build system detection with sensible priority order.The detection order prioritizes more specific build systems (CMake, Meson) before generic ones (Makefile), which is correct since a project might have both.
200-243: Well-structured analysis orchestration.The
analyzemethod cleanly:
- Resolves paths to absolute
- Delegates to specific analyzers
- Checks installation status
- Collects missing packages (with deduplication)
- Generates appropriate build commands
244-282: Good CMake parsing for common patterns.The regex patterns correctly handle:
find_package(OpenSSL REQUIRED)pkg_check_modules(GLIB REQUIRED glib-2.0)CHECK_INCLUDE_FILE("zlib.h" HAVE_ZLIB)Using
errors="ignore"is appropriate for potentially mixed-encoding build files.
453-488: Good alternative package detection with sensible fallbacks.The method:
- First tries exact name match
- Falls back to
lib{name}prefix- Prefers
-devpackages for build dependenciesThis covers common scenarios well.
582-648: Well-formatted analysis output with rich components.The
run_analyze_commandfunction provides excellent user experience:
- Shows build system detected
- Suggests packaged alternatives when available
- Displays dependencies in a formatted table
- Shows missing packages with install command
- Lists build commands
650-726: Clean command functions delegating to TarballHelper.All run_* functions follow a consistent pattern:
- Validate inputs
- Create TarballHelper instance
- Delegate to appropriate method
- Handle output/feedback
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@jeremylongshore Documentation missing, also Kindly address bot comments.
|
Thanks for the review! I've addressed all the feedback: Bot comment fixes:
Documentation:
All 71 tests passing with 99% code coverage. Ready for re-review. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/TARBALL_HELPER.md (1)
120-120: Consider using lowercase for CMake function names.While CMake is case-insensitive, the convention is to use lowercase for function names.
check_include_file()is more commonly seen in CMake documentation thanCHECK_INCLUDE_FILE().📝 Minor documentation improvement
-| CMake | `CMakeLists.txt` | `find_package()`, `pkg_check_modules()`, `CHECK_INCLUDE_FILE()` | +| CMake | `CMakeLists.txt` | `find_package()`, `pkg_check_modules()`, `check_include_file()` |
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/tarball_helper.pydocs/TARBALL_HELPER.md
🚧 Files skipped from review as they are similar to previous changes (1)
- cortex/tarball_helper.py
🔇 Additional comments (6)
docs/TARBALL_HELPER.md (6)
1-52: Excellent introduction and quick start section.The overview clearly communicates the tool's purpose and key features. The quick start example is comprehensive, showing the complete workflow from analysis through cleanup with realistic output. The progression is logical and immediately demonstrates value to users.
54-115: Clear and comprehensive command documentation.All five commands are well documented with usage examples and options. The structure is consistent, making it easy for users to find the information they need.
126-161: Build commands are accurate and helpful.The build commands for each system follow standard practices and include helpful notes (like the autoreconf step for Autotools). This will guide users effectively through the build process.
162-200: Dependency mappings are accurate and comprehensive.The header-to-package, pkg-config-to-package, and build-tool mappings are correct for Debian/Ubuntu systems. This will be very helpful for users trying to resolve build dependencies.
201-246: Storage format and workflow example are well documented.The JSON storage format is clearly documented with all fields explained. The nginx workflow example is particularly valuable—it walks through a complete real-world scenario from download to cleanup, including the helpful reminder about considering packaged alternatives.
248-283: Limitations and troubleshooting sections add valuable context.The documentation honestly acknowledges the tool's limitations, which helps set appropriate user expectations. The troubleshooting section addresses likely pain points with practical solutions, including correct apt-cache commands for manual investigation.
Adding myself to enable CLA checks on PRs cortexlinux#557 and cortexlinux#558. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@Anshgrover23 I've addressed all feedback:
Once #568 is merged, the CLA check will pass. Ready for re-review! |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@jeremylongshore CI is failing.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@jeremylongshore Address coderabbit comments.
|
CLA PR #568 has been merged. Could you please re-run the CLA check? Thanks! |
|
recheck |
|
Same issue as #557 - the CLA failure is due to Could |
Adding myself to enable CLA checks on PRs cortexlinux#557 and cortexlinux#558. Co-authored-by: jeremylongshore <jeremylongshore@users.noreply.github.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Implements issue cortexlinux#452 - Tarball Helper with 4 key features: 1. analyze: Detect build system (CMake, Meson, Autotools, Python, Make) and parse dependencies from build files 2. install-deps: Install missing -dev packages via apt 3. track: Record manual installations with packages used 4. cleanup: Remove tracked installations and optionally packages Features: - Auto-detects 6 build systems (CMake, Meson, Autotools, Make, Python, Unknown) - Parses find_package(), pkg_check_modules(), dependency(), AC_CHECK_* - Maps headers and pkg-config names to apt packages - Suggests packaged alternatives before building from source - Tracks installation history in ~/.cortex/manual_builds.json Demo: https://asciinema.org/a/rFzQXtU4HqmrRscL
- Add timeouts (5s) to pkg-config and dpkg-query subprocess calls - Add error feedback message when apt-get install fails - Fix history removal timing: move deletion after user interactions - Add explicit UTF-8 encoding to history file read/write operations - Add comprehensive documentation in docs/TARBALL_HELPER.md
jeremylongshore
force-pushed
the
feature/tarball-helper
branch
from
a7cb1a8 to
2b20c2a
Compare
|
Rebased and removed the Co-Authored-By trailers. CLA check is now passing. Ready for review! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cortex/tarball_helper.py:
- Around line 51-53: Change the type annotation for the variable apt_package
from Optional[str] to the PEP 604 union style str | None, and then remove the
now-unused Optional import from the top-level imports as well as the other
unused Optional import elsewhere in the module (the remaining import that was
only used for Optional). Ensure no other references to Optional remain before
committing.
- Around line 466-500: The return type annotation for find_alternative should
use the PEP 604 union syntax instead of Optional: change the signature from def
find_alternative(self, name: str) -> Optional[str]: to def
find_alternative(self, name: str) -> str | None: and update imports/usages
accordingly (remove or replace typing.Optional import and any other references
to Optional in this file) so the linter stops flagging the annotation.
🧹 Nitpick comments (4)
cortex/tarball_helper.py (4)
366-394: Consider handling missingpkg-configordpkg-querycommands.If
pkg-configordpkg-queryare not installed,subprocess.runwill raiseFileNotFoundError. This could cause the entire analysis to fail on minimal systems.♻️ Proposed fix to handle missing commands gracefully
elif dep.dep_type == "pkg-config" and dep.apt_package: # Check via pkg-config try: result = subprocess.run( ["pkg-config", "--exists", dep.name], capture_output=True, timeout=5, ) dep.found = result.returncode == 0 - except subprocess.TimeoutExpired: + except (subprocess.TimeoutExpired, FileNotFoundError): dep.found = False elif dep.apt_package: # Check if apt package is installed try: result = subprocess.run( ["dpkg-query", "-W", "-f=${Status}", dep.apt_package], capture_output=True, text=True, timeout=5, ) dep.found = "install ok installed" in result.stdout - except subprocess.TimeoutExpired: + except (subprocess.TimeoutExpired, FileNotFoundError): dep.found = False
565-580: Consider checking the result of package removal.The
apt-get removecommand result is not checked. If removal fails, the user won't get feedback, yet the installation is still removed from history.♻️ Proposed improvement
if packages: remove_pkgs = Confirm.ask( f"Remove {len(packages)} packages that were installed for this build?" ) if remove_pkgs: cmd = ["sudo", "apt-get", "remove", "-y"] + packages - subprocess.run(cmd) + result = subprocess.run(cmd) + if result.returncode != 0: + console.print("[yellow]Warning: Package removal may have failed.[/yellow]") # Remove from history after all user interactions del history[name] self._save_history(history)
591-593: Consider handling write errors in_save_history.If the write fails (disk full, permission denied), the exception will propagate up and could leave the system in an inconsistent state where packages were removed but history wasn't updated.
♻️ Suggested improvement
def _save_history(self, history: dict) -> None: """Save installation history to file.""" - self.history_file.write_text(json.dumps(history, indent=2), encoding="utf-8") + try: + self.history_file.write_text(json.dumps(history, indent=2), encoding="utf-8") + except OSError as e: + console.print(f"[red]Warning: Failed to save history: {e}[/red]")
617-619: Name extraction may be inaccurate for hyphenated software names.The logic
path.name.split("-")[0]assumes the patternname-version, but software likenginx-1.25.3works whilemy-cool-app-1.0would extract justmyinstead ofmy-cool-app.This is a heuristic and may not always work perfectly. Consider documenting this limitation or using a more sophisticated version-detection regex if accuracy is important.
# Alternative approach - split on version-like patterns import re dir_name = re.split(r'-\d', path.name)[0] # Split on hyphen followed by digit
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/cli.pycortex/tarball_helper.pydocs/TARBALL_HELPER.mdtests/test_tarball_helper.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_tarball_helper.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/tarball_helper.pycortex/cli.py
🧬 Code graph analysis (2)
cortex/tarball_helper.py (1)
cortex/branding.py (1)
cx_header(82-88)
cortex/cli.py (1)
cortex/tarball_helper.py (5)
run_analyze_command(596-661)run_cleanup_command(713-721)run_install_deps_command(664-684)run_list_command(687-710)run_track_command(724-740)
🪛 GitHub Actions: CI
cortex/tarball_helper.py
[error] 51-51: ruff check failed: UP045 Use X | None for type annotations.
🪛 GitHub Check: lint
cortex/tarball_helper.py
[failure] 466-466: Ruff (UP045)
cortex/tarball_helper.py:466:46: UP045 Use X | None for type annotations
[failure] 51-51: Ruff (UP045)
cortex/tarball_helper.py:51:18: UP045 Use X | None for type annotations
🪛 GitHub Check: Lint
cortex/tarball_helper.py
[failure] 466-466: Ruff (UP045)
cortex/tarball_helper.py:466:46: UP045 Use X | None for type annotations
[failure] 51-51: Ruff (UP045)
cortex/tarball_helper.py:51:18: UP045 Use X | None for type annotations
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (8)
docs/TARBALL_HELPER.md (1)
1-283: LGTM! Comprehensive and well-structured documentation.The documentation thoroughly covers the Tarball Helper feature including usage examples, supported build systems, dependency mappings, and troubleshooting guidance. The content aligns with the implementation in
cortex/tarball_helper.py.cortex/cli.py (3)
604-685: LGTM! Well-structured command handler following existing patterns.The
tarball()method:
- Uses lazy imports for
cortex.tarball_helperto avoid impacting CLI startup time- Follows the same dispatch pattern as
sandbox()and other handlers- Includes proper docstring with type hints
- Provides helpful fallback help text when no action is specified
2357-2399: LGTM! Argument parser configuration is complete and consistent.The tarball subcommand parser follows the established patterns used for sandbox and other command groups. All five subcommands (analyze, install-deps, track, list, cleanup) are properly configured with appropriate arguments.
2662-2663: LGTM! Command routing correctly wired.The tarball command is properly routed to
cli.tarball(args)in the main dispatch block.cortex/tarball_helper.py (4)
56-93: LGTM! Dataclasses are well-defined.Proper use of
field(default_factory=list)for mutable default values, and comprehensive docstrings for all attributes.
167-198: LGTM! Initialization and build system detection are sound.The
__init__safely creates the config directory, anddetect_build_systemproperly validates the input and checks build files in a sensible priority order.
200-242: LGTM! Analysis flow is well-structured.The
analyzemethod properly dispatches to build-system-specific analyzers. Note thatBuildSystem.MAKEintentionally has no specialized analyzer since Makefiles don't have standardized dependency declarations.
724-740: LGTM! Track command is straightforward and correct.Properly creates a
ManualInstallrecord with current timestamp and delegates totrack_installation.
|
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@jeremylongshore Your SonarQubeCloud Quality Gate is failing kindly address that issues. Also address coderabbit comments that are still open.
2 participants
Summary
Implements the Tarball Helper requested in #452, providing 4 key features:
Features
find_package(),pkg_check_modules(),dependency(),AC_CHECK_*~/.cortex/manual_builds.jsonDemo
Test Coverage
Usage
AI Disclosure
This implementation was developed with assistance from Claude (Anthropic). The AI helped with:
All code was reviewed and the tests verify correct functionality.
Fixes #452
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.