fix: attempt to add dependency handling for vcpkg#7299
fix: attempt to add dependency handling for vcpkg#7299waruqi merged 10 commits intoxmake-io:devfrom
Conversation
Summary of ChangesHello @138436724, 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 significantly improves Xmake's integration with Vcpkg by introducing robust dependency handling. Previously, Xmake might only process the primary requested package; now, it intelligently queries Vcpkg for all associated dependencies and incorporates their respective build information (like include paths and libraries) into the project configuration. This change streamlines the use of complex Vcpkg packages, ensuring that all necessary components are correctly linked and available during the build process. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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 attempts to add dependency handling for vcpkg, which is a valuable improvement. However, the current implementation introduces several critical logical flaws. There's a lack of error handling when calling os.iorunv, which could lead to a crash. Additionally, the logic for aggregating results from dependencies is incorrect, causing information from previously processed dependencies to be overwritten. I've left specific comments with suggestions on how to address these issues.
| if #pkgconfig_files > 0 then | ||
| local pkgconfig_result = _find_package_from_pkgconfig(pkgconfig_files, {installdir = installdir, linkdirs = result and result.linkdirs}) | ||
| if pkgconfig_result then | ||
| result = pkgconfig_result |
There was a problem hiding this comment.
Inside the loop over dependencies, result = pkgconfig_result completely overwrites the result table. This means that for each dependency that has a .pc file, all previously collected dependency information (like includedirs, linkdirs from other dependencies) is discarded. You should merge pkgconfig_result into the existing result table instead of replacing it.
| if not result.version then | ||
| result.version = infoname:match(prefix .. "_(%d+%.?%d*%.-)_" .. arch) | ||
| -- save version | ||
| if result then |
There was a problem hiding this comment.
The version of the package is being overwritten in each iteration of the dependency loop. This will result in result.version holding the version of the last processed dependency, which is likely not what's intended. The version should probably be set only for the main package being sought (name).
if result and dependencyname == name then
ea99b91 to
7a6078c
Compare
|
试了下，感觉不太对，这些依赖库的链接顺序都没处理，完全反的。。 现在是完全按照 depend-info 输出的遍历顺序提取的 key 作为的链接顺序 而正确的顺序应该是 另外，不应该仅仅解析 你参考下这里的实现 要根据实际每个子库的依赖关系， 去构建 DAG 图，然后生成最终的依赖顺序。 根据这个依赖顺序，去查找每个子库的 links ，来追加 links 。
links = {
"bz2",
"lzma",
"zlib",
"zstd",
"libexpat",
"imath-3_2",
"deflate",
"jpeg",
"turbojpeg",
"minizip-ng",
"pystring",
"yaml-cpp",
"openjph.0.26",
"fmt",
"libpng16",
"opencolorio",
"iex-3_4",
"ilmthread-3_4",
"openexr-3_4",
"openexrcore-3_4",
"openexrutil-3_4",
"tiff",
"openimageio",
"openimageio_util"
}, |
|
目前生成的链接顺序是不对的，尽管在 windows 上没问题，但是在 linux/macos ，肯定会出问题。
|
I tried it, but it didn't feel right. The link order of these dependent libraries was not processed, it was completely the opposite. . Now the key is extracted exactly according to the traversal order output by depend-info as the link order. The correct order should be In addition, you should not only parse the key in Please refer to the implementation here According to the actual dependencies of each sub-library, To build a DAG graph and then generate the final dependency order. According to this dependency order, search the links of each sub-library to append links.
links = {
"bz2",
"lzma",
"zlib",
"zstd",
"libexpat",
"imath-3_2",
"deflate",
"jpeg",
"turbojpeg",
"minizip-ng",
"pystring",
"yaml-cpp",
"openjph.0.26",
"fmt",
"libpng16",
"opencolorio",
"iex-3_4",
"ilmthread-3_4",
"openexr-3_4",
"openexrcore-3_4",
"openexrutil-3_4",
"tiff",
"openimageio",
"openimageio_util"
}, |
|
vcpkg的depend-info本身是有顺序的，那个顺序从上到下应该就是依赖需要的顺序。如果需要反过来，是否可以将解析到的links循环插入到result.links头部来解决。 |
The depend-info of vcpkg itself is in order, and the order from top to bottom should be the order required for dependencies. If the reverse is needed, can the parsed links loop be inserted into the result.links header to solve the problem? |
看着像是有序的，可以明确指定下顺序， 你可以改成 |
|
已添加 |
Added |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces dependency handling for vcpkg by leveraging vcpkg depend-info, which is a significant improvement. The implementation is well-structured, including a refactoring of the package finding logic and the addition of a new test case to validate the changes. I have identified one potential high-severity issue regarding the use of table.unwrap that could lead to unexpected behavior, and a minor typo in a test file comment. Overall, this is a valuable contribution.
|
我发现了一个问题，vcpkg还可以加入 如果是不同的类型的包写在一起或许还得加入一下这个参数来进行查询。 我认为try/catch是没必要的，因为如果没有这个包，上面infofile就会直接return，而有的包vcpkg一定能查询到。 |
I found a problem, vcpkg can also add the If different types of packages are written together, you may need to add this parameter for query. I think try/catch is unnecessary, because if there is no such package, the above infofile will return directly, and some packages vcpkg can definitely be queried. |
凡事都要兜底，没有 100% 一定成功的情况，比如： vcpkg 后续升级更新导致 depend-info 参数改动给 break 了，比如 infofile 路径格式变动，导致后续判断失效，比如用户使用了很低的 vcpkg 版本，还没有 depend-info 支持。。各种情况都要考虑，不可能完全 100% 。 凡事皆有可能，像 conan 2.0 还魔改了呢，break 了一堆 |
Everything must be done carefully, and there is no 100% guarantee of success. For example: subsequent upgrades and updates to vcpkg cause the depend-info parameter to change to break. For example, the infofile path format changes, causing subsequent judgments to become invalid. . All situations must be considered and it is impossible to be 100% sure. Everything is possible. For example, conan 2.0 has been modified and broken a lot. |
|
1失败和6跳过，那个失败看起来像是系统环境的问题，跳过不知道是什么原因。我需要rebase到最新的提交然后强制推送一下吗 |
|
不用 |
1 fails and 6 skips. That failure seems to be a problem with the system environment. I don’t know why it is skipped. Do I need to rebase to the latest commit and then force push it? |
Need not |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Attempt to add dependency handling for vcpkg
#7297