Skip to content

Commit

Permalink
Fix for issue#4801 (#4826)
Browse files Browse the repository at this point in the history
  • Loading branch information
@yfyeung
yfyeung committed Feb 20, 2023
1 parent 362ecc2 commit 59299d1
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion cmake/gen_cmake_skeleton.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ def gen_code(self):

if len(self.depends) > 0:
ret.append("target_link_libraries(" + self.target_name + " PUBLIC")
for d in self.depends:
for d in self.depends + ['-lcblas', '-llapack']:
ret.append(" " + d)
ret.append(")\n")

Expand Down

5 comments on commit 59299d1

@MrZyu
Copy link

@MrZyu MrZyu commented on 59299d1 Feb 20, 2023

Choose a reason for hiding this comment

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

大佬，有些相关kaldi的问题想请教您一下，方便留个邮箱或者其他什么的吗？

@yilmazay74
Copy link

Choose a reason for hiding this comment

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

[email protected]

我在哪兒見過你嗎?

@FredSRichardson
Copy link

Choose a reason for hiding this comment

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

@yfyeung - this change to cmake/gen_cmake_skeleton.py breaks the CMAKE build on Linux when you are using MKL as the MATHLIB (i.e. passing -DMATHLIB=MKL to CMAKE). I had to back out this change to get the MKL build to work. I don't know if there a way to check the setting for MATLIB inside this script before appending the cblas and lapack libraries to the target link library list.

@csukuangfj
Copy link
Contributor

Choose a reason for hiding this comment

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

@FredSRichardson

I don't know if there a way to check the setting for MATLIB inside this script before appending the cblas and lapack libraries to the target link library list.

Yes, there is a way. Please change

kaldi/cmake/gen_cmake_skeleton.py

Lines 270 to 274 in 71f38e6

if len(self.depends) > 0:
ret.append("target_link_libraries(" + self.target_name + " PUBLIC")
for d in self.depends + ['-lcblas', '-llapack']:
ret.append(" " + d)
ret.append(")\n")

to

        if len(self.depends) > 0:
            ret.append("target_link_libraries(" + self.target_name + " PUBLIC")
            for d in self.depends:
                ret.append("    " + d)
            ret.append(")\n")

        ret.append(f"""
if(NOT MATHLIB STREQUAL "MKL")
  target_link_libraries({self.target_name} PUBLIC -lcblas -llapack)
endif()
        """
        )

The following is the diff

diff --git a/cmake/gen_cmake_skeleton.py b/cmake/gen_cmake_skeleton.py
index c8fee4c41..758618a4d 100644
--- a/cmake/gen_cmake_skeleton.py
+++ b/cmake/gen_cmake_skeleton.py
@@ -269,10 +269,17 @@ class CMakeListsLibrary(object):

         if len(self.depends) > 0:
             ret.append("target_link_libraries(" + self.target_name + " PUBLIC")
-            for d in self.depends + ['-lcblas', '-llapack']:
+            for d in self.depends:
                 ret.append("    " + d)
             ret.append(")\n")

+        ret.append(f"""
+if(NOT MATHLIB STREQUAL "MKL")
+  target_link_libraries({self.target_name} PUBLIC -lcblas -llapack)
+endif()
+        """
+        )
+
         def get_test_exe_name(filename):
             exe_name = os.path.splitext(f)[0]
             if self.dir_name.startswith("nnet") and exe_name.startswith("nnet"):

If the above fix works for you, would you mind creating a pull-request?

@csukuangfj
Copy link
Contributor

Choose a reason for hiding this comment

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

大佬，有些相关kaldi的问题想请教您一下，方便留个邮箱或者其他什么的吗？

@MrZyu 推荐使用 由 Dan 主导开发的新一代 Kaldi https://github.com/k2-fsa

我们有活跃的微信交流群、QQ 交流群 (群号 744602236) 和 新一代 Kaldi 微信公众号。

Please sign in to comment.