#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, GitHub, GitLab) | 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 (53)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
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
- 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:
0afc4dc | use AM_ICONV from gettext
|
comment:4 Changed 3 years ago by
- Cc mjo added
comment:5 Changed 3 years ago by
- Status changed from needs_review to needs_work
on OSX this also needs lib-link.m4...
comment:6 Changed 3 years ago by
- Commit changed from 0afc4dc503267f9af5c91b0e8d91456f5fef5342 to 4ea22415c12958c5bb367b1693fdaa88c9aaa30b
Branch pushed to git repo; I updated commit sha1. New commits:
4ea2241 | moar autocon stuff added
|
comment:7 Changed 3 years ago by
- Commit changed from 4ea22415c12958c5bb367b1693fdaa88c9aaa30b to ff3d5ed15263c25e18fab585a2e7740d78f5f393
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ff3d5ed | moar autocon stuff added
|
comment:8 Changed 3 years ago by
- Commit changed from ff3d5ed15263c25e18fab585a2e7740d78f5f393 to 29ca93192dde1c7cf4e1a138c7ee81d91ca89e66
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
29ca931 | moar autocon stuff added
|
comment:9 Changed 3 years ago by
- Status changed from needs_work to needs_review
OK, this much is needed for OSX...
comment:10 Changed 3 years ago by
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
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
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: ↓ 14 Changed 3 years ago by
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
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
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
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
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
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}" 28 28 CONFVERSION=`cat $PKG/package-version.txt` 29 29 30 30 31 install_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 31 66 bootstrap () { 32 67 rm -f m4/sage_spkg_configures.m4 33 68 spkg_configures="" … … SAGE_SPKG_CONFIGURE_$(echo ${pkgname} | tr '[a-z]' '[A-Z]')" 40 75 echo "$spkg_configures" >> m4/sage_spkg_configures.m4 41 76 42 77 aclocal -I m4 && \ 78 install_config_rpath && \ 43 79 automake --add-missing --copy build/make/Makefile-auto && \ 44 80 autoconf
comment:19 Changed 3 years ago by
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
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
- Commit changed from 29ca93192dde1c7cf4e1a138c7ee81d91ca89e66 to be482fdd63928f5442bdf97e809b781512e2d0d0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
be482fd | use AM_ICONV, copy config.rpath from gettext's datadir
|
comment:22 Changed 3 years ago by
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
Except this won't work on my gettext where it's spelled "gettext_dir".
comment:24 follow-up: ↓ 26 Changed 3 years ago by
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: ↓ 27 Changed 3 years ago by
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
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: ↓ 29 Changed 3 years ago by
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" fiAlso 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
- Commit changed from be482fdd63928f5442bdf97e809b781512e2d0d0 to 0bb824e16e1e84345b536521a875c315530b1d11
Branch pushed to git repo; I updated commit sha1. New commits:
0bb824e | Support versions of gettext older than 2014-04-21
|
comment:29 in reply to: ↑ 27 Changed 3 years ago by
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
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
Oh yeah, you're right. This is a little messed up now.
comment:32 Changed 3 years ago by
- Commit changed from 0bb824e16e1e84345b536521a875c315530b1d11 to 57efc6f4f5647bed26819d6d9b66021b9fd3ecef
Branch pushed to git repo; I updated commit sha1. New commits:
57efc6f | Reworked to prefer gettext_datadir (for newer gettexts) and fall back to gettext_dir only for older versions.
|
comment:33 Changed 3 years ago by
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
- Reviewers set to Erik Bray, Dima Pasechnik
- Status changed from needs_review to positive_review
comment:35 Changed 3 years ago by
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
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
- 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 3 years ago by
- Commit changed from 57efc6f4f5647bed26819d6d9b66021b9fd3ecef to 1604b82c2b609765f56923f893679d002b7ba5d3
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fdad7d9 | use AM_ICONV, copy config.rpath from gettext's datadir
|
7f8d72d | Support versions of gettext older than 2014-04-21
|
bf2479d | Reworked to prefer gettext_datadir (for newer gettexts) and fall back to gettext_dir only for older versions.
|
1604b82 | return in install_rpath_config; analyse its result
|
comment:39 Changed 3 years ago by
- Status changed from needs_work to needs_review
rebased, added returns, removed exits
comment:40 Changed 3 years ago by
- Commit changed from 1604b82c2b609765f56923f893679d002b7ba5d3 to f79ccf95f3f4b8a626c85f78f1433efb46813c6b
Branch pushed to git repo; I updated commit sha1. New commits:
f79ccf9 | do not run aclocal before we tried gettex, and correct return code
|
comment:41 Changed 3 years ago by
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
- 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 3 years ago by
- 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:
2b5ceb1 | return must be between 0 and 255
|
comment:44 Changed 3 years ago by
- Status changed from needs_review to positive_review
renumbered the return code to be between 0 and 255.
comment:45 Changed 3 years ago by
- Branch changed from public/configure/iconv-config to 2b5ceb1d77c17ef83f2738bf4a1a82c84f124749
- Resolution set to fixed
- Status changed from positive_review to closed
comment:46 Changed 3 years ago by
- 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:48 Changed 3 years ago by
maybe add AC_CHECK_HEADER for iconv.h ?
comment:49 Changed 3 years ago by
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
to get this working on OSX's home-brew, do
brew install gettext brew link gettext --force
comment:52 Changed 19 months ago by
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 19 months ago by
I have added this remark to #29549.
there is a cryptic
I presume the latter line should be "NLS package", no ?
Does this mean that the Cygwin's "native" libiconv does not work?