Opened 7 months ago

Closed 5 months ago

Last modified 3 months ago

#27823 closed enhancement (fixed)

spkg-configure.m4 for iconv

Reported by: embray Owned by:
Priority: major Milestone: sage-8.9
Component: build: configure Keywords:
Cc: mjo, slelievre Merged in:
Authors: Dima Pasechnik, Erik Bray Reviewers: Erik Bray, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 2b5ceb1 (Commits) Commit:
Dependencies: Stopgaps:

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 (50)

comment:1 Changed 6 months ago by dimpase

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 6 months ago by dimpase

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 6 months ago by dimpase

  • Authors set to Dima Pasechnik
  • Branch set to public/configure/iconv-config
  • Commit set to 0afc4dc503267f9af5c91b0e8d91456f5fef5342
  • Status changed from new to needs_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 6 months ago by dimpase

  • Cc mjo added

comment:5 Changed 6 months ago by dimpase

  • Status changed from needs_review to needs_work

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

comment:6 Changed 6 months ago by git

  • Commit changed from 0afc4dc503267f9af5c91b0e8d91456f5fef5342 to 4ea22415c12958c5bb367b1693fdaa88c9aaa30b

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

4ea2241moar autocon stuff added

comment:7 Changed 6 months ago by git

  • Commit changed from 4ea22415c12958c5bb367b1693fdaa88c9aaa30b to ff3d5ed15263c25e18fab585a2e7740d78f5f393

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

ff3d5edmoar autocon stuff added

comment:8 Changed 6 months ago by git

  • Commit changed from ff3d5ed15263c25e18fab585a2e7740d78f5f393 to 29ca93192dde1c7cf4e1a138c7ee81d91ca89e66

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

29ca931moar autocon stuff added

comment:9 Changed 6 months ago by dimpase

  • Status changed from needs_work to needs_review

OK, this much is needed for OSX...

comment:10 Changed 6 months ago by embray

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 6 months ago by dimpase

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 6 months ago by embray

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 follow-up: Changed 6 months ago by 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.

comment:14 in reply to: ↑ 13 Changed 6 months ago by dimpase

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 6 months ago by embray

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 6 months ago by dimpase

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 6 months ago by embray

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 6 months ago by embray

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 6 months ago by dimpase

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 6 months ago by embray

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 6 months ago by git

  • Commit changed from 29ca93192dde1c7cf4e1a138c7ee81d91ca89e66 to be482fdd63928f5442bdf97e809b781512e2d0d0

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 6 months ago by dimpase

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 6 months ago by embray

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

comment:24 follow-up: Changed 6 months ago by 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.

comment:25 follow-up: Changed 6 months ago by 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.

comment:26 in reply to: ↑ 24 Changed 6 months ago by dimpase

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 ; follow-up: Changed 6 months ago by dimpase

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 6 months ago by git

  • Commit changed from be482fdd63928f5442bdf97e809b781512e2d0d0 to 0bb824e16e1e84345b536521a875c315530b1d11

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 6 months ago by embray

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 6 months ago by dimpase

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 6 months ago by embray

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

comment:32 Changed 6 months ago by git

  • Commit changed from 0bb824e16e1e84345b536521a875c315530b1d11 to 57efc6f4f5647bed26819d6d9b66021b9fd3ecef

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 6 months ago by embray

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 6 months ago by dimpase

  • Authors changed from Dima Pasechnik to Dima Pasechnik, Erik Bray
  • Reviewers set to Erik Bray, Dima Pasechnik
  • Status changed from needs_review to positive_review

comment:35 Changed 6 months ago by mjo

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 6 months ago by dimpase

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

comment:37 Changed 5 months ago by vbraun

  • Status changed from positive_review to needs_work

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

comment:38 Changed 5 months ago by git

  • Commit changed from 57efc6f4f5647bed26819d6d9b66021b9fd3ecef to 1604b82c2b609765f56923f893679d002b7ba5d3

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 5 months ago by dimpase

  • Status changed from needs_work to needs_review

rebased, added returns, removed exits

comment:40 Changed 5 months ago by git

  • Commit changed from 1604b82c2b609765f56923f893679d002b7ba5d3 to f79ccf95f3f4b8a626c85f78f1433efb46813c6b

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 5 months ago by dimpase

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 5 months ago by dimpase

  • Milestone changed from sage-8.8 to sage-8.9
  • Status changed from needs_review to positive_review

Correction: it does work in the latter scenario.

comment:43 Changed 5 months ago by git

  • Commit changed from f79ccf95f3f4b8a626c85f78f1433efb46813c6b to 2b5ceb1d77c17ef83f2738bf4a1a82c84f124749
  • Status changed from positive_review to needs_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 5 months ago by dimpase

  • Status changed from needs_review to positive_review

renumbered the return code to be between 0 and 255.

comment:45 Changed 5 months ago by vbraun

  • Branch changed from public/configure/iconv-config to 2b5ceb1d77c17ef83f2738bf4a1a82c84f124749
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:46 Changed 4 months ago by embray

  • Commit 2b5ceb1d77c17ef83f2738bf4a1a82c84f124749 deleted

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 4 months ago by embray

  • Cc slelievre added

ping slelievre for first discovering this.

comment:48 Changed 4 months ago by dimpase

maybe add AC_CHECK_HEADER for iconv.h ?

comment:49 Changed 4 months ago by embray

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 months ago by dimpase

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

brew install gettext
brew link gettext --force
Note: See TracTickets for help on using tickets.