Opened 14 years ago
Closed 11 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 )
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": trac_3537_scripts.patch
- keep $RM as it is, but document it: trac_3537-doc.patch (needs_work)
- don't set $RM at all: trac_3537-unset-RM.patch
Attachments (3)
Change History (30)
Changed 14 years ago by
comment:1 Changed 14 years ago by
- Milestone set to sage-3.0.4
- Owner changed from mabshoff to gfurnish
- Status changed from new to assigned
comment:2 Changed 14 years ago by
- Summary changed from [with patch, needs review] sage-env breaks makefile RM to [with patch, positive review] sage-env breaks makefile RM
comment:3 Changed 14 years ago by
- 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 14 years ago by
- 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: ↓ 7 Changed 12 years ago by
- 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 12 years ago by
- Status changed from new to needs_review
comment:7 in reply to: ↑ 5 ; follow-up: ↓ 8 Changed 12 years ago by
- 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-install
s 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: ↓ 9 Changed 12 years ago by
Why does RM
need to be defined at all?
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 11 Changed 12 years ago by
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 12 years ago by
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: ↓ 12 Changed 12 years ago by
Replying to leif:
Because
${RM} some_filewill (hopefully) fail if
RM
is not set or empty?
Where is such code used? My guess: nowhere.
comment:12 in reply to: ↑ 11 Changed 12 years ago by
comment:13 follow-up: ↓ 14 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
[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 11 years ago by
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 11 years ago by
comment:17 Changed 11 years ago by
- Description modified (diff)
comment:18 follow-up: ↓ 19 Changed 11 years ago by
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 11 years ago by
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 11 years ago by
See #9497 for a patch to change "$RM" to "rm" in Singular's spkg-install script.
comment:21 Changed 11 years ago by
- 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 11 years ago by
- 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.
comment:23 follow-up: ↓ 24 Changed 11 years ago by
- 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 11 years ago by
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 11 years ago by
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 11 years ago by
comment:27 Changed 11 years ago by
- 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.
The intent of -f is to avoid errors for the conditions cited as a problem.