Avoid $RM, $CP and $MV definition in funclib.sh

[praiskup]

It has been reported that those variables cause problems, for example:
https://savannah.gnu.org/support/index.php?108987

The problem is naming: The '$RM' name does not clearly say the semantics of the variable (is that rm -rf, is that rm -f or rm -r, or plain /bin/rm only?). Scripts sourcing funclib.sh or using bootstrap can override those variables with command with slightly different semantics not compatible with semantics we expect in boostrap project. What's worse there are users exporting those variables in environment...

I tend to remove those variables from funclib.sh .. and if some of those is really needed, we can have something like FL_RM (FuncLib) instead of plain RM. There might be potential users of those variables now .. but to me, it looks like it is still worth braking it now, when there is relatively small userbase. Any ideas?

[gvvaughan]

The semantics have been applied inconsistently through the years, to the point where anyone that has tried to set those variables in their libtool environment almost certainly breaks one or more of the call sites :-(

At this point I agree that the cleanest fix is to ignore RM et. al. in the environment and call the commands directly from the users PATH (i.e. use 'rm' instead of '/bin/rm' for example).

My understanding of the intent of these variables is not for the user to override (they can do that with wrapper scripts at the front of their PATH), but so that they can be replaced with say RM=": rm" and affect just the places necessary to make --dry-run and --debug work correctly. So call sites should use 'rm' on their own files or any temporary files not useful for debugging, but use 'eval $RM' on the user's files. I think it would be exceptionally hard to get that just right everywhere and for all the variables involved without excellent test coverage. But still something worth aspiring to.

[praiskup]

[re-sending an email from morning again, through web-form now]
Hi Gary,

On Monday, October 24, 2016 7:15:45 PM CEST Gary V. Vaughan wrote:

At this point I agree that the cleanest fix is to ignore RM et. al. in the environment and call the commands directly from the users PATH (i.e. use 'rm' instead of '/bin/rm' for example).

Yeah, but seeing the following paragraph it would be a step backwards.

My understanding of the intent of these variables is not for the user to override (they can do that with wrapper scripts at the front of their PATH), but so that they can be replaced with say RM=": rm" and affect just the places necessary to make --dry-run and --debug work correctly. So call sites should use 'rm' on their own files or any temporary files not useful for debugging, but use 'eval $RM' on the user's files. I think it would be exceptionally hard to get that just right everywhere and for all the variables involved without excellent test coverage. But still something worth aspiring to.

Thanks! I didn't know this intention. Using this to ease the --dry-run
implementation makes sense. I fail to see what way you'd like to have
all fixed, though.

Does (a) removing such variables from funclib.sh and (b) renaming the
variables in bootsrap.in (to e.g. $BS_RM) look like a good idea?

[gvvaughan]

Hi Pavel,

I think (a) is only a partial fix - if RM, CP et al are going away, they should be replace by hard-coded rm, cp etc in funclib and all its clients too. A partial --dry/run implementation is not useful at all.

For the same reasons (b) does not present a complete solution either.

I would recommend replacing all the environment variables with hard oder command names to be looked up in the users' PATH here- -and probably disabling --dry-run or having it echo a call for implementation if invoked. And then if someone feels sufficiently motivated, a separate patch that audits all the shell command invocations that would do something non-temporary and change those back to eval $BS_RM with a matching BS_RM='echo rm' etc in the dry-run setup...

HTH,
Gary

[gvvaughan]

s/hard oder/hard coded/

thanks autocorrect! ;-)

[praiskup]

On Tuesday, October 25, 2016 8:54:16 AM CEST Gary V. Vaughan wrote:

Hi Pavel,

I think (a) is only a partial fix - if RM, CP et al are going away, they should be replace by hard-coded rm, cp etc in funclib

Fortunately, those variables don't seem to be used directly from
funclib.sh, so we should be safe with (a).

and all its clients too. A partial --dry/run implementation is not useful at all.

Right, clients (e.g. bootstrap.in) are subject of change, that's the (b)
I proposed. But consumers not coming from this repo are potential risk
(I'm able to fix GNU libtool easily, some scripts there define those
variables too).

For the same reasons (b) does not present a complete solution either.

Right, (a) would complement (b) and vice versa, or maybe we just don't
understand each othe? Do you have a concrete example from existing code which
we could break?

I would recommend replacing all the environment variables with hard coded command names to be looked up in the users' PATH here- -and probably disabling --dry-run or having it echo a call for implementation if invoked.

This still sounds like a bit less flexible solution, can you please once
more elaborate?

And then if someone feels sufficiently motivated, a separate patch that audits all the shell command invocations that would do something non-temporary and change those back to eval $BS_RM with a matching BS_RM='echo rm' etc in the dry-run setup...

I feel quite motivated, but I fail to understand. Seems like that if
there's something related to $opt_dry_run, then this is also related to
func_show_eval. But there's no use like eval $cmd where
cmd='$RM ..'. So what we should audit/cover by testsuite?

I'm quite sure now however that we should be fine with avoiding the
$RM-like variables at all. At least in bootstrap script, where the
portability is not that big concern, but also in funclib.sh where are
some "direct" 'rm' and 'mv' calls already (checks for $SED and $GREP).

[gvvaughan]

Hi Pavel,

I see what you mean now. I was talking entirely in the abstract before, and should have looked at the actual code sooner.

I'm not aware of any other direct clients of funclib.sh in the wild (other than Libtool and our two bootstrap repos), so you are quite right that (a) is totally safe.

Now that I've realized that, (b) seems fine to me too. I was worrying that some of the implementation of --dry-run would call in to funclib.sh functions that did rely on being able to change the contents of RM, MV or CP. Searching (on my phone) doesn't come up with anything though.

The audit I was suggesting earlier was to make sure that each instance of $RM, $CP or $MV is paired with an eval so that spaces (either from a dry-run mode echo prefix or -f argument etc) are handled properly. Still worthwhile IMHO, but orthogonal to this issue.

Cheers,
Gary

(and apologies for muddying the conversation earlier)