Skip to content

Support custom encodings on Windows through GNU libiconv#11480

Merged
straight-shoota merged 4 commits into
crystal-lang:masterfrom
HertzDevil:feature/windows-libiconv
Nov 26, 2021
Merged

Support custom encodings on Windows through GNU libiconv#11480
straight-shoota merged 4 commits into
crystal-lang:masterfrom
HertzDevil:feature/windows-libiconv

Conversation

@HertzDevil
Copy link
Copy Markdown
Contributor

@HertzDevil HertzDevil commented Nov 20, 2021

This PR adds support for IOs with custom encodings on Windows, using https://github.com/pffang/libiconv-for-Windows, which wraps GNU libiconv in an MSVC project.

To make things work more generically across platforms, there is now a new flag -Dwithout_iconv which disables both libiconv and the built-in iconv_* functions from the C library, plus all the associated tests; any attempt to use custom encodings will raise during runtime, similar to Windows before this PR. This is useful when the target platform has a poor iconv implementation (e.g. Android NDK) and libiconv is not yet available on that platform. The code below is all that is needed to select between libiconv and iconv at the moment:

# src/crystal/iconv.cr
{% if flag?(:win32) %}
  require "./lib_iconv"
  private USE_LIBICONV = true
{% else %}
  require "c/iconv"
  private USE_LIBICONV = false
{% end %}

Thus Windows is hardcoded to use libiconv, and other platforms will continue to use iconv, but it is easy to change this in the future. If -Dwithout_iconv is provided then src/crystal/iconv.cr itself would not be required.

The decision to distinguish LibIconv from LibC is intentional, because they are not strictly API-compatible (e.g. FreeBSD has __iconv, GNU libiconv has iconvctl).

Related: #5430

@HertzDevil HertzDevil added kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:text labels Nov 20, 2021
Comment thread src/crystal/iconv.cr
Comment thread .github/workflows/win.yml Outdated
Comment thread src/crystal/lib_iconv.cr
require "c/stddef"

{% if flag?(:without_iconv) %}
{% raise "The `without_iconv` flag is preventing you to use the LibIconv module" %}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
{% raise "The `without_iconv` flag is preventing you to use the LibIconv module" %}
{% raise "The `without_iconv` flag is preventing you from using the LibIconv module" %}

Comment thread src/crystal/lib_iconv.cr
@straight-shoota straight-shoota mentioned this pull request Nov 24, 2021
22 tasks
@straight-shoota straight-shoota merged commit c88be0c into crystal-lang:master Nov 26, 2021
@oprypin
Copy link
Copy Markdown
Member

oprypin commented Nov 26, 2021

Every invocation of Crystal now gives a warning

iconv.lib(iconv.obj) : warning LNK4099: PDB 'libiconvStatic.pdb' was not found with 'iconv.lib(iconv.obj)' or at 'D:\a\crystal\crystal\libiconvStatic.pdb'; linking object as if no debug info
iconv.lib(localcharset.obj) : warning LNK4099: PDB 'libiconvStatic.pdb' was not found with 'iconv.lib(localcharset.obj)' or at 'D:\a\crystal\crystal\libiconvStatic.pdb'; linking object as if no debug info

https://github.com/crystal-lang/crystal/runs/4338209730?check_suite_focus=true#step:37:34

Currently that disrupts compilation of Shards
https://github.com/crystal-lang/install-crystal/runs/4338333591?check_suite_focus=true#step:4:29
https://github.com/crystal-lang/shards/blob/v0.16.0/Makefile#L27

@HertzDevil HertzDevil deleted the feature/windows-libiconv branch November 29, 2021 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:text

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants