Fix Nimble find_package to use latest package list format#6996
Fix Nimble find_package to use latest package list format#6996waruqi merged 2 commits intoxmake-io:devfrom
Conversation
Summary of ChangesHello @tenten8401, 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 provides a critical update to the Nimble package manager integration, addressing a recent change in its command-line interface output. The modifications ensure that Xmake can continue to accurately detect and manage installed Nimble packages and their versions, maintaining compatibility and functionality for users relying on Nimble for dependency management. 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
|
There was a problem hiding this comment.
Code Review
This pull request updates the Nimble package finder to support a new output format from nimble list -i. The parsing logic for the new format is mostly correct, but I've found a couple of issues. The regular expression for parsing version strings is too restrictive and could fail on valid semantic versions with pre-release tags. Additionally, the logic for selecting a package version doesn't correctly pick the latest satisfying version when multiple are available. I've included suggestions to address these points to make the implementation more robust.
| end | ||
|
|
||
| -- match version lines like: └── @1.92.4.0 (...) or ├── @0.2.5 (...) | ||
| local version_str = line:match("^[├└].-@([%d%.]+)") |
There was a problem hiding this comment.
The regular expression [%d%.]+ used to capture the version string is too restrictive. It will not correctly parse versions that include pre-release tags (e.g., 1.0.0-alpha) or build metadata. To make this more robust, you should capture all non-whitespace characters following the @ symbol.
local version_str = line:match("^[├└].-@(%S+)")
| if package_version and (opt.require_version == "latest" or semver.satisfies(package_version, opt.require_version)) then | ||
| result = {version = package_version} | ||
| result = { version = package_version } | ||
| break | ||
| end |
There was a problem hiding this comment.
The current logic selects the first version that satisfies the requirements and then immediately stops searching. If multiple installed versions of a package satisfy the version constraint, this won't guarantee that the latest one is chosen. The loop should continue in order to find the highest version that meets the criteria.
Additionally, the if package_version and ... check on line 79 is redundant, as package_version is already verified to be non-nil on line 77.
if opt.require_version == "latest" or semver.satisfies(package_version, opt.require_version) then
if not result or semver.new(package_version):gt(result.version) then
result = { version = package_version }
end
end
There was a problem hiding this comment.
This is kinda out of my area of expertise :(
|
I don't know from which version of nimble changed its format, but I think we should prioritize compatibility with the output formats of earlier versions of nimble. You could first probe the output format and then parse them separately. |
Hi!
nimble list -ihas changed their CLI output in recent versions. It is now just a list of packages, no version number.The new way to call it is
nimble list -i --ver:This PR tries to parse find_package.lua so that it can parse the new package list format. Apologies if this PR is poorly written. I don't know Lua and had Kimi K2 help with most of it. I've given it a quick look over and it looks to be sound, and I tested the output with a valid version number I have installed and an invalid version number that does not exist yet and it seems to work well.
Hopefully you can give it a quick review to see if anything is obviously messed up. I apologize if so.