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

spkg-configure.m4 for iconv #27823

Closed
embray opened this issue May 13, 2019 · 66 comments
Closed

spkg-configure.m4 for iconv #27823

embray opened this issue May 13, 2019 · 66 comments

Comments

@embray
Copy link
Contributor

embray commented May 13, 2019

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.

CC: @orlitzky @slel

Component: build: configure

Author: Dima Pasechnik, Erik Bray

Branch: 2b5ceb1

Reviewer: Erik Bray, Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/27823

@embray embray added this to the sage-8.8 milestone May 13, 2019
@dimpase
Copy link
Member

dimpase commented Jun 10, 2019

comment:1

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?

@dimpase
Copy link
Member

dimpase commented Jun 10, 2019

comment:2

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.

@dimpase
Copy link
Member

dimpase commented Jun 10, 2019

Branch: public/configure/iconv-config

@dimpase
Copy link
Member

dimpase commented Jun 10, 2019

comment:3

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

@dimpase
Copy link
Member

dimpase commented Jun 10, 2019

Author: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Jun 10, 2019

Commit: 0afc4dc

@dimpase
Copy link
Member

dimpase commented Jun 11, 2019

comment:5

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2019

Changed commit from 0afc4dc to 4ea2241

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2019

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

4ea2241moar autocon stuff added

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2019

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

ff3d5edmoar autocon stuff added

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2019

Changed commit from 4ea2241 to ff3d5ed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2019

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

29ca931moar autocon stuff added

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2019

Changed commit from ff3d5ed to 29ca931

@dimpase
Copy link
Member

dimpase commented Jun 11, 2019

comment:9

OK, this much is needed for OSX...

@embray
Copy link
Contributor Author

embray commented Jun 11, 2019

comment:10

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...

@dimpase
Copy link
Member

dimpase commented Jun 11, 2019

comment:11

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...

@embray
Copy link
Contributor Author

embray commented Jun 11, 2019

comment:12

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.

@embray
Copy link
Contributor Author

embray commented Jun 11, 2019

comment:13

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.

@dimpase
Copy link
Member

dimpase commented Jun 11, 2019

comment:14

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).

@embray
Copy link
Contributor Author

embray commented Jun 11, 2019

comment:15

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.

@dimpase
Copy link
Member

dimpase commented Jun 11, 2019

comment:16

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2019

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2019

Changed commit from 1604b82 to f79ccf9

@dimpase
Copy link
Member

dimpase commented Jun 27, 2019

comment:41

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?

@dimpase
Copy link
Member

dimpase commented Jun 27, 2019

comment:42

Correction: it does work in the latter scenario.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2019

Changed commit from f79ccf9 to 2b5ceb1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2019

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

@dimpase
Copy link
Member

dimpase commented Jun 27, 2019

comment:44

renumbered the return code to be between 0 and 255.

@vbraun
Copy link
Member

vbraun commented Jul 4, 2019

Changed branch from public/configure/iconv-config to 2b5ceb1

@embray
Copy link
Contributor Author

embray commented Aug 14, 2019

comment:46

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...

@embray
Copy link
Contributor Author

embray commented Aug 14, 2019

Changed commit from 2b5ceb1 to none

@embray
Copy link
Contributor Author

embray commented Aug 14, 2019

comment:47

ping slelievre for first discovering this.

@dimpase
Copy link
Member

dimpase commented Aug 14, 2019

comment:48

maybe add AC_CHECK_HEADER for iconv.h ?

@embray
Copy link
Contributor Author

embray commented Aug 14, 2019

comment:49

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...

@dimpase
Copy link
Member

dimpase commented Sep 19, 2019

comment:50

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

brew install gettext
brew link gettext --force

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 19, 2020

comment:51

Follow up: #29532

@dimpase
Copy link
Member

dimpase commented Jan 30, 2021

comment:52

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 7, 2021

comment:53

I have added this remark to #29549.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants