Skip to content
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] New Port #13692

Merged
merged 25 commits into from
Oct 20, 2020
Merged

[kenlm] New Port #13692

merged 25 commits into from
Oct 20, 2020

Conversation

jacobkahn
Copy link
Contributor

@jacobkahn jacobkahn commented Sep 23, 2020

Adds a port for the the KenLM library.

Things to note:

  • KenLM doesn't support a CMake install target, so files are manually globbed as needed.
  • KenLM appears to only build static libs.
  • The KenLM repo doesn't have tags, so I've used the current commit on top of master.
  • Adds bin tools as built by the library.

Which triplets are supported/not supported? Have you updated the CI baseline?

Tested x64-linux.

Does your PR follow the maintainer guide?

Yes.

cc @kpu

@ghost
Copy link

ghost commented Sep 23, 2020

CLA assistant check
All CLA requirements met.

@jacobkahn jacobkahn changed the title KenLM port [kenlm] New Port Sep 23, 2020
@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Sep 24, 2020
ports/kenlm/vcpkg.json Outdated Show resolved Hide resolved
ports/kenlm/portfile.cmake Outdated Show resolved Hide resolved
ports/kenlm/portfile.cmake Outdated Show resolved Hide resolved
ports/kenlm/portfile.cmake Outdated Show resolved Hide resolved
ports/kenlm/portfile.cmake Outdated Show resolved Hide resolved
ports/kenlm/portfile.cmake Outdated Show resolved Hide resolved
ports/kenlm/portfile.cmake Outdated Show resolved Hide resolved
ports/kenlm/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor

JackBoosY commented Sep 24, 2020

  • KenLM doesn't support a CMake install target, so files are manually globbed as needed.

We need to patch it's cmake files.

ports/kenlm/vcpkg.json Outdated Show resolved Hide resolved
Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

Please make sure the headers are all installed.

@JackBoosY JackBoosY marked this pull request as ready for review September 24, 2020 08:29
@kpu
Copy link

kpu commented Sep 24, 2020

Hi! You're welcome to drive by and patch my cmake.

@jacobkahn
Copy link
Contributor Author

@JackBoosY @kpu thanks very much for both of your time working on this — where do we stand now? It looks like Port CI is failing on Linux...

@kpu
Copy link

kpu commented Oct 1, 2020

The linux build is failing on https://dev.azure.com/vcpkg/public/_build/results?buildId=43506&view=logs&j=d3670d0c-cc82-50c9-762c-eccde036829f&t=b5ba9409-7383-59d6-95ee-7a65895d5946&l=2182 complaining it is missing streaming_example from the interpolation code. This works in master...

@kpu
Copy link

kpu commented Oct 1, 2020

I think the only patch you still need is IF( WIN32 AND BUILD_SHARED_LIBS) which I wouldn't do because it broke continuous integration on windows.

@JackBoosY
Copy link
Contributor

I'm OOF now, will continue this PR when I return.

@JackBoosY
Copy link
Contributor

@kpu Tool streaming_example is gone in *inx ?

@kpu
Copy link

kpu commented Oct 11, 2020

streaming_example is more documentation about how to use the code as a library, compiled to check that it still works, rather than an actually useful executable worth installing.

@JackBoosY
Copy link
Contributor

JackBoosY commented Oct 12, 2020

@kpu, fine. So I'm done. Do you have any suggestions about this PR?

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Oct 13, 2020
@jacobkahn
Copy link
Contributor Author

Gentle bump, cc @kpu — are we ok to merge?

Fix interpolate + Windows error desc

Co-authored-by: Billy O'Neal <bion@microsoft.com>
@jacobkahn
Copy link
Contributor Author

jacobkahn commented Oct 19, 2020

@JackBoosY — are we ok to merge this? I think we need an accept from you.

@JackBoosY
Copy link
Contributor

@BillyONeal Ping for merge this PR.

@BillyONeal BillyONeal merged commit 8ab8add into microsoft:master Oct 20, 2020
@BillyONeal
Copy link
Member

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants