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

vim-full can not be compiled as the version was built without iconv() #14328

Open
BKPepe opened this issue Dec 24, 2020 · 15 comments
Open

vim-full can not be compiled as the version was built without iconv() #14328

BKPepe opened this issue Dec 24, 2020 · 15 comments

Comments

@BKPepe
Copy link
Member

BKPepe commented Dec 24, 2020

Maintainer: @ratkaj
Environment: all Turris routers (powerpc, ARMv7 - arm_cortex-a9_vfpv3-d16) , ARMv8 - aarch64_cortex-a53), OpenWrt master
Description:

1623 translated messages, 3 fuzzy translations.
echo af ca cs cs.cp1250 da de en_GB eo es fi fr ga it ja ja.euc-jp ja.sjis ko ko.UTF-8 lv nb nl no pl pl.UTF-8 pl.cp1250 pt_BR ru ru.cp1251 sk sk.cp1250 sr sv tr uk uk.cp1251 vi zh_CN zh_CN.UTF-8 zh_CN.cp936 zh_TW zh_TW.UTF-8  | tr " " "\n" |sed -e '/\./d' | sort > LINGUAS
msgfmt --desktop -d . --template gvim.desktop.in -o tmp_gvim.desktop
msgfmt: Cannot convert from "ISO-8859-1" to "UTF-8". msgfmt relies on iconv(). This version was built without iconv().
make[5]: *** [Makefile:177: gvim.desktop] Error 1
make[5]: Leaving directory '/foo-mox/build/build_dir/target-aarch64_cortex-a53_musl/vim82/src/po'
make[4]: *** [Makefile:2092: languages] Error 2
make[4]: Leaving directory '/foo-mox/build/build_dir/target-aarch64_cortex-a53_musl/vim82/src'
make[3]: *** [Makefile:39: all] Error 2
@BKPepe
Copy link
Member Author

BKPepe commented Dec 24, 2020

It seems that this patch https://gitlab.nic.cz/turris/turris-build/-/commit/a8764223e8c7e291a65d05d2be24fe2c99018cbc solves the issue for me locally and I will run full build and then send a PR.

@neheb
Copy link
Contributor

neheb commented Dec 24, 2020

Last I looked this was an issue with the host build of libiconv. I never investigated.

@BKPepe
Copy link
Member Author

BKPepe commented Dec 25, 2020

I run all build and linked patch didn't solved the issue.

@CharlesARoy
Copy link

Also getting this issue. The patch didn't work for you in the end @BKPepe?

@BKPepe
Copy link
Member Author

BKPepe commented Jan 10, 2021

No, the patch does not work when I run full build.

@dl12345
Copy link
Contributor

dl12345 commented Feb 1, 2021

I unfortunately don't have the time to create a robust PR to fix this problem, but I can tell you precisely why it's happening so that someone with the time on their hands can create a PR to fix.

The error message indicates that msgfmt, part of the host build of gettext-full, is not being built with iconv. This message is indeed correct. It's not built with iconv.

There are two reasons for this

  • The use of iconv is explicitly turned off in the host compilation configure arguments
  • The configure script in the host build of gettext-full/gettext-tools fails the check for iconv and thus disables it by undefining the preprocessor symbol HAVE_ICONV.

Both issues need to be fixed before getting an iconv-enabled msgfmt. The first problem is fixed by the following patch

diff --git a/package/libs/gettext-full/Makefile b/package/libs/gettext-full/Makefile
index 2e65571701..2a033303d7 100644
--- a/package/libs/gettext-full/Makefile
+++ b/package/libs/gettext-full/Makefile
@@ -73,10 +73,7 @@ HOST_CONFIGURE_ARGS += \
 
 
 HOST_CONFIGURE_VARS += \
-       EMACS="no" \
-       am_cv_lib_iconv=no \
-       am_cv_func_iconv=no \
-       ac_cv_header_iconv_h=no \
+       EMACS="no"
 
 HOST_CFLAGS += $(HOST_FPIC)

Now, the cause of the second problem. I have not made a robust solution for it. But looking at the configure logs, the compilation of the test program in the configure script in gettext-tools fails thus:

usr/bin/ld: /tmp/ccXkMouV.o: in function `main':
conftest.c:(.text.startup+0x10): undefined reference to `libiconv_open'
/usr/bin/ld: conftest.c:(.text.startup+0x24): undefined reference to `libiconv'
/usr/bin/ld: conftest.c:(.text.startup+0x2c): undefined reference to `libiconv_close'
collect2: error: ld returned 1 exit status

I compared the non-host build configure log to the host build configure log. The former passes the iconv check, while the latter does not. To cut a long story short, the reason for this is the iconv.h header file. The non-host build is picking up iconv.h from the toolchain include directory, while the host build is picking it up from the staging_dir/hostpkg/include directory.

Looking at the declarations and comparing the two, the reason for the failure becomes obvious. The file in staging_dir/hostpkg/include/iconv.h is from the non-full version of libiconv and has incorrect declarations if using libiconv-full. The following shows what's actually wrong with it, although it should not be taken as a patch that needs to be applied - just as an illustration of why it's failing. That said, if you edit the file as per the patch below, msgfmt will be built successfully with iconv and the compilation of vim-fuller will succeed.

diff --git a/iconv.h b/iconv.h
index 8767be4..3407620 100644
--- a/iconv.h
+++ b/iconv.h
@@ -11,11 +11,7 @@ extern "C" {
 
 extern int _libiconv_version; /* Likewise */
 
-typedef long iconv_t;
-
-#define iconv_open libiconv_open
-#define iconv libiconv
-#define iconv_close libiconv_close
+typedef void* iconv_t;
 
 extern iconv_t
 iconv_open(const char *tocode, const char *fromcode);

So, it looks like libiconv-full needs a host build.

I did try to copy over libiconv-full's version of the iconv.h header file to the hostpkg/include directory to test. One needs to add a "HOST_CFLAGS += -DLIBICONV_PLUG" to the gettext-full Makefile so that the correct definitions are made when including the iconv.h file. However, just copying the header will fail without an updated libiconv.a as there is an extern def in the header for a symbol which needs the full libiconv.a library available to link against.

Since my build host and target architecture are both x86_64, to test, I just blindly copied over libiconv-full's version of libiconv.a to the staging_dir/hostpkg/lib directory. It works, but for some reason, I had to add an explicit link this time to libiconv.a in the gettext-full Makefile HOST_LDFLAGS += -l:libiconv.a, as it threw the same undefined symbol again.

So the edits I made to the gettext-full Makefile look as follows

diff --git a/package/libs/gettext-full/Makefile b/package/libs/gettext-full/Makefile
index 2e65571701..5597388501 100644
--- a/package/libs/gettext-full/Makefile
+++ b/package/libs/gettext-full/Makefile
@@ -73,12 +73,10 @@ HOST_CONFIGURE_ARGS += \
 
 
 HOST_CONFIGURE_VARS += \
-       EMACS="no" \
-       am_cv_lib_iconv=no \
-       am_cv_func_iconv=no \
-       ac_cv_header_iconv_h=no \
+       EMACS="no"
 
-HOST_CFLAGS += $(HOST_FPIC)
+HOST_CFLAGS += $(HOST_FPIC) -DLIBICONV_PLUG 
+HOST_LDFLAGS += -l:libiconv.a
 
 define Build/InstallDev
        $(INSTALL_DIR) $(1)/usr/lib/libintl-full/include

I have not done a full build after this, so not sure if changing the header in hostpkg/include will break anything else. I do know that this way gettext-full's msgfmt gets built with iconv and the compilation of vim-fuller succeeds

So there's a little bit of work to do to find a robust solution for the libiconv issue, but this should point you in the right direction.

@neheb
Copy link
Contributor

neheb commented Feb 1, 2021

You should point those comments here: openwrt/openwrt#3763

@dl12345
Copy link
Contributor

dl12345 commented Feb 1, 2021

You should point those comments here: openwrt/openwrt#3763

It shows up a reference in the PR to this issue. I assume that's enough?

@dl12345
Copy link
Contributor

dl12345 commented Feb 2, 2021

I think there's a cleaner way to do this than making a host build of libiconv-full. The following two diffs show patches to libiconv and to gettext-full. They allow gettext's msgfmt to build with iconv support and the build of vim-fuller succeeds.

diff --git a/package/libs/libiconv/src/include/iconv.h b/package/libs/libiconv/src/include/iconv.h
index 8767be42ee..48cd75a4cc 100644
--- a/package/libs/libiconv/src/include/iconv.h
+++ b/package/libs/libiconv/src/include/iconv.h
@@ -11,12 +11,20 @@ extern "C" {
 
 extern int _libiconv_version; /* Likewise */
 
+#ifdef LIBICONV_PLUG
+
+typedef void* iconv_t;
+
+#else
+
 typedef long iconv_t;
 
 #define iconv_open libiconv_open
 #define iconv libiconv
 #define iconv_close libiconv_close
 
+#endif
+
 extern iconv_t
 iconv_open(const char *tocode, const char *fromcode);

The LIBICONV_PLUG preprocessor symbol is the one used in libiconv-full for the same purpose, so it seems like the right one to use.

diff --git a/package/libs/gettext-full/Makefile b/package/libs/gettext-full/Makefile
index 2e65571701..b168dff46a 100644
--- a/package/libs/gettext-full/Makefile
+++ b/package/libs/gettext-full/Makefile
@@ -73,12 +73,9 @@ HOST_CONFIGURE_ARGS += \
 
 
 HOST_CONFIGURE_VARS += \
-       EMACS="no" \
-       am_cv_lib_iconv=no \
-       am_cv_func_iconv=no \
-       ac_cv_header_iconv_h=no \
+       EMACS="no"
 
-HOST_CFLAGS += $(HOST_FPIC)
+HOST_CFLAGS += $(HOST_FPIC) -DLIBICONV_PLUG 
 
 define Build/InstallDev
        $(INSTALL_DIR) $(1)/usr/lib/libintl-full/include

@nobk
Copy link

nobk commented Feb 24, 2021

Because gvim.desktop is not need by openwrt, use this patch to bypass desktop things building.

cat feeds/packages/utils/vim/patches/003-bypass-gvim-desktop-build.patch

--- a/src/po/Makefile
+++ b/src/po/Makefile
@@ -32,7 +32,7 @@
        $(VIM) -u NONE -e -X -S check.vim -c "if error == 0 | q | endif" -c cq $<
        touch $@

-all: $(MOFILES) $(MOCONVERTED) $(MSGFMT_DESKTOP)
+all: $(MOFILES) $(MOCONVERTED)

 check: $(CHECKFILES)


@micmac1
Copy link
Contributor

micmac1 commented Feb 25, 2021

I added a possible way out of the msgfmt dilemma to openwrt repo here. vim package would still need a bit up changing like suggestion below, but at least the required tools would be available.

diff --git a/utils/vim/Makefile b/utils/vim/Makefile
index adc3751a7..46888b8be 100644
--- a/utils/vim/Makefile
+++ b/utils/vim/Makefile
@@ -27,11 +27,12 @@ HOST_BUILD_PARALLEL:=1
 
 include $(INCLUDE_DIR)/package.mk
 include $(INCLUDE_DIR)/host-build.mk
+include $(INCLUDE_DIR)/nls.mk
 
 define Package/vim/Default
   SECTION:=utils
   CATEGORY:=Utilities
-  DEPENDS:=+libncurses
+  DEPENDS:=+libncurses $(ICONV_DEPENDS) $(INTL_DEPENDS)
   TITLE:=Vi IMproved - enhanced vi editor
   URL:=http://www.vim.org/
   SUBMENU:=Editors
@@ -144,6 +145,8 @@ CONFIGURE_VARS += \
        vim_cv_tty_group=root \
        vim_cv_tty_mode=0620
 
+HOST_LDFLAGS+=-Wl,-rpath,$(STAGING_DIR_HOSTPKG)/lib
+
 ifneq ($(HOST_OS),Linux)
   TARGET_PATH_PKG:=$(CURDIR)/scripts:$(TARGET_PATH_PKG)
 endif

@nobk
Copy link

nobk commented Feb 27, 2021

@micmac1 iconv only need while compile time, to complete gvim.desktop po file building, and iconv no need at run time.
If you add depends iconv and intl, it will enlarge on router package size, but never be used.
To accomplish vim package compile, just skip gvim.desktop things building is enough. Openwrt no GUI, don't need gvim.

@micmac1
Copy link
Contributor

micmac1 commented Feb 27, 2021 via email

@nobk
Copy link

nobk commented Feb 27, 2021

I am also willing that host gettext-full got iconv function, but don't want vim package add not using dependencies.

@nobk
Copy link

nobk commented Feb 27, 2021

@dl12345 I guess this issue cause by the current autoconf and automake tools are not compatible with gettext-full new version v0.21

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

No branches or pull requests

6 participants