Re: Split patch for Mac OS/ was Re: Help with Building toolchain with crosstool-ng on Mac OS X 10.6.2 (Snow Leopard)

[Titus von Boxberg <tvb377 at gmx dot de>]

Hello Yann, all,

Am 18.02.2010 um 21:33 schrieb Yann E. MORIN:
>> diff --git a/configure b/configure
>> --- a/configure
>> +++ b/configure
> [--SNIP--]
>> -has_or_abort prog="awk gawk" ver='^GNU Awk' err="GNU 'awk' was not found"
>> +has_or_abort prog="gawk awk" ver='^GNU Awk' err="GNU 'awk' was not found"
> Huh? Why do you need to switch the check ?
As far as I remember has_or_abort did not check for the 2nd prog name, and
the linux boxes in my company have gawk as well, so: pure lazyness ;-)
awk is not called by ctng (at least not in incompatible
ways) and many if not all tool configures look for gawk, anyway.
Can insert that into the GNU wrapper --with-xx-list as well.

> [--SNIP--]
>> -has_or_abort prog=stat ver='GNU coreutils'
>> +has_or_abort prog=stat
> 
> One separate patch for that, please. I want to be able to revert just this
> in case it breaks later, whithout reverting the whole patch.
I kindly but strongly object.
This patch by itself does not and can not break anything for
GNU systems because stat really is a *core*util. I cannot think of a
nonbroken GNU/Linux system where stat is not from GNU.
In fact, there were only 2 major (multiple times throughout the code)
non-portability items in ctng that could and should (because of the nature
being "coreutils") not be fixed with the imo brilliant wrapper construct,
namely stat and readlink.
So, if you want to keep ctng portable in the future, keep an eye on
those two but do not revert this patch.
Reverting this patch renders portability work useless.
I would even recommend to delete the line to make it consistent
with readlink not being checked for as well, and because I
cannot think of any nonbroken system without stat.

What do you think of this?

> --> sed-portable:
> 
> OK, extended regular expressions are not really needed in this case.
> I will apply!
> 
> Note however that there are many other places where sed is called with -r.
> Did you make sure those where not impacted also?
I'm pretty sure that this is the only location where the links to GNU
tools have not yet been set up.

> --> stat-portable:
> 
>> diff --git a/scripts/build/internals.sh b/scripts/build/internals.sh
>> --- a/scripts/build/internals.sh
>> +++ b/scripts/build/internals.sh
>> @@ -72,7 +72,7 @@
>>         # scripts, we don't know if they would in the end spawn a
>> binary... # Just skip symlinks
>>         for _t in "${CT_TARGET}-"*; do
>> -            if [ "$( LANG=C stat -c '%F' "${_t}" )" != "symbolic link" ]; then
>> +            if [ -z "`readlink ${_t}`" ]; then 
> 
> The rest of the code uses $(...) instead of `...` Although there is
> nothing wrong with `...`, I'd like to keep the code homogeneous.
> 
> And separate patch for that, please, as it's not related to the rest
> of the patch.
It is related because it replaces a non portable call to stat
which is what the rest of this patch does, too.

> 
>> +            ;;
>> +        *)
>> +            echo "OS unknown";
> 
> Redirect to stderr!
CT_DoLog maybe?

>> -src_inode=$(stat -c '%i' "${CT_ROOT_SRC_DIR}/.")
>> -dst_inode=$(stat -c '%i' "${CT_ROOT_DST_DIR}/." 2>/dev/null || true)
>> +src_inode=$(CT_getinode "${CT_ROOT_SRC_DIR}/.")
>> +dst_inode=$(CT_getinode "${CT_ROOT_DST_DIR}/." 2>/dev/null || true)
> 
> Oh well, stderr is consigned to oblivion here, which means you wont see
> the "OS unknown" error message anyway...
> 
>> if [ "${src_inode}" -eq "$((dst_inode+0))" ]; then
>>     echo "$myname: source and destination are the same!"
>>     exit 1
> Separate patch, please.
> Seems we can do way better here: move the '2>/dev/null || true' up into
> the function.
Imo such a function with a name suggesting general usability
should normally complain visibly when the file was not found.
So I would opt to leave the /dev/null where it is.
Or didn't I get the message right?

> Could you rework your patches, please? Thank you! :-)
Will do so.

Regards
Titus


--
For unsubscribe information see http://sourceware.org/lists.html#faq