[openfst] Add new port#27542
[openfst] Add new port#27542day253 wants to merge 11 commits intomicrosoft:masterfrom day253:openfst
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
dg0yt
left a comment
dg0yt
left a comment
There was a problem hiding this comment.
Just a quick review.
If the CMake config isn't from upstream, then
- the config shall be named (and installed as)
unofficial-openfst - the targets shall be exported to namespace
unofficial::openfst::
If there are no dependencies (which use targets), you don't need to include CMakeFindDependencyMacro, and you could directly export to unofficial-openfst-config.cmake.
(If you submitted the config change upstream, and it was accepted, then the unofficial prefix may be obsolete.)
MonicaLiu0311
added
the
category:new-port
|
@day253 |
This port is an unofficial mirror of openfst. And it's the most popular one. Can we use the official openfst name. |
Thank you very much for your contribution, but our review standard stipulates that duplicates must be checked. You can use other names, such as |
| find_library(FST_LIBRARY NAMES fst fstscript) | ||
| target_link_libraries(main PRIVATE ${FST_LIBRARY}) |
There was a problem hiding this comment.
I don't know why the reviewers asked to remove the include dir stuff. It is necessary to find the headers.
OTOH I would suggest to ensure that pkg-config info is valid on windows and on-windows, and describe cmake usage with pkg-config. Cf. port poppler for an example.
There was a problem hiding this comment.
I will follow the comments.
There was a problem hiding this comment.
@FrankXie05 Can you explain why you asked for the usage to be changed?
There was a problem hiding this comment.
@BillyONeal I think as header only, it is more intuitive.
There was a problem hiding this comment.
But it isn't header only if the user needs to add libraries to their link line?
There was a problem hiding this comment.
I doubt that this has been correctly verified. I'm irritated that this must be discussed at all. Basically you always have to deal with include directories. Only in the case of linking to CMake targets, this is handled implicitly via target properties. But here, we don't have targets, only find_library.
There was a problem hiding this comment.
I doubt that this has been correctly verified. I'm irritated that this must be discussed at all.
Re-test the two usages, and both failed:
usage 1:
find_path(FST_INCLUDE_DIRS "fst/fst.h")
find_library(FST_LIBRARY NAMES fst fstscript)
target_include_directories(main PRIVATE ${FST_INCLUDE_DIRS})
target_include_directories(main PRIVATE ${FST_INCLUDE_DIRS})
usage 2:
find_library(FST_LIBRARY NAMES fst fstscript)
target_include_directories(main PRIVATE ${FST_INCLUDE_DIRS})
Test Results:
~/CMakeFindUsage/build$ cmake --build .
[ 50%] Building CXX object CMakeFiles/CMakeFindUsage.dir/CMakeFindUsage.cpp.o
/home/vliumonica/CMakeFindUsage/CMakeFindUsage.cpp:3:10: fatal error: fst/fst.h: No such file or directory
3 | #include "fst/fst.h"
| ^~~~~~~~~~~
compilation terminated.
gmake[2]: *** [CMakeFiles/CMakeFindUsage.dir/build.make:76: CMakeFiles/CMakeFindUsage.dir/CMakeFindUsage.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/CMakeFindUsage.dir/all] Error 2
gmake: *** [Makefile:91: all] Error 2
Directory structure:
CMakeFindUsage/
CMakeFindUsage.cpp
CMakeLists.txt
build/
CMakeFindUsage.cpp
#include <iostream> #include "fst/fst.h"using namespace std;
int main()
{
cout << "Hello CMake." << endl;
return 0;
}
CMakeLists.txt
cmake_minimum_required (VERSION 3.8)set(CMAKE_TOOLCHAIN_FILE "/home/monica/openfst/scripts/buildsystems/vcpkg.cmake")
project ("CMakeFindUsage")
find_path(FST_INCLUDE_DIRS "fst/fst.h")
find_library(FST_LIBRARY NAMES fst fstscript)add_executable (CMakeFindUsage "CMakeFindUsage.cpp")
target_link_libraries(CMakeFindUsage PRIVATE ${FST_INCLUDE_DIRS})
target_link_libraries(CMakeFindUsage PRIVATE ${FST_LIBRARY})
I'm very sorry, the previous usage test may have passed because I didn't find the header file. This is my mistake, sorry again😶...In short, thanks very much for the contribution of @day253 and the review of @dg0yt 🌻
There was a problem hiding this comment.
It would be good to see the compile command line. Did you always start with a clean CMake cache (e.g. empty build dir)?
find_library(FST_LIBRARY NAMES fst fstscript)
This also looks questionable. By the names I would assume these are two independent libraries. But find_library will only return one of them. And we won't notice some linking errors unless we actually use symbols from those libs.
There was a problem hiding this comment.
It would be good to see the compile command line. Did you always start with a clean CMake cache (e.g. empty build dir)?
Yes, I empty build/ before every build.
There was a problem hiding this comment.
Re-test the two usages, and both failed:
usage 1:
find_path(FST_INCLUDE_DIRS "fst/fst.h") find_library(FST_LIBRARY NAMES fst fstscript)
This will find only one of those libraries. Are they really equivalent? Or did you want to use both?
target_include_directories(main PRIVATE ${FST_INCLUDE_DIRS}) target_include_directories(main PRIVATE ${FST_INCLUDE_DIRS})
The last line should be target_link_libraries(FST_LIBRARY) or similar.
usage 2:
find_library(FST_LIBRARY NAMES fst fstscript) target_include_directories(main PRIVATE ${FST_INCLUDE_DIRS})Test Results:
~/CMakeFindUsage/build$ cmake --build . [ 50%] Building CXX object CMakeFiles/CMakeFindUsage.dir/CMakeFindUsage.cpp.o /home/vliumonica/CMakeFindUsage/CMakeFindUsage.cpp:3:10: fatal error: fst/fst.h: No such file or directory 3 | #include "fst/fst.h" | ^~~~~~~~~~~ compilation terminated. gmake[2]: *** [CMakeFiles/CMakeFindUsage.dir/build.make:76: CMakeFiles/CMakeFindUsage.dir/CMakeFindUsage.cpp.o] Error 1 gmake[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/CMakeFindUsage.dir/all] Error 2 gmake: *** [Makefile:91: all] Error 2
Complete divergence between set variable and read variable, so totally broken.
FrankXie05
added
info:needs-maintainer-attention
BillyONeal
added
requires:author-response
and removed
info:needs-maintainer-attention
MonicaLiu0311
dismissed stale reviews from FrankXie05 and github-actions[bot]
via
75a76e3
|
Directory structure: 🔴usage 1: failed CMakeLists.txtcmake_minimum_required(VERSION 3.8) Configuremonica@monica003:~/CMakeFindUsage/build$ cmake .. -- The C compiler identification is GNU 11.3.0 -- The CXX compiler identification is GNU 11.3.0 -- Detecting C compiler ABI info -- Detecting C compiler ABI info - done -- Check for working C compiler: /usr/bin/cc - skipped -- Detecting C compile features -- Detecting C compile features - done -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Check for working CXX compiler: /usr/bin/c++ - skipped -- Detecting CXX compile features -- Detecting CXX compile features - done -- Configuring done -- Generating done -- Build files have been written to: /home/monica/CMakeFindUsage/build buildmonica@monica003:~/CMakeFindUsage/build$ cmake --build .
[ 50%] Building CXX object CMakeFiles/CMakeFindUsage.dir/CMakeFindUsage.cpp.o
/home/monica/CMakeFindUsage/CMakeFindUsage.cpp:3:10: fatal error: fst/fst.h: No such file or directory
3 | #include
| ^~~~~~~~~~~
compilation terminated.
gmake[2]: *** [CMakeFiles/CMakeFindUsage.dir/build.make:76: CMakeFiles/CMakeFindUsage.dir/CMakeFindUsage.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/CMakeFindUsage.dir/all] Error 2
gmake: *** [Makefile:91: all] Error 2
🟢usage 2: successful CMakeLists.txtcmake_minimum_required(VERSION 3.8) Configuremonica@monica003:~/CMakeFindUsage/build$ cmake .. -- The C compiler identification is GNU 11.3.0 -- The CXX compiler identification is GNU 11.3.0 -- Detecting C compiler ABI info -- Detecting C compiler ABI info - done -- Check for working C compiler: /usr/bin/cc - skipped -- Detecting C compile features -- Detecting C compile features - done -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Check for working CXX compiler: /usr/bin/c++ - skipped -- Detecting CXX compile features -- Detecting CXX compile features - done -- Configuring done WARNING: Target "CMakeFindUsage" requests linking to directory "/home/monica/openfst/installed/x64-linux/include". Targets may link only to libraries. CMake is dropping the item. -- Generating done -- Build files have been written to: /home/monica/CMakeFindUsage/build buildmonica@monica003:~/CMakeFindUsage/build$ cmake --build . [ 50%] Building CXX object CMakeFiles/CMakeFindUsage.dir/CMakeFindUsage.cpp.o [100%] Linking CXX executable CMakeFindUsage [100%] Built target CMakeFindUsage |
Do a unified reply: Please help with the final check, thanks again. |
I asked to reconsider this pattern, but it is presented again. |
@day253 |
|
Convert this PR to draft since there is no progress. Please ping us if this PR is ready for review again. |
|
Closing this PR since it seems that no progress is being made. Please ping us to reopen if work is still being done. |
5 participants
Describe the pull request
What does your PR fix?
Fixes [New Port Request] openfst #27541
Which triplets are supported/not supported? Have you updated the CI baseline?
!windows, Yes
Does your PR follow the maintainer guide?
Yes
If you have added/updated a port: Have you run
./vcpkg x-add-version --alland committed the result?Yes
If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/