Opened 11 years ago

Closed 8 years ago

#3537 closed defect (fixed)

Change $RM in sage-env

Reported by: gfurnish Owned by: gfurnish
Priority: major Milestone: sage-4.7.1
Component: scripts Keywords:
Cc: mabshoff, Snark, drkirkby, jdemeyer Merged in: sage-4.7.1.alpha1
Authors: Jeroen Demeyer Reviewers: Mariah Lenox
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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:

Apply trac_3537-unset-RM.patch

Attachments (3)

trac_3537_scripts.patch (501 bytes) - added by gfurnish 11 years ago.
trac_3537-doc.patch (2.3 KB) - added by jhpalmieri 8 years ago.
trac_3537-unset-RM.patch (558 bytes) - added by jdemeyer 8 years ago.
Do not set $RM in sage-env

Download all attachments as: .zip

Change History (30)

Changed 11 years ago by gfurnish

comment:1 Changed 11 years ago by gfurnish

  • Milestone set to sage-3.0.4
  • Owner changed from mabshoff to gfurnish
  • Status changed from new to assigned

comment:2 Changed 11 years ago by ghtdak

  • Summary changed from [with patch, needs review] sage-env breaks makefile RM to [with patch, positive review] sage-env breaks makefile RM

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

comment:3 Changed 11 years ago by mabshoff

  • Priority changed from blocker to major
  • Summary changed from [with patch, positive review] sage-env breaks makefile RM to [with patch, mixed review] sage-env breaks makefile RM

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

comment:4 Changed 11 years ago by mabshoff

  • Milestone changed from sage-3.0.4 to sage-duplicate/invalid
  • Resolution set to wontfix
  • Status changed from assigned to closed

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

comment:5 follow-up: Changed 9 years ago by vbraun

  • Authors set to gfurnish
  • Cc mabshoff Snark added
  • Description modified (diff)
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-4.6.2
  • Report Upstream set to N/A
  • Resolution wontfix deleted
  • Status changed from closed to new
  • Summary changed from [with patch, mixed review] sage-env breaks makefile RM to [with patch] sage-env should set RM="rm -f"

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?

comment:6 Changed 9 years ago by vbraun

  • Status changed from new to needs_review

comment:7 in reply to: ↑ 5 ; follow-up: Changed 9 years ago by leif

  • Cc drkirkby jdemeyer added

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.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 9 years ago by jdemeyer

Why does RM need to be defined at all?

comment:9 in reply to: ↑ 8 ; follow-up: Changed 9 years ago by leif

Replying to jdemeyer:

Why does RM need to be defined at all?

Because

${RM} some_file

will (hopefully) fail if RM is not set or empty?

Imagine some_file was executable in that case...


It would be a bit odd to test and eventually set all such variables individually in every script that uses some of these, therefore we have sage-env.

Though CP, RM etc. are meanwhile hardly used within Sage; perhaps because setting them isn't necessary on any proper OS...

Instead, it would be better to have e.g. EGREP, (POSIX-conformant) GREP etc., detected / set by Sage's configure.

comment:10 Changed 9 years ago by vbraun

I seriously doubt that anybody uses the "destructive existence test" $RM filename || ... :-)

Automake has an official list of variables that it looks at. Perhaps it is a bug that RM is on the list, but its still our problem and I doubt upstream will change because some guys from Sage have a variable name collision. See also http://www.gnu.org/software/automake/manual/make/Implicit-Variables.html

comment:11 in reply to: ↑ 9 ; follow-up: Changed 9 years ago by jdemeyer

Replying to leif:

Because

${RM} some_file

will (hopefully) fail if RM is not set or empty?

Where is such code used? My guess: nowhere.

comment:12 in reply to: ↑ 11 Changed 9 years ago by leif

Replying to jdemeyer:

Replying to leif:

Because

${RM} some_file

will (hopefully) fail if RM is not set or empty?

Where is such code used? My guess: nowhere.

E.g. Singular's spkg-install does. (It actually does $RM -f ....)

I wouldn't base such changes on guesses... ;-)

comment:13 follow-up: Changed 9 years ago by vbraun

For the record:

[vbraun@volker-desktop spkg-unpacked]$ grep '\$RM' */*
gap-4.4.12.p4/SPKG.txt: * #7873 Remove references to $LN, $MKIDR and $RM and replace
mercurial-1.6.4.p0/SPKG.txt: * Changes occurrences of $RM to 'rm', $CP to 'cp' and similar.
sage_scripts-4.6.1.alpha3.p0/sage-env:if [ "$RM" = "" ]; then
singular-3-1-1-4.p3/spkg-install:        $RM -f LIB
singular-3-1-1-4.p3/spkg-install:    $RM -f Singular singular
singular-3-1-1-4.p3/spkg-install:        $RM -f Singular-1-0-2   # remove previous version of Singular.

comment:14 in reply to: ↑ 13 Changed 9 years ago by leif

Replying to vbraun:

For the record:

[vbraun@volker-desktop spkg-unpacked]$ grep '\$RM' */*

Redo for ${RM}, too? (Also $(RM) in Makefiles.)

Not sure if there are some "deeper" instances as well.

comment:15 Changed 9 years ago by vbraun

[vbraun@volker-desktop spkg-unpacked]$ grep '\$.RM' */*
singular-3-1-1-4.p3/install-sh:rmprog="${RMPROG-rm}"

and

[vbraun@volker-desktop spkg-unpacked]$ grep '\$.*RM' */*
gap-4.4.12.p4/SPKG.txt: * #7873 Remove references to $LN, $MKIDR and $RM and replace
mercurial-1.6.4.p0/SPKG.txt: * Changes occurrences of $RM to 'rm', $CP to 'cp' and similar.
sage_scripts-4.6.1.alpha3.p0/sage-check-64:		case $CONFIRM in
sage_scripts-4.6.1.alpha3.p0/sage-env:if [ "$RM" = "" ]; then
singular-3-1-1-4.p3/install-sh:rmprog="${RMPROG-rm}"
singular-3-1-1-4.p3/spkg-install:        $RM -f LIB
singular-3-1-1-4.p3/spkg-install:    $RM -f Singular singular
singular-3-1-1-4.p3/spkg-install:        $RM -f Singular-1-0-2   # remove previous version of Singular.

comment:16 Changed 8 years ago by jhpalmieri

So right now, singular is the only place this is used, and it consistently uses "$RM -f", right? Given this, rather than set RM="rm -f", it seems better to leave it as is and document it in the developer's guide (in the section on producing new spkg's), and perhaps also in the installation guide (in the section on environment variables). Here's a patch doing that; what do you think?

Changed 8 years ago by jhpalmieri

comment:17 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

comment:18 follow-up: Changed 8 years ago by jdemeyer

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

comment:19 in reply to: ↑ 18 Changed 8 years ago by drkirkby

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

#!/bin/sh

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

comment:20 Changed 8 years ago by jhpalmieri

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

comment:21 Changed 8 years ago by mariah

  • Description modified (diff)
  • Reviewers set to Mariah Lenox
  • Status changed from needs_review to positive_review

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.

comment:22 Changed 8 years ago by jdemeyer

  • Component changed from build to scripts
  • Description modified (diff)
  • Milestone changed from sage-4.7 to sage-4.7.1
  • Status changed from positive_review to needs_work
  • Summary changed from [with patch] sage-env should set RM="rm -f" to Change $RM in sage-env

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

Changed 8 years ago by jdemeyer

Do not set $RM in sage-env

comment:23 follow-up: Changed 8 years ago by mariah

  • Status changed from needs_work to positive_review

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

comment:24 in reply to: ↑ 23 Changed 8 years ago by jdemeyer

Replying to mariah:

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.

comment:25 Changed 8 years ago by mariah

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.

comment:26 Changed 8 years ago by jdemeyer

  • Authors changed from gfurnish to Jeroen Demeyer

comment:27 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed

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

Note: See TracTickets for help on using tickets.