Opened 12 years ago

Closed 12 years ago

#7873 closed defect (fixed)

Fix 'gap' to remove usage of '$RM' and replace wth 'rm'

Reported by: drkirkby Owned by: GeorgSWeber
Priority: major Milestone: sage-4.3.1
Component: build Keywords:
Cc: Merged in: sage-4.3.1.rc0
Authors: David Kirkby Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

As agreed here:

http://groups.google.com/group/sage-devel/browse_thread/thread/bd7ae07a1157bead/970aa0dc8fa56ab7?lnk=raot

there is no need to have variables for very basic commands such as 'rm'. Gap relies on the use of $RM, $LN and $MKDIR, which seems a bit pointless. All are standard Unix commands, defined by POSIX. We should not make make use of any special options some versions might use.

I'm no fan of GNU, but even their coding standards acknowledge one can assume some commands exist, and include all of these.

http://www.gnu.org/prep/standards/standards.html#Utilities-in-Makefiles

Hence I would replace the use of $LN, $RM and $MKDIR on the spkg-install and anywhere else it may be found in gap.

Attachments (1)

gap-#7873.patch (2.7 KB) - added by drkirkby 12 years ago.
patch file to replace $RM with 'rm' and similar

Download all attachments as: .zip

Change History (8)

Changed 12 years ago by drkirkby

patch file to replace $RM with 'rm' and similar

comment:1 Changed 12 years ago by drkirkby

  • Status changed from new to needs_review

comment:2 Changed 12 years ago by drkirkby

  • Authors set to David Kirkby

comment:3 follow-up: Changed 12 years ago by jhpalmieri

Two comments: you've changed "$CP" to "cp" even though $CP is still defined in sage-env. Does this matter?

Also (and this is not new -- it happens with the old spkg, too): when I install the spkg, I get this at the end:

cp: ../../bin is a directory (not copied).
cp: cp: No such file or directory

Any ideas why? Anyway, since gap works, maybe we shouldn't worry about this right now.

comment:4 in reply to: ↑ 3 Changed 12 years ago by drkirkby

Replying to jhpalmieri:

Two comments: you've changed "$CP" to "cp" even though $CP is still defined in sage-env. Does this matter?

No. I looked at that before deciding to replace them, but could see no reason not to in this case. One was a simple copy

cp patches/gap_cygwin "$SAGE_LOCAL"/bin/gap

the other was recursive, but simply used the POSIX compatible '-r' option. There seemed to be no reason to use any other version of cp in such cases.

The GNU verison of 'cp' has some non-POSIX options (-a being one of them). Had that be iused in gap, then I would have left the $CP, but in this case, with only a very standard option used, there is no reason not to use whatever version of 'cp' is in the path first. Any 'cp' will work.

Also (and this is not new -- it happens with the old spkg, too): when I install the spkg, I get this at the end:

cp: ../../bin is a directory (not copied).
cp: cp: No such file or directory

Any ideas why? Anyway, since gap works, maybe we shouldn't worry about this right now.

Looking at the 'cp' (or $CP) command I could not work out what spkg-install was trying to do (I just posted something on sage-devel about it). The code simply makes no sense to me.

Since

  • The package functions, despite the errors.
  • I wanted to do it asap, in case it caused an issue with the sage-env ticket (#7818)
  • I could not work out what was the intended behavior. 'cp' is used in a way I'd never use it.
  • There is talk on sage-devel of updating gap

it seemed like it was best left to another day. Like the fact CC and CXX get unset. I think it would be wise to find a way around the issues this creates, but again I did not attempt to fix it. That will certainly present a problem if one tried to use a Sun compiler to build gap.

I'm not even convinced this will work in 64-bit mode on OS X, as it does not have the SAGE64 stuff which every other spkg-install file has.

So, overall, the changes I made were only necessary ones, and no others.

Dave

comment:5 Changed 12 years ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

It works for me on OS X 10.6, and the changes are obviously the right ones to make. Positive review.

comment:6 Changed 12 years ago by drkirkby

Thank you very much for the positive review.

comment:7 Changed 12 years ago by rlm

  • Merged in set to sage-4.3.1.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.