-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add lib stemmer in setup scripts #10984
Conversation
✅ Deploy Preview for meta-velox canceled.
|
45710db
to
e48a0e4
Compare
@kgpai, I just broke down my reverted pr into two. This one only covers the installation of libstemmer and I will create another pr to add Findstemmer.cmake. Could you please review this pr? Thanks! |
e48a0e4
to
472c14c
Compare
7c453ff
to
8a7416b
Compare
Friendly ping @kgpai, could you review this pr? The CI failure is not related. |
@assignUser, could you take a look also? |
15523b1
to
699808a
Compare
@kgpai, @majetideepak, thanks so much for your comments! Could you take a look again? |
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.
Thanks, @PHILO-HE
@PHILO-HE should we add this for macos setup script as well? |
+1 on adding it to macos script as well |
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.
Thanks! The redundancy will be cleaned up with #10670
I just noticed that the macos jobs where not triggered, could you add |
@assignUser, yes, we should add it. Just updated the pr. |
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
50a83a6
to
cc5694e
Compare
@Yuhta, I just resolved code conflict. Could you please re-import the pull request? |
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@PHILO-HE Can you rebase again? There was another PR changing the same file |
cc5694e
to
a1ee399
Compare
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Summary: Lib stemmer can be installed through setup script. See #10984. This pr allows cmake to find installed lib stemmer. Pull Request resolved: #10998 Reviewed By: xiaoxmeng, DanielHunte Differential Revision: D63569657 Pulled By: kagamiori fbshipit-source-id: e0e0c2402616a7e9f194665363bcfc0208303fe6
The early merged pr (38f9a1f) was reverted by #10965 due to this issue: #10963.
In the original impl., a patch is expected to be applied to add
-fPIC
before building libstemmer, but when building docker image (ghcr.io/facebookincubator/velox-dev:centos9),
that patch file is not available to use due to the copied setup script outside velox repo is
executed. See code:
velox/scripts/centos.dockerfile
Line 22 in 7f2d7ad
This pr proposes the installation of lib stemmer with the above issue fixed.