-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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] add lapack,libmct,mklml third_party cache #54326
[cmake] add lapack,libmct,mklml third_party cache #54326
Conversation
你的PR提交成功,感谢你对开源项目的贡献! |
set(LAPACK_DOWNLOAD_DIR | ||
${PADDLE_SOURCE_DIR}/third_party/lapack/${CMAKE_SYSTEM_NAME}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里是为了防止,有用户会在docker和本机同时编译所以在这里以系统分文件夹, 比如在macos同时使用docker和本机
目录结构
├── ...
└── lapack/
├── .../
|── Darwin/
| ├── ...
| └──xxx.gz
└── Linux/
├── ...
└──xxx.gz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a great idea, and we didn't think about that before
COMMAND ${CMAKE_COMMAND} -E copy ${LIBMCT_DOWNLOAD_DIR}/CMakeLists.txt | ||
${LIBMCT_INSTALL_DIR} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里用拷贝是因为当make -j
时会清空build/third_parth/install/libmct
文件夹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,there are some problems with the Coverage CI, but they are not caused by your PR.I helped you rerun , after CI passed to help you merge
set(LAPACK_DOWNLOAD_DIR | ||
${PADDLE_SOURCE_DIR}/third_party/lapack/${CMAKE_SYSTEM_NAME}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a great idea, and we didn't think about that before
If cross compilation is supported in the future, Then it is necessary to continue distinguishing between CPU architectures |
cmake/external/lapack.cmake
Outdated
@@ -49,10 +50,10 @@ elseif(WIN32) | |||
set(LAPACK_LIB "${LAPACK_LIB_DIR}/liblapack.dll") | |||
else() | |||
set(LAPACK_VER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个如果把版本改成带后缀的文件名,那最好就换个变量名,比如LAPACK_FILE之类
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
PREFIX ${LAPACK_PREFIX_DIR} | ||
DOWNLOAD_NO_PROGRESS 1 | ||
PATCH_COMMAND "" | ||
UPDATE_COMMAND "" | ||
CONFIGURE_COMMAND "" | ||
BUILD_COMMAND "" | ||
INSTALL_COMMAND ${CMAKE_COMMAND} -E copy_directory ${LAPACK_SOURCE_DIR} | ||
${LAPACK_LIB_DIR} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个直接删除了没有影响吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里没问题,因为指定了SOURCE_DIR
直接就解压到原先的LAPACK_LIB_DIR
里面了
PR types
Others
PR changes
Others
Description
添加
lapack
,libmct
,mklml
库编译缓存相关链接: