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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



Eric Blake wrote:
> 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).
> 

I know, too. Using echo is not me but many projects using autotools.
Frankly, I did not look into autotools itself whether or not it uses
echo. But it seems that many projects using autotools assumes a path
does not include '\'. To avoid this, this patch is reasonable.

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

Ah, OK. Unfortunately, however, a compiler itself cannot be located
without ac_executable_extensions. Therefore, $EXEEXT cannot be set as
well. I know, '/bin/sh' to /bin/sh.exe' translation also depends on
ac_executable_extensions or $EXEEXT.


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

I admit. ^^

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

Ooops... That's too bad in my point of view. Hmm...

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

As I said above, it's not me who using echo. ^^

>> 
>>>> + +  # 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?

Frankly, I don't know who is the original author. I know, the last
porter is Andreas Buening. So I just guess he would be the author of
this patch. He opened the patches. I think, copyright restriction is
none. But I'm not sure.

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

Yes, this is exactly one of the reasons why I submit these patches.

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

Ok.

- -- 
KO Myung-Hun

Using Mozilla SeaMonkey 2.7.2
Under OS/2 Warp 4 for Korean with FixPak #15
In VirtualBox v4.1.32 on Intel Core i7-3615QM 2.30GHz with 8GB RAM

Korean OS/2 User Community : http://www.ecomstation.co.kr

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (OS/2)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iD8DBQFUIPewE9YstvghgroRAnzvAJ9Gjp8V7CQrbLNV9f11y7apJ4mDMACfR4XO
3hhxJ7Zcjr7BgzVwtsyfUbA=
=gGgs
-----END PGP SIGNATURE-----