Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the project version, refactors CMake configurations for better portability and Windows support, and adjusts the Python package structure. I have provided feedback regarding the removal of hardcoded local paths in CMake files, a potential issue with RPATH construction, a suggestion to use relative imports in Python, and a concern regarding the breaking change to the package distribution name.
| $ENV{HOME}/Downloads/kaldifst-1.8.0.tar.gz | ||
| ${CMAKE_SOURCE_DIR}/kaldifst-1.8.0.tar.gz | ||
| ${CMAKE_BINARY_DIR}/kaldifst-1.8.0.tar.gz | ||
| /tmp/kaldifst-1.8.0.tar.gz | ||
| /star-fj/fangjun/download/github/kaldifst-1.8.0.tar.gz |
There was a problem hiding this comment.
The list of possible file locations contains a hardcoded absolute path that is specific to a local development environment. This should be removed to ensure the build remains portable and does not leak local system information.
$ENV{HOME}/Downloads/kaldifst-1.8.0.tar.gz
${CMAKE_SOURCE_DIR}/kaldifst-1.8.0.tar.gz
${CMAKE_BINARY_DIR}/kaldifst-1.8.0.tar.gz
/tmp/kaldifst-1.8.0.tar.gz
| endif() | ||
|
|
||
| if(NOT WIN32) | ||
| target_link_libraries(_kaldi_decoder PRIVATE "-Wl,-rpath,${KALDI_DECODER_RPATH_ORIGIN}/kaldi_decoder/lib") |
There was a problem hiding this comment.
The rpath construction appears to be incorrect. Since the library _kaldi_decoder is installed directly into the lib directory (i.e., kaldi_decoder/lib/ relative to the prefix), ${KALDI_DECODER_RPATH_ORIGIN} (which expands to $ORIGIN on Linux or @loader_path on macOS) already points to that directory. Appending /kaldi_decoder/lib creates a non-existent nested path like .../kaldi_decoder/lib/kaldi_decoder/lib.
target_link_libraries(_kaldi_decoder PRIVATE "-Wl,-rpath,${KALDI_DECODER_RPATH_ORIGIN}")
| @@ -1,4 +1,4 @@ | |||
| from _kaldi_decoder import ( | |||
| from kaldi_decoder.lib._kaldi_decoder import ( | |||
There was a problem hiding this comment.
Using a relative import is more idiomatic and robust here. It avoids hardcoding the package name, which makes the code more maintainable if the package is ever renamed or vendored.
| from kaldi_decoder.lib._kaldi_decoder import ( | |
| from .lib._kaldi_decoder import ( |
|
|
||
|
|
||
| package_name = "kaldi-decoder" | ||
| package_name = "kaldi_decoder" |
There was a problem hiding this comment.
Changing the distribution name from kaldi-decoder to kaldi_decoder is a breaking change for users who install the package via PyPI or depend on it in requirements.txt. It is also inconsistent with the repository name and the project URL. Unless this change is intentional for a specific branding reason, it is recommended to keep the hyphenated name for the distribution.
| package_name = "kaldi_decoder" | |
| package_name = "kaldi-decoder" |
1 participant
No description provided.