Opened 4 years ago

Closed 3 years ago

Last modified 22 months ago

#27823 closed enhancement (fixed)

spkg-configure.m4 for iconv

Reported by: Erik Bray Owned by:
Priority: major Milestone: sage-8.9
Component: build: configure Keywords:
Cc: Michael Orlitzky, Samuel Lelièvre Merged in:
Authors: Dima Pasechnik, Erik Bray Reviewers: Erik Bray, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 2b5ceb1 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

In general the iconv SPKG is not even required. It's not installed (or rather, a dummy empty package is installed) for any platforms except Cygwin, HP-UX, and Solaris.

The requirement for it on Cygwin is probably outdated--iconv is a standard library on almost all Cygwin installations, and is surely more up-to-date than when this SPKG was first added in #8567. Support for the other two platforms has not been maintained much.

We can add a configure-time check for it, and treat it as a dummy package in most cases.

Change History (53)

comment:1 Changed 3 years ago by Dima Pasechnik

there is a cryptic

    # Disable NLS on Cygwin to be able to build libiconv without the Cygwin
    # libiconv package.

I presume the latter line should be "NLS package", no ?

Does this mean that the Cygwin's "native" libiconv does not work?

comment:2 Changed 3 years ago by Dima Pasechnik

According to #13912, that --disable-nls on Cygwin was dictated by the desire to reduce the number of Cygwin deps. I think it can be scrapped.

comment:3 Changed 3 years ago by Dima Pasechnik

Authors: Dima Pasechnik
Branch: public/configure/iconv-config
Commit: 0afc4dc503267f9af5c91b0e8d91456f5fef5342
Status: newneeds_review

using AM_ICONV (a bit of a hack). Needs config.rpath from gettext. (I also saw packages just creating an empty config.rpath, so perhaps we can do this too).

Does this work on Cygwin?


New commits:

0afc4dcuse AM_ICONV from gettext

comment:4 Changed 3 years ago by Dima Pasechnik

Cc: Michael Orlitzky added

comment:5 Changed 3 years ago by Dima Pasechnik

Status: needs_reviewneeds_work

on OSX this also needs lib-link.m4...

comment:6 Changed 3 years ago by git

Commit: 0afc4dc503267f9af5c91b0e8d91456f5fef53424ea22415c12958c5bb367b1693fdaa88c9aaa30b

Branch pushed to git repo; I updated commit sha1. New commits:

4ea2241moar autocon stuff added

comment:7 Changed 3 years ago by git

Commit: 4ea22415c12958c5bb367b1693fdaa88c9aaa30bff3d5ed15263c25e18fab585a2e7740d78f5f393

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ff3d5edmoar autocon stuff added

comment:8 Changed 3 years ago by git

Commit: ff3d5ed15263c25e18fab585a2e7740d78f5f39329ca93192dde1c7cf4e1a138c7ee81d91ca89e66

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

29ca931moar autocon stuff added

comment:9 Changed 3 years ago by Dima Pasechnik

Status: needs_workneeds_review

OK, this much is needed for OSX...

comment:10 Changed 3 years ago by Erik Bray

For the config.rpath stuff, it seems that file is included with gettext, typically under /usr/share/gettext/config.rpath. Normally it is installed by gettextize, but I don't think we actually want to run that, since we really just want the file; we don't want to prepare the program for direct use of gettext.

I wonder if there's a reasonably reliable way to just get the path to this file...

comment:11 Changed 3 years ago by Dima Pasechnik

the problem is that AM_ICONV pulls in a lot of macros from gettext, so config.rpath is just one of many things needed, as I found out on a gettext-less OSX box...

comment:12 Changed 3 years ago by Erik Bray

It turns out gettextize is just a shell script, and the path to /usr/share/gettext is just in some variables in that file that are output when gettext is configured.

comment:13 Changed 3 years ago by Erik Bray

the problem is that AM_ICONV pulls in a lot of macros from gettext

Are you saying you're trying to get it so you can run configure without the macros from gettext? The thing about config.rpath is that it's not a macro; it's just a file that needs to be copied into place. automake and the like don't do this automatically for the most part.

comment:14 in reply to:  13 Changed 3 years ago by Dima Pasechnik

Replying to embray:

the problem is that AM_ICONV pulls in a lot of macros from gettext

Are you saying you're trying to get it so you can run configure without the macros from gettext? The thing about config.rpath is that it's not a macro; it's just a file that needs to be copied into place. automake and the like don't do this automatically for the most part.

that's correct, all of this is just to use AM_ICONV. On OSX iconv is provided by the system, no need for gettext (or iconv) implementation; however AM_ICONV is a part of gettext package, and has several deps there, that need to be installed (and some of these deps need config.rpath script installed into the right place).

comment:15 Changed 3 years ago by Erik Bray

Apparently it's actually a macro called AC_LIB_RPATH that requires the config.rpath file, and it happens to be used indirectly by AM_ICONV. Most of the google results for AC_LIB_RPATH relate to this problem.

comment:16 Changed 3 years ago by Dima Pasechnik

Ha! Where is my medal?! Cf. Paul Eggert saying that only brave people use AM_ICONV directly: https://lists.gnu.org/archive/html/autoconf/2002-09/msg00181.html

comment:17 Changed 3 years ago by Erik Bray

I think this mostly just comes down to a misconfiguration on many distributions.

automake has a "libdir" that it searches for these generated files that it copies into your project. For example on my system my automake libdir is /usr/share/automake-1.14 in which I have I have:

$ ls -l /usr/share/automake-1.14
total 512
drwxr-xr-x 2 root root   4096 May 26  2016 am
-rwxr-xr-x 1 root root   5826 Jan  2  2014 ar-lib
drwxr-xr-x 2 root root   4096 May 26  2016 Automake
-rwxr-xr-x 1 root root   7333 Jan  2  2014 compile
lrwxrwxrwx 1 root root     20 Jan  2  2014 config.guess -> ../misc/config.guess
lrwxrwxrwx 1 root root     18 Jan  2  2014 config.sub -> ../misc/config.sub
-rw-r--r-- 1 root root  35147 Jan  2  2014 COPYING
-rwxr-xr-x 1 root root  23566 Jan  2  2014 depcomp
-rw-r--r-- 1 root root  15752 Jan  2  2014 INSTALL
-rwxr-xr-x 1 root root  13997 Jan  2  2014 install-sh
-rwxr-xr-x 1 root root   6047 Jan  2  2014 mdate-sh
-rwxr-xr-x 1 root root   6872 Jan  2  2014 missing
-rwxr-xr-x 1 root root   3538 Jan  2  2014 mkinstalldirs
-rwxr-xr-x 1 root root   4670 Jan  2  2014 py-compile
-rwxr-xr-x 1 root root  15285 Jan  2  2014 tap-driver.pl
-rwxr-xr-x 1 root root  19464 Jan  2  2014 tap-driver.sh
-rwxr-xr-x 1 root root   4287 Jan  2  2014 test-driver
-rw-r--r-- 1 root root 323102 Jan  2  2014 texinfo.tex
-rwxr-xr-x 1 root root   6858 Jan  2  2014 ylwrap

As you can see, the commonly used config.guess and config.sub are actually symlinks to /usr/misc. It turns out on my system these files are not even provided by automake, but rather a package called autotools-dev.

For this to work properly the distro ought to be creating a symlink /usr/share/automake-1.14/config.rpath -> ../gettext/config.rpath.

But they don't, so it doesn't get installed properly.

comment:18 Changed 3 years ago by Erik Bray

Here's a snippet of code I wrote to do what automake should be doing, but in our bootstrap script. Seems to work nicely but have not tried in the MacOS hinterlands...

  • bootstrap

    diff --git a/bootstrap b/bootstrap
    index 2505897..eca22b8 100755
    a b MAKE="${MAKE:-make}" 
    2828CONFVERSION=`cat $PKG/package-version.txt`
    2929
    3030
     31install_config_rpath() {
     32    # The file config.rpath which comes from gettext is supposed to be
     33    # installed by automake, but due to a bug in most distros it is not;
     34    # see https://trac.sagemath.org/ticket/27823#comment:17
     35    #
     36    # Here we need to determine where gettext stores its data files and
     37    # copy config.rpath from there to config/
     38    gettextize="$(command -v gettextize)"
     39    if [ -z "$gettextize" ]; then
     40        echo >&2 "gettext and the gettextize program must be installed."
     41        exit 1
     42    fi
     43    eval `sed -n '/^prefix=\(.*\)$/p' $gettextize`
     44    eval `sed -n '/^datarootdir=\(.*\)$/p' $gettextize`
     45    eval `sed -n '/^gettext_dir=\(.*\)$/p' $gettextize`
     46
     47    if [ -z "$gettext_dir" ]; then
     48        echo >&2 "Failed to read the gettext_dir directory from $gettextize"
     49        echo >&2 "The config.rpath file must manually be copied into config/"
     50        echo >&2 "This file is installed with gettext typically in /usr/share/gettext"
     51        exit 1
     52    fi
     53
     54    config_rpath="$gettext_dir/config.rpath"
     55    if [ ! -f "$config_rpath" ]; then
     56        echo >&2 "Missing $config_rpath file; this indicates a broken gettext install."
     57        exit 1
     58    fi
     59
     60    echo "bootstrap:$LINENO: installing 'config/config.rpath'"
     61    cp "$config_rpath" config/
     62}
     63
     64
     65
    3166bootstrap () {
    3267    rm -f m4/sage_spkg_configures.m4
    3368    spkg_configures=""
    SAGE_SPKG_CONFIGURE_$(echo ${pkgname} | tr '[a-z]' '[A-Z]')" 
    4075    echo "$spkg_configures" >> m4/sage_spkg_configures.m4
    4176
    4277    aclocal -I m4 && \
     78    install_config_rpath && \
    4379    automake --add-missing --copy build/make/Makefile-auto && \
    4480    autoconf

comment:19 Changed 3 years ago by Dima Pasechnik

on my gentoo box I have /usr/share/gettext/config.rpath, yet it does not get picked up, i.e. if I remove the copying of config.rpath from bootstrap I get

$ ./bootstrap 
rm -rf config configure build/make/Makefile-auto.in
configure.ac:320: installing 'config/compile'
configure.ac:105: installing 'config/config.guess'
m4/sage_spkg_configures.m4:35: error: required file 'config/config.rpath' not found
configure.ac:105: installing 'config/config.sub'
configure.ac:68: installing 'config/install-sh'
configure.ac:68: installing 'config/missing'

Apparently it has to installed:

$ grep -R "config.rpath" /usr/share/aclocal
/usr/share/aclocal/guile.m4:  dnl the file gnulib/build-aux/config.rpath.
/usr/share/aclocal/lib-link.m4:  dnl Complain if config.rpath is missing.

/usr/share/aclocal/lib-link.m4:  AC_REQUIRE_AUX_FILE([config.rpath])
/usr/share/aclocal/lib-link.m4:    ${CONFIG_SHELL-/bin/sh} "$ac_aux_dir/config.rpath" "$host" > conftest.sh

Naturally, on a box with gettext and autoconf installed none of these *.m4 files in this branch are needed, but config.path still is needed.

comment:20 Changed 3 years ago by Erik Bray

Try my patch above. The problem is that automake simply doesn't install the file if it's only found in /usr/share/gettext.

comment:21 Changed 3 years ago by git

Commit: 29ca93192dde1c7cf4e1a138c7ee81d91ca89e66be482fdd63928f5442bdf97e809b781512e2d0d0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

be482fduse AM_ICONV, copy config.rpath from gettext's datadir

comment:22 Changed 3 years ago by Dima Pasechnik

OK, this works with gettext 0.19 and 0.20 (also on OSX, with some extra effort). I had to change /^gettext_dir to /gettext_datadir and replace gettext_dir with gettext_datadir in the rest.

Anyhow, this also needs to be tested on a "clean" OSX, without autotoots, with pre-generated configure, too.

comment:23 Changed 3 years ago by Erik Bray

Except this won't work on my gettext where it's spelled "gettext_dir".

comment:24 Changed 3 years ago by Erik Bray

I guess mine must be quite old: https://github.com/autotools-mirror/gettext/commit/ff18897068486560e2bb421004cfbd42b7cdd0f8

This will still need to be reworked a bit. It's maddening that the tool doesn't just have like a --print-gettext-dir option.

comment:25 Changed 3 years ago by Erik Bray

This works in both versions:

eval `sed -n '/^prefix=.*$/p' $gettextize`
eval `sed -n '/^datarootdir=.*$/p' $gettextize`
eval `sed -n '/^gettext_dir=.*$/p' $gettextize`
if [ -z "$gettext_dir" ]; then
    # In newer versions this is spelled gettext_datadir
    eval `sed -n '/^: \${gettext_datadir=.*$/p' $gettextize`
    gettext_dir="$gettext_datadir"
fi

Also simplified a bit from my old version where I left in some groupings in the sed regexp that were not actually used.

comment:26 in reply to:  24 Changed 3 years ago by Dima Pasechnik

Replying to embray:

I guess mine must be quite old: https://github.com/autotools-mirror/gettext/commit/ff18897068486560e2bb421004cfbd42b7cdd0f8

This will still need to be reworked a bit. It's maddening that the tool doesn't just have like a --print-gettext-dir option.

Well, one can try to ask for it, but then it would need to get approved by Ulrich Drepper, it seems...

comment:27 in reply to:  25 ; Changed 3 years ago by Dima Pasechnik

Replying to embray:

This works in both versions:

eval `sed -n '/^prefix=.*$/p' $gettextize`
eval `sed -n '/^datarootdir=.*$/p' $gettextize`
eval `sed -n '/^gettext_dir=.*$/p' $gettextize`
if [ -z "$gettext_dir" ]; then
    # In newer versions this is spelled gettext_datadir
    eval `sed -n '/^: \${gettext_datadir=.*$/p' $gettextize`
    gettext_dir="$gettext_datadir"
fi

Also simplified a bit from my old version where I left in some groupings in the sed regexp that were not actually used.

well, this is out of sync with the ticket branch - but feel free to do your own version!

comment:28 Changed 3 years ago by git

Commit: be482fdd63928f5442bdf97e809b781512e2d0d00bb824e16e1e84345b536521a875c315530b1d11

Branch pushed to git repo; I updated commit sha1. New commits:

0bb824eSupport versions of gettext older than 2014-04-21

comment:29 in reply to:  27 Changed 3 years ago by Erik Bray

Replying to dimpase:

well, this is out of sync with the ticket branch - but feel free to do your own version!

I went ahead and updated the branch. Perhaps it's a little unnecessary to support much older versions, but it at least came up on one machine which I'd like to keep working for now.

comment:30 Changed 3 years ago by Dima Pasechnik

It's fine, sure - but did you test this branch on old gettext? It seems to me that you need else clause to if [ -z "$gettext_dir" ]; then doing

   gettext_datadir="$gettext_dir"

comment:31 Changed 3 years ago by Erik Bray

Oh yeah, you're right. This is a little messed up now.

comment:32 Changed 3 years ago by git

Commit: 0bb824e16e1e84345b536521a875c315530b1d1157efc6f4f5647bed26819d6d9b66021b9fd3ecef

Branch pushed to git repo; I updated commit sha1. New commits:

57efc6fReworked to prefer gettext_datadir (for newer gettexts) and fall back to gettext_dir only for older versions.

comment:33 Changed 3 years ago by Erik Bray

I think this is good now. Tested on both versions.

Also tested this on Cygwin and confirmed (as expected) that it works well using the system iconv (thank you!)

comment:34 Changed 3 years ago by Dima Pasechnik

Authors: Dima PasechnikDima Pasechnik, Erik Bray
Reviewers: Erik Bray, Dima Pasechnik
Status: needs_reviewpositive_review

comment:35 Changed 3 years ago by Michael Orlitzky

Is gettext guaranteed to be installed for some reason? If not, maybe we can punt and add it to the list at

http://doc.sagemath.org/html/en/installation/source.html#command-line-tools

comment:36 Changed 3 years ago by Dima Pasechnik

gettext, just like autotools, is needed to run ./bootstrap; none of this is documented, though. We should, though...

comment:37 Changed 3 years ago by Volker Braun

Status: positive_reviewneeds_work

install_config_rpath calls exit(1) in some code paths, but its supposed to fall back to downloading the confball...

comment:38 Changed 3 years ago by git

Commit: 57efc6f4f5647bed26819d6d9b66021b9fd3ecef1604b82c2b609765f56923f893679d002b7ba5d3

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fdad7d9use AM_ICONV, copy config.rpath from gettext's datadir
7f8d72dSupport versions of gettext older than 2014-04-21
bf2479dReworked to prefer gettext_datadir (for newer gettexts) and fall back to gettext_dir only for older versions.
1604b82return in install_rpath_config; analyse its result

comment:39 Changed 3 years ago by Dima Pasechnik

Status: needs_workneeds_review

rebased, added returns, removed exits

comment:40 Changed 3 years ago by git

Commit: 1604b82c2b609765f56923f893679d002b7ba5d3f79ccf95f3f4b8a626c85f78f1433efb46813c6b

Branch pushed to git repo; I updated commit sha1. New commits:

f79ccf9do not run aclocal before we tried gettex, and correct return code

comment:41 Changed 3 years ago by Dima Pasechnik

ok, so this works without gettext installed (some funny thing with the return code, I return 779 and receive 11)

on a broken gettext install, with absent AM_ICONV, bootstrap -d would still fail.

Do we care about the latter?

comment:42 Changed 3 years ago by Dima Pasechnik

Milestone: sage-8.8sage-8.9
Status: needs_reviewpositive_review

Correction: it does work in the latter scenario.

comment:43 Changed 3 years ago by git

Commit: f79ccf95f3f4b8a626c85f78f1433efb46813c6b2b5ceb1d77c17ef83f2738bf4a1a82c84f124749
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

2b5ceb1return must be between 0 and 255

comment:44 Changed 3 years ago by Dima Pasechnik

Status: needs_reviewpositive_review

renumbered the return code to be between 0 and 255.

comment:45 Changed 3 years ago by Volker Braun

Branch: public/configure/iconv-config2b5ceb1d77c17ef83f2738bf4a1a82c84f124749
Resolution: fixed
Status: positive_reviewclosed

comment:46 Changed 3 years ago by Erik Bray

Commit: 2b5ceb1d77c17ef83f2738bf4a1a82c84f124749

It seems that, at least on Cygwin, AM_ICONV will pass, somehow, even if iconv.h is installed, thus making the libicon-devel package on Cygwin a requirement, at least when it comes to building R (which otherwise fails at #include <iconv.h>).

For now I'll just update the Cygwin build instructions to require libiconv-devel as a prerequisite which is preferable anyways. But maybe this still needs to be checked for somehow...

comment:47 Changed 3 years ago by Erik Bray

Cc: Samuel Lelièvre added

ping slelievre for first discovering this.

comment:48 Changed 3 years ago by Dima Pasechnik

maybe add AC_CHECK_HEADER for iconv.h ?

comment:49 Changed 3 years ago by Erik Bray

That's what I'm thinking, though now I need to double-check what AM_ICONV is actually doing, since I'm surprised it doesn't seem to need to do that...

comment:50 Changed 3 years ago by Dima Pasechnik

to get this working on OSX's home-brew, do

brew install gettext
brew link gettext --force

comment:51 Changed 3 years ago by Matthias Köppe

Follow up: #29532

Last edited 3 years ago by Matthias Köppe (previous) (diff)

comment:52 Changed 22 months ago by Dima Pasechnik

Bruno Haible says on bug-autoconf that one should use gnulib's iconv module to install config.rpath etc. Indeed,

gnulib-tool --import iconv

will install config.rpath into config/ (among with other things that probably can be ignored). So this is something to try in order to get rid of the hacky install_config_rpath() in comment:18

comment:53 Changed 22 months ago by Matthias Köppe

I have added this remark to #29549.

Note: See TracTickets for help on using tickets.