Add support for uv and pnpm in check-requirements [--development]#429
Conversation
tmorrell
left a comment
tmorrell
left a comment
There was a problem hiding this comment.
Yup, this needs to go in before v14. I'm not sure if we want to be stricter with the requirements, but at a minimum it shouldn't fail with uv and pnmp
| ) | ||
| ) | ||
| ] | ||
|
|
There was a problem hiding this comment.
Do we want to do node_version = 24 here?
There was a problem hiding this comment.
added node_version = 24 for v14+
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
relaxed it to v22
There was a problem hiding this comment.
I will be testing some node variants in the docker image Monday so can comment on that further. It's not a blocker.
fenekku
left a comment
fenekku
left a comment
There was a problem hiding this comment.
General idea is good, most comments are minor/potential improvements. Am curious about the node version. If we can be accepting of wider range, it would be nice.
| 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) |
There was a problem hiding this comment.
minor (just me and seeing the repetition, obfuscation of clear matrix of parameters): could chunk it down (and similar for uv/pipenv)
| 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) | |
| # although this whole array could/should be passed as argument | |
| js_pkg_managers = [ | |
| {"binary": "pnpm", "major": 10, "minor": -1, "patch": -1}, | |
| {"binary": "npm", "major": 10, "minor": -1, "patch": -1}, | |
| ] | |
| for pkg_manager in js_pkg_managers: | |
| binary = pkg_manager["binary"] | |
| try: | |
| result = run_cmd([binary, "--version"]) | |
| version = cls._version_from_string(result.output.strip()) | |
| result = cls._check_version( | |
| binary, | |
| version, | |
| pkg_manager["major"], | |
| pkg_manager["minor"], | |
| pkg_manager["patch"], | |
| False | |
| ) | |
| if result.status_code == 0: | |
| break | |
| except Exception as err: | |
| return ProcessResponse(error=f"{binary} not found. Got {err}.", status_code=1) | |
| return result |
but with only 2 cases it's not that important.
There was a problem hiding this comment.
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.
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).
* previously, `invenio-cli check-requirements` would crash if `pipenv` is not installed
* previously we were displaying the parsed `parts` (integer array) instead of the original string representation of the `version`, which resulted in weird-looking output
fenekku
left a comment
fenekku
left a comment
There was a problem hiding this comment.
That's good for now and we can look into spring cleaning improvements as part of that other ticket.
3 participants
Since v14 has
uvandpnpmas the blessed tools, I think it makes sense to consider them forcheck-requirements.This also fixes some weird wording and handles the case better where the command is executed outside of a valid InvenioRDM project.