GNOME Bugzilla – Bug 152020
Use absolute paths to iconv and gettext
Last modified: 2004-12-22 21:47:04 UTC
intltool uses iconv and gettext (msgfmt, msgmerge and xgettext) at run time. To run these, it uses the shell's which command, which relies on the path to find them. IMHO, this approach is wrong, as it can cause some problems. Let me describe them. Given that all these utilities are dependencies of intltool, they can be looked for at configuration time (just like it actually does for perl). And, once found, their path can be hardcoded in the generated scripts. This way intltool will _always_ run the same utilities, no matter what the path is for a given user. But where are the problems? Consider a system with multiple versions of gettext installed. For example, it could be NetBSD, which comes with GNU gettext in the packages collection and with a BSD replacement in the base system. Sometimes, the gettext in the base system causes compatibility problems, so the one from the packages collection has to be used. With the current situation, a user may install intltool in the system and use it with his path preferring the GNU gettext; he will get no problems. But if a user tries to run it with his path pointing to the other gettext by default, intltool will misteriously fail. Having the absolute path hardcoded in it could have avoided this problem. Note that this problem is hypotetical in NetBSD, although I have hit it with other packages (not intltool). However, it may perfectly happen on other systems, or in a future if GNU-specific functionality is used. Therefore, I think that fixing this issue ASAP will improve future packages. IMHO, I think that this is a problem. However you may perfectly disagree. In such case, I may try to convince you, or you can simply close this bug...
Created attachment 31344 [details] [review] Proposed patch
Patch looks fine, but you always have to add a ChangeLog entry. Can people confirm that intltool still works after applying this patch? If so, it can be committed.
I'm not sure this is the correct fix. It is possible that one app will need a different version of gettext than another app, and so being able to point at different prefixes is quite useful. I don't think we should hardcode paths. If we require specific options that only exist in GNU gettext, perhaps we should check to make sure that we are using GNU gettext.
I agree with Rodney. We might want to use the approach we already use for some of the stuff (eg. my $iconv = $ENV{"INTLTOOL_ICONV"} || "@INTLTOOL_ICONV@";) for everything, including gettext functions. So, introduce $MSGMERGE and $MSGFMT as well ($XGETTEXT is already correctly set based on environment variable XGETTEXT).
Created attachment 31409 [details] [review] Patch with env variables Let's see how this looks. This new patch is the same as the previous one, but recognizes the $MSGFMT and $MSGMERGE environment variables; these can be used to override the paths hardcoded in intltool, in case that it's necessary.
I have a few issues with this patch, but they are easily fixable. There are various inconsistencies with variable names and such. If the MSGFMT and MSGMERGE env vars are going to not have INTLTOOL_ prefixed, then so should ICONV. And if we're going to allow setting the env vars, we should for XGETTEXT also. It seems like one would want to use the same version of xgettext as msgfmt and msgmerge. Also, the sed rule for @INTLTOOL_EXTRACT@ should use $(bindir) instead of ${exec_prefix}/bin. It gets properly expanded in the shell anyway. The general idea behind the patch seems ok though.
Rodney, we're already using XGETTEXT environment variable (look around line 586 in intltool-update.in.in), like I said in comment #4. On the topic of variable names, I agree, though I didn't want to insist on it right away.
Btw, I suppose it's a good idea to have this documented in a README in a section titled eg. "Modifying intltool behaviour with environment variables". Anyone looking forward to doing that? ;-)
Ah. But the code is inconsistent. We should fix that up to be consistent with the rest of the env var usages then. It looks weird. :)
Yes, indeed. "Inconsistent" is even a bit too soft: "messy" might be even more appropriate ;-)
Hmm yes, at first I thought that having INTLTOOL_ prepended to all those variables could be good. Maybe I didn't do it because some of the existing variables didn't have it (XGETTEXT, IIRC). So what'd be the best way to go? Name all the variables INTLTOOL_*?
Note that replacing XGETTEXT with INTLTOOL_XGETTEXT might mean incompatible change (though not very visible one, since I suspect not many users are relying on XGETTEXT being used).
I think we should replace INTLTOOL_ICONV with plain ICONV and check for both, falling back to whatever we find at configure time, and keep all the gettext variables without the INTLTOOL_ prefix.
Created attachment 32028 [details] [review] Updated patch to fix issues Here is an updated version of the patch, that fixes the issues I mentioned. If nobody complains, I'm going to commit this and do a release. Danilo? :)
Looks good, go ahead ;-)
Committed and made a release with the few important bug fixes.