Re: [PATCH 2/4] AC_SITE_LOAD: add OS/2-specific initialization

[Eric Blake]

On 09/22/2014 08:39 PM, KO Myung-Hun wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hi/2.
> 
> Eric Blake wrote:
>> On 09/22/2014 12:59 AM, KO Myung-Hun wrote:
>>> \ may be recognized as an escape character on some shells such
>>> as pdksh. And the executables on OS/2 have .exe as an extension.
>>
>> Umm, \ is an escape character on ALL sh-related shells.  And .exe 
>> handling on OS/2 should behave as it does on mingw.
>>
> 
> Sorry, my change log was not enough somewhat. I meant 'echo'. At
> least, echo of pdksh treats \ as an escape char without -E.

Yes, passing \ through echo is non-portable, and you must use printf
instead of echo if the string to be echoed is likely to contain a
backslash (in addition to the shell quoting rules that backslash has
outside of echo).

> 
> How does mingw handle .exe ?

Configure probes early on if .exe is produced when compiling an
executable, and sets $EXEEXT accordingly.  But in many cases, executing
'/bin/sh' is automatically translated into '/bin/sh.exe' without the
user having to explicitly request .exe as part of the executable name.
Maybe I'm missing a subtlety of OS/2 and what is automated vs. manually
required, but giving more details will only make your case for this
patch stronger.

> 
> 
>>>
>>> * lib/autoconf/general.m4 (AC_SITE_LOAD): Convert \ in PATH to
>>> /. Add .exe to ac_executable_extensions.
>>
>> This says what you changed, but not why.  A good commit message
>> gives rationale on WHY the change is important, such as a
>> demonstration of what goes wrong without the patch.
>>
> 
> I thought I explained WHY above message.

But you never mentioned that 'echo' was at fault.

> 
>>> --- lib/autoconf/general.m4 |   28 ++++++++++++++++++++++++++++ 1
>>> files changed, 28 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/lib/autoconf/general.m4 b/lib/autoconf/general.m4 
>>> index 77f71d2..5a87d5e 100644 --- a/lib/autoconf/general.m4 +++
>>> b/lib/autoconf/general.m4 @@ -1951,6 +1951,34 @@ do ||
>>> AC_MSG_FAILURE([failed to load site script $ac_site_file]) fi 
>>> done + +if test -n "$OS2_SHELL"; then +  # Backslashes into
>>> forward slashes: +  # The following OS/2 specific code is
>>> performed AFTER config.site +  # has been loaded to allow users
>>> to change their environment there. +  # This strange code is
>>> necessary to deal with handling of backslashes by +  # ksh. +
>>> ac_save_IFS="$IFS" +  IFS="\\" +  ac_TEMP_PATH= +  for ac_dir in
>>> $PATH; do +    IFS=$ac_save_IFS +    if test -z "$ac_TEMP_PATH";
>>> then +      ac_TEMP_PATH="$ac_dir" +    else +
>>> ac_TEMP_PATH="$ac_TEMP_PATH/$ac_dir" +    fi +  done +  export
>>> PATH="$ac_TEMP_PATH" +  unset ac_TEMP_PATH

Your email client is horribly botching quoting.

>>
>> It looks like this is an (overly-complex) way of converting all \
>> in $PATH into / before proceeding.  But why is it necessary?
>>
> 
> As I said above, without this, echoing components of PATH may be
> corrupted. For examples, x:\usr\bin will be x:\usin on pdksh.

Only if you do something non-portable like 'echo "$PATH"'.  If you do
'printf %s\\n "$PATH"', the problem goes away.

> 
>>> + +  # add .exe to ac_executable_extensions +  if test -z
>>> "$ac_executable_extensions"; then +
>>> AC_MSG_WARN([ac_executable_extensions not set, assuming .exe]) +
>>> fi +  ac_executable_extensions="$ac_executable_extensions .exe" +
>>> export ac_executable_extensions
>>
>> Why is the existing code that sets ac_executable_extensions not 
>> sufficient?
> 
> What is the existing code ? Anyway without this,
> ac_executable_extensions is not set at all.
> 
>> And why do you have to export it into the environment of child
>> processes?
> 
> I just preserved the old codes from OS/2 fork if possible. If it is
> not needed, I'll remove it.

Well, just reposting an old patch calls into play other questions - are
you the original author of the patch, and if not, are there any
copyright restrictions that would prevent us from applying the patch?
Also, just because a downstream fork wrote a patch does not mean it was
the ideal patch; discussing the issues in the open can often lead to a
better understanding of the real issue and a better patch.

> 
> This might be better as two separate patches, since it
>> is doing two unrelated changes.
>>
> 
> I thought both these were OS/2 init codes. Anyway, I'll split.

They are both related to OS/2, but tackling different items.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature