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

vcpkg dynamic triplet #5023

Merged
merged 8 commits into from
Apr 28, 2024
Merged

vcpkg dynamic triplet #5023

merged 8 commits into from
Apr 28, 2024

Conversation

SHIINASAMA
Copy link
Contributor

目前完成了 #5021 解决方案中的前两点

@@ -2850,6 +2850,11 @@ function target.linkname(filename, opt)
if count == 0 and opt.plat == "mingw" then
linkname, count = filename:gsub(target.filename("__pattern__", "static", {plat = "windows"}):gsub("%.", "%%."):gsub("__pattern__", "(.+)") .. "$", "%1")
end
-- for custom shared libraries name, xxx.so, xxx.dylib
if filename:endswith(".so") or filename:endswith(".dylib") then
Copy link
Member

Choose a reason for hiding this comment

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

不是这么改的,这里原本就会对 .so/.dylib 处理,不需要加这个硬编码

Copy link
Member

Choose a reason for hiding this comment

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

$ xmake l core.project.target.linkname libfoo.dylib shared
"foo"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

前面几个 case 其实也看过了,这里如果库名不为 libxxx.so,比如没有前缀 lib, 那么 xxx.so 将无法被识别为 xxx。
我把这个部分的放到所有判断分支的最后,是希望能兼容这种不是很讲规矩的命名。

Copy link
Member

Choose a reason for hiding this comment

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

那应该先得判断 之前的 linkname 没被匹配到,而不是直接覆盖。。另外,不要直接 return ,也不要加 ;

统一走 linkname 返回,并且这种不规范的也得严格检测。。if not filename:startswith("lib")

triplet = triplet .. "-dynamic"
end
elseif plat == "macosx" then
if arch == "x64" and configs.shared == true then
Copy link
Member

Choose a reason for hiding this comment

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

直接 if configs.shared then 就行了,不用加 == true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

哦哦,随手写的可能没太注意

@SHIINASAMA SHIINASAMA requested a review from waruqi April 27, 2024 15:11
return filename;
if count == 0 and not filename:startswith("lib") then
count = count + 1
linkname = filename
Copy link
Member

Choose a reason for hiding this comment

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

最好再判断下后缀名

@waruqi waruqi changed the base branch from master to dev April 27, 2024 15:36
@@ -56,6 +56,14 @@ function triplet(configs, plat, arch)
if configs.runtimes and configs.runtimes:startswith("MD") then
triplet = triplet .. "-md"
end
elseif plat == "linux" then
if (arch == "x64" or arch == "x86") and configs.shared then
Copy link
Member

Choose a reason for hiding this comment

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

我感觉你这都没测试过,确定 ok? linux/macosx 下应该是 x86_64/i386 目前

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个我昨天仔细看了一下,arch 本身是用来拼接 triplet 字符串的,应该就是对应 vcpkg 的架构,至于这个 x86_64/i386 可能是 xmake 自己的 arch 分类?
细看才发现原来不支持 x86-linux-dynamic,同时 macos 下的 plat 应该是 osx,同时我直接挨个对照现有的 triplet 改了一次。
至于测试,我这只能测 linux 的设备,手头上暂时没有 mac,mac 的虚拟机也出问题了...

Copy link
Member

Choose a reason for hiding this comment

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

arch 那个昨天我看错了,应该没问题。。那现在 linux 下测试 ok 么?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

某种意义上也没错,你说了之后昨天检查确实也有问题

测试的话,我这边 linux 下是 ok 的。

$ xmake l core.project.target.linkname libfoo.so shared
"foo"
$ xmake l core.project.target.linkname foo.so shared
"foo.so"

Copy link
Member

Choose a reason for hiding this comment

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

如果没啥问题,那可以准备 merge 了。。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

要说的话问题只剩 #5021 中的第三点,设置环境变量的问题,我不知道这个不完美的解决方案是否能作为过渡。我不知道在 xmake run 命令下如何获取到 vcpkg install 目录,我暂时没有能力解决这个问题。

@waruqi
Copy link
Member

waruqi commented Apr 27, 2024

建议改完 先自己这边测试通过,再提 pr ,避免 break 现有的一些逻辑

@Issues-translate-bot
Copy link

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


It is recommended that after the modification, you first pass the test yourself, and then submit the PR to avoid breaking some existing logic.

@SHIINASAMA SHIINASAMA marked this pull request as draft April 27, 2024 16:55
no triplet named x86-linux-dynamic
and the plat on macos is osx
@SHIINASAMA SHIINASAMA marked this pull request as ready for review April 28, 2024 08:01
@waruqi waruqi merged commit 5f1a07e into xmake-io:dev Apr 28, 2024
18 of 19 checks passed
@waruqi waruqi added this to the v2.9.2 milestone Apr 28, 2024
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.

3 participants