-
Notifications
You must be signed in to change notification settings - Fork 47
Add support for uv and pnpm in check-requirements [--development]
#429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6cebc87
9202ff4
f2daeb5
c2af4b5
bd50822
9b260d8
2908782
43a6e1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ | |
|
|
||
|
|
||
| class RequirementsCommands(object): | ||
| """Pre-requirements check.""" | ||
| """Requirements check.""" | ||
|
|
||
| @classmethod | ||
| def _version_from_string(cls, string): | ||
|
|
@@ -63,7 +63,7 @@ def _check_version(cls, binary, version, major, minor=-1, patch=-1, exact=False) | |
| expected_version = f"{major}.{minor}.{patch}" | ||
|
|
||
| return ProcessResponse( | ||
| error=f"{binary} wrong version." f"Got {parts} expected {expected_version}", | ||
| error=f"{binary} wrong version. Got {version}, expected {expected_version}", | ||
| status_code=1, | ||
| ) | ||
|
|
||
|
|
@@ -79,13 +79,41 @@ def check_node_version(cls, major, minor=-1, patch=-1, exact=False): | |
| return ProcessResponse(error=f"Node not found. Got {err}.", status_code=1) | ||
|
|
||
| @classmethod | ||
| def check_npm_version(cls, major, minor=-1, patch=-1, exact=False): | ||
| def check_js_pkg_manager_version(cls, **kwargs): | ||
| """Check the available version of pnpm or npm.""" | ||
| response = cls.check_pnpm_version(**kwargs) | ||
| if response.status_code != 0: | ||
| response = cls.check_npm_version(**kwargs) | ||
|
|
||
| return response | ||
|
|
||
| @classmethod | ||
| def check_pnpm_version( | ||
| cls, pnpm_major, pnpm_minor=-1, pnpm_patch=-1, pnpm_exact=False, **kwargs | ||
| ): | ||
| """Check the pnpm version.""" | ||
| # Output comes in the form of '11.3.0\n' | ||
| try: | ||
| result = run_cmd(["pnpm", "--version"]) | ||
| version = cls._version_from_string(result.output.strip()) | ||
| return cls._check_version( | ||
| "PNPM", version, pnpm_major, pnpm_minor, pnpm_patch, pnpm_exact | ||
| ) | ||
| except Exception as err: | ||
| return ProcessResponse(error=f"PNPM not found. Got {err}.", status_code=1) | ||
|
|
||
| @classmethod | ||
| def check_npm_version( | ||
| cls, npm_major, npm_minor=-1, npm_patch=-1, npm_exact=False, **kwargs | ||
| ): | ||
| """Check the npm version.""" | ||
| # Output comes in the form of '6.14.13\n' | ||
| try: | ||
| result = run_cmd(["npm", "--version"]) | ||
| version = cls._version_from_string(result.output.strip()) | ||
| return cls._check_version("NPM", version, major, minor, patch, exact) | ||
| return cls._check_version( | ||
| "NPM", version, npm_major, npm_minor, npm_patch, npm_exact | ||
| ) | ||
| except Exception as err: | ||
| return ProcessResponse(error=f"NPM not found. Got {err}.", status_code=1) | ||
|
|
||
|
|
@@ -163,6 +191,39 @@ def check_git_version(cls, major, minor=-1, patch=-1, exact=False): | |
| status_code=1, | ||
| ) | ||
|
|
||
| @classmethod | ||
| def check_python_pkg_manager_installed(cls): | ||
| """Check for uv or pipenv being available.""" | ||
| response = cls.check_uv_installed() | ||
| if response.status_code != 0: | ||
| response = cls.check_pipenv_installed() | ||
|
|
||
| return response | ||
|
|
||
| @classmethod | ||
| def check_uv_installed(cls): | ||
| """Check the uv version.""" | ||
| # Output comes in the form of: | ||
| # 'uv 0.11.29 (901092ee1 2026-07-15 x86_64-unknown-linux-gnu)\n' | ||
| try: | ||
| result = run_cmd(["uv", "--version"]) | ||
| tool, version, *_ = result.output.split(" ", 2) | ||
| if tool != "uv": | ||
| return ProcessResponse( | ||
| error=f"UV not found. Got {tool}.", status_code=1 | ||
| ) | ||
| else: | ||
| return ProcessResponse( | ||
| output=f"UV OK. Got version {version}.", status_code=0 | ||
| ) | ||
|
|
||
| except FileNotFoundError: | ||
| return ProcessResponse(error="UV not found.", status_code=1) | ||
| except Exception as e: | ||
| return ProcessResponse( | ||
| error=f"Could not check for UV. Reason: {e}", status_code=1 | ||
| ) | ||
|
|
||
| @classmethod | ||
| def check_pipenv_installed(cls): | ||
| """Check the pipenv version.""" | ||
|
|
@@ -182,18 +243,50 @@ def check_pipenv_installed(cls): | |
| return ProcessResponse( | ||
| output=f"Pipenv OK. Got version {version}.", status_code=0 | ||
| ) | ||
| except Exception: | ||
| except FileNotFoundError: | ||
| return ProcessResponse(error="Pipenv not found.", status_code=1) | ||
| except Exception as e: | ||
| return ProcessResponse( | ||
| error=f"Pipenv not found. Got {result.error}.", status_code=1 | ||
| error=f"Could not check for pipenv. Reason: {e}", status_code=1 | ||
| ) | ||
|
|
||
| @classmethod | ||
| def report_error(cls, message): | ||
| """Function step that always fails with the specified message.""" | ||
| return lambda: ProcessResponse(error=message, status_code=1) | ||
|
|
||
| @classmethod | ||
| def check_dev(cls): | ||
| """Steps to check the development pre-requisites.""" | ||
| if rdm_version()[0] >= 12: | ||
| rdm_major_version = None | ||
| try: | ||
| if (version := rdm_version()) is not None: | ||
| rdm_major_version = version[0] | ||
| else: | ||
| return [ | ||
| FunctionStep( | ||
| func=cls.report_error( | ||
| "Could not find Invenio-App-RDM as dependency in the project definition." | ||
| ) | ||
| ) | ||
| ] | ||
|
|
||
| except FileNotFoundError as e: | ||
| return [ | ||
| FunctionStep( | ||
| func=cls.report_error( | ||
| f"Could not determine the version of Invenio-App-RDM: {e}" | ||
| ) | ||
| ) | ||
| ] | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to do node_version = 24 here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is 24 a hard minimum? 22 works too right but we "recommend" 24? If 22 works (or even something lower) would keep it to be as open. (we will only the build docker images for the recommended configuration though).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. relaxed it to v22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will be testing some node variants in the docker image Monday so can comment on that further. It's not a blocker. |
||
| if rdm_major_version >= 14: | ||
| node_version = 22 | ||
| npm_version = 10 | ||
| elif rdm_major_version >= 12: | ||
| node_version = 18 | ||
| npm_version = 10 | ||
| elif rdm_version()[0] >= 11: | ||
| elif rdm_major_version >= 11: | ||
| node_version = 16 | ||
| npm_version = 7 | ||
| else: | ||
|
|
@@ -208,9 +301,9 @@ def check_dev(cls): | |
| message="Checking Node version...", | ||
| ), | ||
| FunctionStep( | ||
| func=cls.check_npm_version, | ||
| args={"major": npm_version}, | ||
| message="Checking NPM version...", | ||
| func=cls.check_js_pkg_manager_version, | ||
| args={"npm_major": npm_version, "pnpm_major": 10}, | ||
|
fenekku marked this conversation as resolved.
|
||
| message="Checking if either PNPM or NPM are installed...", | ||
| ), | ||
| FunctionStep( | ||
| func=cls.check_imagemagick_version, | ||
|
|
@@ -236,8 +329,8 @@ def check(cls, development=False): | |
| message="Checking Python version...", | ||
| ), | ||
| FunctionStep( | ||
| func=cls.check_pipenv_installed, | ||
| message="Checking Pipenv is installed...", | ||
| func=cls.check_python_pkg_manager_installed, | ||
| message="Checking if either UV or Pipenv are installed...", | ||
| ), | ||
| FunctionStep( | ||
| func=cls.check_docker_version, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor (just me and seeing the repetition, obfuscation of clear matrix of parameters): could chunk it down (and similar for uv/pipenv)
but with only 2 cases it's not that important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created an issue motivating a general spring cleaning for this package (which may or may not be a bit dramatically worded): #430
Shall we consider this refactoring as part of that issue, as task for future us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!