Re: [PATCH] tests: feed -no-undefined when linking libtool libraries

Hi Peter,

On 27 ก.ย. 2012, at 5:03, Peter Rosin <[email protected]> wrote:
> On 2012-09-26 14:57, Peter Rosin wrote:
>> On 2012-09-22 05:31, Gary V. Vaughan wrote:
> 
> [Heavily snipped]
> 
>> Why not commit the sc_prohibit_test_const_follows_var improvement
>> last, after fixing all violations?
> 
> That doesn't make sense, sorry. But the idea would have worked
> initially, before the check first existed. I.e., write the check,
> fixup violations and commit in smaller digestible chunks, then
> finally commit the actual check preventing any new violations from
> creeping in.

Even then, I'm on the fence, since that doesn't leave a revision to
demonstrate that the syntax-check is finding the violations... on the
other hand, we also don't want to push "broken" commits since that
obviously screws up git bisection.

>>> If not, I will branch before 962aa91, run the script, and then apply
>>> the rest of master to that branch one patch at a time until I arrive
>>> at a diff that I can apply to master itself, rather than using revert
>>> as I did on the current temporary branch.
>> 
>> I have to say, given all these difficulties, is it really worth it?
>> Besides, I think
> 
> Hmmm, I said that in the wrong context. Your plan above seems like the
> best path forward. I guess I was too "excited" about the bugs and didn't
> read properly, sorry again about my poor communication skills.

No apologies necessary.  I'm working on the revised redo, which will
result in a much smaller patch for master later today or tomorrow.

> Some of the noise between master and redo-test-operand-order are a bunch
> of hunks with this pattern:
> -   if test bar $op $foo; then
> +   if test bar $op $foo ; then

Nice catch.  I already tweaked the script regexs to fix that once, but must
have missed one of the ways for that to happen :(

> You should perhaps add a commit to redo-test-operand-order which silences
> that and makes other more substantial changes stand out more before you
> proceed with the above plan? There are perhaps other harmless changes
> that can be excluded from the "light" revert? Because who needs the
> oscillation?

Agreed.  It was all these edge cases that made me abandon the scripted
solution last year.  I'm working on improving the script first to catch this and
the other glitches you found.

> BTW, here is another strange-looking hunk from
> "git diff master redo-test-operand-order"
> 
> diff --git a/m4/libtool.m4 b/m4/libtool.m4
> index 4413a6d..8ec9beb 100644
> --- a/m4/libtool.m4
> +++ b/m4/libtool.m4
> @@ -5563,7 +5563,7 @@ _LT_EOF
>       ;;
>     esac
> 
> -    if test sni = "$host_vendor"; then
> +    if test x$host_vendor = xsni; then
>       case $host in
>       sysv4 | sysv4.2uw2* | sysv4.3* | sysv5*)
>    _LT_TAGVAR(export_dynamic_flag_spec, $1)='$wl-Blargedynsym'
> 
> What has caused the difference in this hunk? Why hasn't the script
> caught this instance? And why isn't the syntax-check triggered?
> Are the "missing" quotes the key here?

Not sure... investigating now.

> The check is named sc_prohibit_test_const_follows_var, so one
> would guess that it should prohibit "test x$foo = xbar", no? Or are
> you supposed to be able to dodge the check by needlessly adding x
> in front of LHS $variables?

Not at all.  But the long list of substitutions must be interacting
unexpectedly.  The idea is for the syntax-check rule to flag any of:

  test x"$foo" = "xbar"
  test x"$foo" = xbar
  test "x$foo" = "xbar"
  test "x$foo" = xbar
  test x$foo = xbar

where 'x' is one of '.', 'x', or 'X'.

I think we're getting much closer to a working solution though.  Many
thanks for taking the time to review.

Cheers,
-- 
Gary V. Vaughan (gary AT gnu DOT org)