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

Use golang native iconv library instead of gnu iconv #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

justinmichaelvieira
Copy link

@justinmichaelvieira justinmichaelvieira commented Apr 25, 2023

@hennedo, when you get a chance, can you take a look at this?

Removes github.com/qiniu/iconv in favor of github.com/justinmichaelvieira/iconv.

https://github.com/justinmichaelvieira/iconv is my fork of (current) https://github.com/mushroomsir/iconv, with CP850 support added (I would wager CP850 support was what you saw missing, when you attempted the same, so I added it in and added a test case on my fork; Opened a PR there as well, but for now the fork seems to work).

mushroomsir/iconv@6b19bf0 is the relevant commit adding CP850 to go native iconv lib.

Fixes #11

@justinmichaelvieira
Copy link
Author

@hennedo just pinging you as a gentle reminder to please take a look if you get a chance. I think removing the CGO dependency greatly increases the utility of this lib.

This was referenced Jan 16, 2024
@justinmichaelvieira
Copy link
Author

justinmichaelvieira commented May 16, 2024

@hennedo this should be very safe to merge at this point, a few other developers in this repo have said they have tested and are using my fork, which is equivalent to this PR, currently, just so they can save themselves hassle of compiling the gnu iconv dependency, and I've been using it successfully for >1yr at this point without issue.

@abdullah-hallaq
Copy link

When this can be marged

@justinmichaelvieira
Copy link
Author

When this can be marged

@abdullah-hallaq feel free to use my fork (https://github.com/justinmichaelvieira/escpos) until @hennedo gets a chance to review and merge it here. I have merged it there.

@abdullah-hallaq
Copy link

Thank you @justinmichaelvieira

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 this pull request may close these issues.

Libiconv dependency handling
2 participants