Change $RM in sage-env

[garyfurnish]

The env variable RM is set to rm instead of rm -f. This breaks newer libtools, for example anything in Fedora 12 or later (libtool 2.2.6, autoconf 2.63, automake 1.11.1). They assume that $RM is either unset or RM="rm -f", that is, deleting non-existing files must not cause an error.

One of the symptoms of this breakage is that configure ends with

rm: cannot remove `libtoolT': No such file or directory

Compile will break later on...

---

There are three approaches presented here:

• change $RM from "rm" to "rm -f": attachment: trac_3537_scripts.patch
• keep $RM as it is, but document it: attachment: trac_3537-doc.patch (needs_work)
• don't set $RM at all: attachment: trac_3537-unset-RM.patch

Apply attachment: trac_3537-unset-RM.patch

CC: @sagetrac-mabshoff @SnarkBoojum @sagetrac-drkirkby @jdemeyer

Component: scripts

Author: Jeroen Demeyer

Reviewer: Mariah Lenox

Merged: sage-4.7.1.alpha1

Issue created by migration from https://trac.sagemath.org/ticket/3537

[garyfurnish]

Attachment: trac_3537_scripts.patch.gz

[sagetrac-ghtdak]

comment:2

The intent of -f is to avoid errors for the conditions cited as a problem.

[sagetrac-mabshoff]

comment:3

I am not convinced yet that this is the right thing to do and it is rather likely that this will break something else in the tree. The solution to M2's problem is to unset RM in spkg-install and then to set RM to some value you desire.

And this is definitely not a blocker for 3.0.4.

Cheers,

Michael

[sagetrac-mabshoff]

comment:4

If M2 depends on the default behavior of RM in its Makefiles it should set RM in its top level makefile. I see no reason to change Sage's behavior and as is if someone sets RM outside of sage-env it is propagated anyway. So this is wontfix.

Cheers,

Michael

[vbraun]

comment:5

I'm reopening this bug since people keep tripping over this issue. We need to fix this or we'll end up with every spkg working around the RM=rm issue. As the spkg maintainer/author, I know that cddlib and TOPCOM both need to unset RM or they won't build. In #10285 it was noted a few days ago that Boehm GC 7.2 also trips over this issue. More and more packages will fail because of this issue as soon as upsteam re-runs autotools...

For the record, gfurnish's patch applies cleanly on Sage-4.6.1.alpha3 (apply to the $SAGE_LOCAL/bin repo)

I'd be happy to give this a positive review. Maybe mabshoff can reconsider his objections?

[vbraun]

Author: gfurnish

[nexttime]

comment:7

Replying to @vbraun:

I'm reopening this bug since people keep tripping over this issue. We need to fix this or we'll end up with every spkg working around the RM=rm issue.

Well, is this a bug?

More and more packages will fail because of this issue as soon as upsteam re-runs autotools...

That's IMHO a problem of autotools. $RM is in general not supposed to not return an error on non-existing files; if autotools were a bit smarter, they would just add -f or whatever might be appropriate. If they redefine the meaning of RM, that's not Sage's problem in the first place; of course spkg maintainers would have to unset RM for upstream packages using (newer) autotools.

I'd be happy to give this a positive review. Maybe mabshoff can reconsider his objections?

Michael has quit a while ago, though he perhaps still reads trac notifications... ;-)

Note that redefining RM in sage-env could actually break other parts of Sage, not necessarily limited to spkgs, since e.g. removing a file which is expected to exist, but doesn't, without raising an error might hide other errors and cause arbitrary behavior.

Also, as Michael said, changing the default value in sage-env doesn't help if RM was already defined (intentionally or not) by the user, so w.r.t. autotools really won't fix. We have to unset RM (or add an appropriate flag to force deletion if it's not already contained) in spkg-installs of such packages anyway to be safe.

I think we should prominently document this issue, and close this ticket again.

[jdemeyer]

comment:18

My opinion still is that RM should simply be left unset (of course, a few install scripts will have to be changed).

[sagetrac-drkirkby]

comment:19

Replying to @jdemeyer:

My opinion still is that RM should simply be left unset (of course, a few install scripts will have to be changed).

I agree 100%.

I can understand the logic of variables like $CC, $CXX, $MAKE, but not $RM. I don't know of any system where one might want to specify what version of rm gets used, so I don't see the point in having a variable for it. If some dump package needs $RM defined, then either try to fix the code so it is not so dumb, or add $RM to dump_package.spkg.

BTW, take a look at

http://boxen.math.washington.edu/home/kirkby/bad-code/sympow-1.018.1.p7/src/Configure

where you will find some interesting use of variable names. A script which starts

has a function 

{{{whichexe()}}}

This function makes use of a non-POSIX command {{{which}}}, so there's no reason for {{{which}}} to exist. Then {{{whichexe()}}} function sets variables for things like SH, RM, SED, whilst checking if the commands (including {{{sh}}}) exist. If not the script exits. An abbreviated version is below. 

{{{
#!/bin/sh
whichexe() {
    if [ -f /bin/$1 ]; then
        echo /bin/$1
        return;
    fi;
    if [ -f /usr/bin/$1 ]; then
        echo /usr/bin/$1
        return;
    fi;
    if [ -f /usr/local/bin/$1 ]; then
        echo /usr/local/bin/$1
        return;
    fi;
    echo `which $1`
}
RM=`whichexe rm`
GREP=`whichexe grep` && echo "#define GREP \"$GREP\"" >> $CONFIG
SED=`whichexe sed` && echo "#define SED \"$SED\"" >> $CONFIG
SH=`whichexe sh` && echo "#define SH \"$SH\"" >> $CONFIG
if [ -z "$SH" ]; then
  echo "**ERROR**: Could not find sh"; exit 1;
else
  echo "SH = $SH"
fi
}}}

I might have cleaned this up, so the current version in Sage might not be quite so dumb. 

Dave

[jhpalmieri]

comment:20

See #9497 for a patch to change "$RM" to "rm" in Singular's spkg-install script.

[sagetrac-mariah]

Reviewer: Mariah Lenox

[sagetrac-mariah]

comment:21

When 50 percent or more of spkgs require $RM to be "rm -f" then
change $RM from "rm" to "rm -f". Until then document.

Positive review.

[jdemeyer]

comment:22

Singular's spkg-install no longer uses $RM, so the documentation is not up to date.

[jdemeyer]

Do not set $RM in sage-env

[sagetrac-mariah]

comment:23

Attachment: trac_3537-unset-RM.patch.gz

attachment: trac_3537-unset-RM.patch applied to sage-4.7.rc2 tested on skynet/taurus with make testlong. All tests passed. Positive review.

[jdemeyer]

comment:24

Replying to @sagetrac-mariah:

attachment: trac_3537-unset-RM.patch applied to sage-4.7.rc2 tested on skynet/taurus with make testlong. All tests passed. Positive review.

Did you try building Sage from source with this patch applied? Because this patch affects the Sage build, not the Sage library.

[sagetrac-mariah]

comment:25

Apologies for not be clear. I first applied the patch to the sage-4.7.rc2 source, then built the source, then did make testlong.

[jdemeyer]

Changed author from gfurnish to Jeroen Demeyer

[jdemeyer]

comment:27

I just checked all spkg-install scripts and $RM is used nowhere anymore.

[jdemeyer]

Merged: sage-4.7.1.alpha1