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

Making ssplit-cpp self contained for bergamot project #36

Closed
abhi-agg opened this issue Feb 22, 2021 · 3 comments · Fixed by #45
Closed

Making ssplit-cpp self contained for bergamot project #36

abhi-agg opened this issue Feb 22, 2021 · 3 comments · Fixed by #45

Comments

@abhi-agg
Copy link
Contributor

Currently, users are required to have pcre2 library already installed on the system to be able to use ssplit-cpp in bergamot project. To be able to run it for wasm, we need the pcre2 library that is compiled for WASM and link it.

There are 2 ways to solve this issue:

  1. Either link against a pcre2 library that is compiled for wasm (it is a bad way of intergrating)
  2. Include pcre2 sources in ssplit-cpp in 3rd_party folder, build it and link it in ssplit-cpp as an alternative to linking with pre-installed pcre2 on system

2nd way is clearly more maintainable and clean solution.

I am ready to do this task and can submit a PR.
If @ugermann doesn't want to always compile pcre2 from sources then I am fine with that as well. I can add a cmake option USE_INTERNAL_PCRE2 which can enable/disable building pcre2 from sources.

This is blocking my work and we need to agree fast.

@kpu
Copy link
Member

kpu commented Feb 22, 2021

If you're volunteering, I will agree with option 2.

Also, actual question here: why is linking a bad way of integrating? I mean we link with libc right?

@abhi-agg
Copy link
Contributor Author

@kpu Option 2 done already via this PR. Just waiting for the merge.
@ugermann Can you please merge it?

@abhi-agg abhi-agg linked a pull request Mar 9, 2021 that will close this issue
@abhi-agg
Copy link
Contributor Author

abhi-agg commented Mar 9, 2021

Closing this as #45 is merged to main.

@abhi-agg abhi-agg closed this as completed Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants