-
-
Notifications
You must be signed in to change notification settings - Fork 487
kenlm: add package. #8179
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
kenlm: add package. #8179
Conversation
Summary of ChangesHello @Redbeanw44602, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates the 'kenlm' library into the build system as a new package. The changes define how 'kenlm' is sourced, built, and configured across different platforms, including its dependencies and specific build flags. This addition aims to make the 'kenlm' library readily available for other projects, streamlining its consumption and ensuring a consistent build process. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request adds a new package for kenlm. The implementation is mostly correct, but I've found a few issues. There seems to be a typo in the version string. The configuration for shared libraries is incorrect and prevents building shared libraries on platforms other than Windows. I've also suggested an improvement to the include path handling to follow best practices and avoid polluting the global include namespace. Please see my detailed comments for suggestions.
|
I can't reproduce mingw32 ci error🤔 |
|
I use the artifact's binary, and it can still link successfully locally. |
|
I think I found the problem... for some reason, mingw32 in actions uses the wrong calling convention and generates the wrong symbols.
|
|
I solved this problem.
Compile command: Apparently, bzip2 was not included in the header search directory list, which should have been considered a problem, but we ignored it :( Then comes the most incredible thing I think. The packager of MSYS2 actually modified the header file of bzip2. I totally cannot understand why they did this! So, the fix is obvious. First we should add packagedeps and then patch bzip2 to make it consistent with MSYS2 system package manager (although I really don't like doing that). Attachment: https://fars.ee/5W1t (Compiler output after adding the |
|
Seems xmake dosen't fetch the system bzip2, and package cmake find the system bzip2, not good😡 |
5127a35 to
5fe5ba8
Compare
No description provided.