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

优化cmake支持 #70

Merged
merged 5 commits into from
Nov 19, 2024
Merged

Conversation

HIllya51
Copy link
Contributor

用了好久,感觉有几个地方不是很舒服,希望能优化一下:
1、vscode cmaketool按那个按钮编译是小写开头的"win32",匹配不上
2、有时候希望没有install的时候也能编译(这个不重要,您觉得不好的话可以删掉)
3、这个比较重要,就是一个大的项目下面有几个子项目,include会导致整个项目全都链接vcltl,但其实只希望其中某几个链接vcltl

@MouriNaruto
Copy link
Member

你看看能否改进成无视大小写的字符串比较,这样的话其他人也能写 ARM ARM64 啥的

毛利

@mingkuang-Chuyu mingkuang-Chuyu added 类型:新功能/建议(enhancement) New feature or request 处置:正在讨论(Review) 我们正在讨论如何解决,怎么实现更好。 labels Nov 11, 2024
@mingkuang-Chuyu
Copy link
Collaborator

mingkuang-Chuyu commented Nov 14, 2024

@xspeed1989 请帮忙review下。

  1. 帮忙评估一下更改是否影响早期VC-LTL版本的兼容性
  2. 额外添加的 INTERFACE是否存在一种不生效或者优先级比编译器内置CRT库低的可能性?
  3. option(VC_LTL_EnableCMakeInterface "VC_LTL_EnableCMakeInterface" OFF) 这种方式 CMake 3.5.2是否支持?

@HIllya51
Copy link
Contributor Author

好的

@HIllya51
Copy link
Contributor Author

  1. option(VC_LTL_EnableCMakeInterface "VC_LTL_EnableCMakeInterface" OFF) 这种方式 CMake 3.5.2是否支持?

3.5.2是肯定支持option的,option很早的版本就支持了 https://cmake.org/cmake/help/v3.5/command/option.html
1&2我就不太清楚了,等大佬评估。

if(VC_LTL_EnableCMakeInterface)
add_library(VC_LTL5 INTERFACE)
target_include_directories(VC_LTL5 SYSTEM BEFORE INTERFACE ${VC_LTL_Include})
target_link_directories(VC_LTL5 INTERFACE ${VC_LTL_Library})
Copy link
Collaborator

Choose a reason for hiding this comment

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

根据大胸review,target_link_directories需要3.13开始才支持,目前正在评估cmake minversion提升到3.13,是否会对大家造成困扰。

cmake.org/cmake/help/v3.13/command/target_link_directories.html

Copy link
Contributor Author

@HIllya51 HIllya51 Nov 14, 2024

Choose a reason for hiding this comment

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

其实还好,因为这个选项默认是关闭的,只要不开它即使3.5版也不会报错
不过其实可以glob遍历这个目录下的文件,然后用target_link_libraries来做,这样就不需要提升cmake版本了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我写的时候没注意cmake版本要求。如果需要的话,我可以改一下。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不过感觉3.5确实也有点老了,cmake版本也不是像目标系统那样不方便升级,所以要是决定提升版本的话那就更好了

Copy link
Collaborator

Choose a reason for hiding this comment

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

其实还好,因为这个选项默认是关闭的,只要不开它即使3.5版也不会报错 不过其实可以glob遍历这个目录下的文件,然后用target_link_libraries来做,这样就不需要提升cmake版本了

跟大胸沟通了下,早期的CMake遇到target_link_directories 应该会直接报错,又这个VC_LTL_EnableCMakeInterface判断也没用。一旦决定用target_link_directories,或许只能提升Cmake版本

@mingkuang-Chuyu mingkuang-Chuyu added 处置:等待发布(Publishing) 问题已经解决,等待新版本发布。 and removed 处置:正在讨论(Review) 我们正在讨论如何解决,怎么实现更好。 labels Nov 19, 2024
@mingkuang-Chuyu mingkuang-Chuyu changed the base branch from master to Fea/PendingPR November 19, 2024 06:17
@mingkuang-Chuyu mingkuang-Chuyu merged commit 35ba201 into Chuyu-Team:Fea/PendingPR Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
类型:新功能/建议(enhancement) New feature or request 处置:等待发布(Publishing) 问题已经解决,等待新版本发布。
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants