Opened 9 years ago

Closed 9 years ago

#9444 closed defect (fixed)

Fix "rm: Cannot remove any directory in the path of the current working directory" on t2

Reported by: mpatel Owned by: drkirkby
Priority: minor Milestone: sage-4.5.3
Component: porting: Solaris Keywords:
Cc: drkirkby Merged in: sage-4.5.3.alpha0
Authors: Mitesh Patel Reviewers: David Kirkby, John Palmieri, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

In /rootpool2/local/kirkby/sage-4.5.alpha1 on t2:

$ tail spkg/logs/rubiks-20070912.p11.log
real    2m30.575s
user    2m20.699s
sys     0m5.083s
Successfully installed rubiks-20070912.p11
Now cleaning up tmp files.
rm: Cannot remove any directory in the path of the current working directory
/rootpool2/local/kirkby/sage-4.5.alpha1/spkg/build/rubiks-20070912.p11
Making Sage/Python scripts relocatable...
Making script relocatable
Finished installing rubiks-20070912.p11.spkg

This leaves an empty directory SAGE_ROOT/spkg/build/rubiks-20070912.p11.

It seems the problem is

rm -rf "$SAGE_PACKAGES/build/$PKG_NAME"

near the end of SAGE_LOCAL/bin/sage-spkg. What if we precede this with cd .., say?

Attachments (1)

trac_9444-spkg_build_dir_t2.patch (647 bytes) - added by mpatel 9 years ago.
Step out of spkg/build/foo-x.y.z before deleting it.

Download all attachments as: .zip

Change History (13)

Changed 9 years ago by mpatel

Step out of spkg/build/foo-x.y.z before deleting it.

comment:1 Changed 9 years ago by mpatel

  • Authors set to Mitesh Patel

I've attached a trial patch that uses cd "$SAGE_PACKAGES/build/".

comment:2 Changed 9 years ago by drkirkby

This looks good and I'm confident it would fix this rather annoying bug.

My only concern is whether fixing this bug will cause a problem for any code that might have relied on the old behavior. That will need testing.

Dave

comment:3 Changed 9 years ago by jhpalmieri

I think that this message only appears on Solaris; at least, that's been my experience. On sage.math, for example, I can do this:

$ cd /scratch/palmieri
$ mkdir TEMP
$ cd TEMP
$ rm -rf /scratch/palmieri/TEMP

but doing this on t2.math results in an error:

rm: Cannot remove any directory in the path of the current working directory

So if it ever relied on this behavior, it didn't rely on it on sage.math. (All of the linux machines I've used, and also Mac OS X, behave like sage.math in this regard.)

I'm trying a build on sage.math with this patch, and I'll try it on t2 later.

comment:4 follow-up: Changed 9 years ago by jhpalmieri

  • Status changed from new to needs_review

mpatel: I assume this is ready for review?

comment:5 Changed 9 years ago by leif

  • Reviewers set to Leif Leonhardy
  • Status changed from needs_review to positive_review

I'm ok with the patch, though we could replicate

   # Make triply sure that we are in the build directory before doing 
    # a scary "rm -rf".
    cd "$SAGE_PACKAGES/build"
    if [ $? -ne 0 ]; then
        echo "Unable to find build directory."
    else
        rm -rf "$PKG_BASE-"*
    fi

which is what is done some lines above.

There are many other things to fix or improve in sage-spkg, but I'll leave those for further tickets (something like work in progress) since hopefully this one gets merged soon.

If anyone feels Mitesh's solution is not sufficient, feel free to revert it to "needs review" or "needs work".

-Leif

comment:6 Changed 9 years ago by drkirkby

I'm happy too. My initial concern was that the directory was not getting deleted on any operating system, in which case deleting it could be dangerous. But now I realise it gets deleted on Linux, then clearly there is no risk.

I'm 100% happy with this.

Dave

comment:7 follow-up: Changed 9 years ago by leif

  • Reviewers changed from Leif Leonhardy to David Kirkby, John Palmieri, Leif Leonhardy

Just for the record: Besides other things, I'd like to have something like $SAGE_SPKG_KEEP_SRC (or SAGE_KEEP_BUILT_SPKGS) for developers, because -s is not available for whole builds (with make).

comment:8 in reply to: ↑ 4 Changed 9 years ago by mpatel

Replying to jhpalmieri:

mpatel: I assume this is ready for review?

Yes. I didn't change the status field because I had tested the patch only with sage -f on sage.math and t2.

Were there any problems with your builds?

comment:9 Changed 9 years ago by jhpalmieri

No, everything has worked fine.

comment:10 in reply to: ↑ 7 Changed 9 years ago by jhpalmieri

Replying to leif:

Just for the record: Besides other things, I'd like to have something like $SAGE_SPKG_KEEP_SRC (or SAGE_KEEP_BUILT_SPKGS) for developers, because -s is not available for whole builds (with make).

See #4949.

comment:11 Changed 9 years ago by drkirkby

Why is the milestone for this set to 5.0? Do we really need to wait that long to fix this bug, given it already has positive review and is one of the simplest possible bug fixes.

Dave

comment:12 Changed 9 years ago by mpatel

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