Skip to content
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

Fixed the issue where OptiX could not correctly find the specified ve… #5588

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

xdestiny110
Copy link
Contributor

Fix #5566 .
@star-hengxing Feel free to make comments and thanks a lot!

local version = format("%s.%s.%s", math.floor(version_num/10000), math.floor(version_num%10000/100), version_num%100)
result.version = version
for k, v in pairs(paths) do
local inc = find_path("optix.h", v, {suffixes = "include"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为啥要加 for loop,find_path 原本支持 path array 同时查找

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果存在多个路径同时拥有optix.h的话,似乎find_path只会返回第一个结果
看find_path源码似乎找到第一个result后就直接return了
https://github.com/xmake-io/xmake/blob/6c2929bf34f87f0a865f747022537885bbd04f44/xmake/core/sandbox/modules/import/lib/detect/find_path.lua#L103

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不用改find部分,可以改前面insert部分,match $(env PROGRAMDATA)/NVIDIA Corporation/OptiX SDK *.*.*的时候提取版本号,不符合版本约束的路径不加入查找paths;用的时候还可以指定OptiX_ROOT环境变量,有这个变量的话会优先找这个

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

最后version加个验证倒也没问题,应该不会太影响速度

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

按照 @xq114 的建议进行了修改。麻烦再看看是否还有其他concern @waruqi

@xdestiny110 xdestiny110 force-pushed the bugfix/optix_support_multi_version branch from d4a6b06 to 95a173e Compare October 27, 2024 09:38
@xdestiny110
Copy link
Contributor Author

semver没有办法处理package:version_str()为lastest的情况,导致test case无法通过。在a131223进行了修复。麻烦 @waruqi 再重新review一下


local paths = {"$(env OptiX_ROOT)"}
if package:is_plat("windows") then
for _, dir in ipairs(os.dirs("$(env PROGRAMDATA)/NVIDIA Corporation/OptiX SDK *.*.*")) do
table.insert(paths, dir)
if package:version_str() == "latest" or semver.satisfies(dir:match("OptiX SDK (%d+%.%d+%.%d+)"), package:version_str()) then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

既然只匹配查找当前版本的 dir,为啥不直接在上面 os.dirs 里面 OptiX SDK *.*.* 直接改成版本呢,遍历都省了,直接一个路径 os.isdir 判断是否存在,不是更快么。。

如果是 lastest 再走 os.dirs 遍历

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

版本一般都是范围(7.x这种),取不到具体版本的情况更多

@xdestiny110
Copy link
Contributor Author

en....所有的UT都没有通过。我看了下之前与optix相关的mr,没有执行过test;测试脚本里面似乎看不出来有安装optix,用的镜像也是github默认的。看上去就是测试镜像中没有安装optix?这块应该怎么处理? @xq114

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


en....all UT failed. I looked at the previous mr related to optix, but I have not executed the test; it seems that optix is ​​not installed in the test script, and the image used is also the default one on github. It seems that optix is ​​not installed in the test image? How should this be handled? @xq114

@xq114
Copy link
Contributor

xq114 commented Oct 28, 2024

en....所有的UT都没有通过。我看了下之前与optix相关的mr,没有执行过test;测试脚本里面似乎看不出来有安装optix,用的镜像也是github默认的。看上去就是测试镜像中没有安装optix?这块应该怎么处理? @xq114

目前这类只能靠本地测试,本地测试没问题就行

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


en.... All UTs failed. I looked at the previous mr related to optix, but I have not executed the test; it seems that optix is ​​not installed in the test script, and the image used is also the default one on github. It seems that optix is ​​not installed in the test image? How should this be handled? @xq114

At present, this type can only be tested locally. As long as local testing is fine, it will be fine.

@xdestiny110
Copy link
Contributor Author

xdestiny110 commented Oct 28, 2024

本地执行 xmake l .\scripts\test.lua -vD optix能够正确运行。 @xq114 @waruqi

@xq114 xq114 merged commit e328ef3 into xmake-io:dev Oct 28, 2024
0 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

同时安装多个版本optix时,add_packages中添加版本限制报错
4 participants