Fix install dependencies #19
Conversation
| MINIMUM_SPARSE_VERSION = "0.6.4" | ||
|
|
||
| class SparseDependency(Dependency): | ||
| class InstallDependency(Dependency): |
There was a problem hiding this comment.
If this is no longer specific to sparse.py, these changes should be made to class Dependency directly.
There was a problem hiding this comment.
I figured it would be nice to keep these outside of class Dependency for customization.
| def install_sparse_from_source() -> None: | ||
| """ | ||
| Install sparse from source. | ||
| """ | ||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| src_dir = os.path.join(tmpdir, "sparse") | ||
| subprocess.run(["git", "clone", "git://git.kernel.org/pub/scm/devel/sparse/sparse.git", src_dir], check=True) | ||
| subprocess.run(["make"], cwd=src_dir, check=True) | ||
| subprocess.run(["sudo", "make", f"PREFIX={SANDBOX_PATH}", "install"], cwd=src_dir, check=True) | ||
|
|
||
| def install_llvm_from_source() -> None: | ||
| pass |
There was a problem hiding this comment.
Would it make more sense to make these functions overwrite Dependency.install_from_source() instead?
There was a problem hiding this comment.
I added these functions here to have the option to add custom install from source instructions.
|
I noticed that clang is being installed in ai_code_review.py and also in sparse.py. Should they be put together or stay separate? |
They should stay separate. For example, situations such as ai_code_review requiring clang<=20 and sparse requiring clang>=14, each one should be able to define its own requirements. This brings up a interesting dilemma, what happens if sparse installs clang-22 and then ai_code_review.install() runs and installs clang-12 (for whatever reason)? It's possible for there to be no intersection between the ranges of the compatible versions of certain dependencies, and we don't take that into account, we just run review.install() and continue, and never check to make sure none of the subsequent reviews install an version that's incompatible with the first review. It might be necessary to redesign the dependency check, maybe we can iterate through each reviews' dependencies and create a set (set, not list) of dependencies, that way we can overlay the ranges of compatible versions and ensure that there's an intersection between them. Altneratively, we can create a per-PatchReview bin directory where all of its dependencies are installed, and in |
Signed-off-by: David Lucman <dlucman@qti.qualcomm.com>
davidlucman
force-pushed
the
install-dependencies-07-30
branch
from
9439323 to
806c184
Compare
| self, | ||
| name: str, | ||
| install_name: str = "", | ||
| source: str = "", |
There was a problem hiding this comment.
source is not a string, right? It's supposed to be a function?
| def __init__( | ||
| self, | ||
| name: str, | ||
| install_name: str = "", |
There was a problem hiding this comment.
| install_name: str = "", | |
| package_name: str = "", |
There was a problem hiding this comment.
There's a way to define optional strings with typing
| except ImportError as e: | ||
| self.logger.warning(f"{e}") | ||
| self.logger.info(f"Installing {self.name} from source...") | ||
| self.source() |
There was a problem hiding this comment.
self.source isn't being defined in ai_code_review.py
|
|
||
| DEPENDENCIES = getattr(AiReview, "DEPENDENCIES", []) + [ | ||
| Dependency( | ||
| InstallDependency( |
There was a problem hiding this comment.
self.source isn't being defined in ai_code_review.py
| max_version: Union[int, float, str, None] = None, | ||
| ): | ||
| self.name = name | ||
| self.install_name = install_name if install_name else name |
There was a problem hiding this comment.
| self.install_name = install_name if install_name else name | |
| self.install_name = install_name or name |
| ): | ||
| self.name = name | ||
| self.install_name = install_name if install_name else name | ||
| self.source = source |
There was a problem hiding this comment.
Instead of taking a function as an argument, just define the installation function in the subclasses that need it and call it from _do_install()
| # method_name = ( | ||
| # f"{self.__class__.__name__}.{inspect.currentframe().f_code.co_name}" | ||
| # ) | ||
| # raise NotImplementedError( | ||
| # f"{self.__class__.__name__} must implement {method_name}." | ||
| # ) |
There was a problem hiding this comment.
| # method_name = ( | |
| # f"{self.__class__.__name__}.{inspect.currentframe().f_code.co_name}" | |
| # ) | |
| # raise NotImplementedError( | |
| # f"{self.__class__.__name__} must implement {method_name}." | |
| # ) |
|
Do you think we should add |
Good point, there's definitely value in having a shared Dependency object for LLVM between reviews, let's move |
|
I'm planning to reorganize the Dependency class. First, I'll move it into its own file and turn it into a minimal template class. Then, I'll create specific subclasses like SourceDependency and PackageDependency that inherit from it. This structure will allow us to support different methods for installing custom dependencies. |
|
Mark the PR as a draft until you're ready for review. |
|
Replaced by #40 |
2 participants
No description provided.