Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#26153 closed defect (fixed)

sage-spkg does not correctly overwrite existing directories

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-8.4
Component: build Keywords:
Cc: embray Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #26011 Stopgaps:

GitHub link to the corresponding issue

Description (last modified by jdemeyer)

This line in sage-spkg

$SAGE_SUDO mv "$PREFIX/$filename" "${SAGE_LOCAL%/}/$filename"

might move a file $filename (to be installed) into a directory $filename (if the latter was an existing directory).

This can occur in particular with symlinks to directories, since those can be interpreted both as file and as directory. This is partially responsible for #26152.

Change History (13)

comment:1 Changed 4 years ago by embray

Note: This is probably fixed by #26011

comment:2 Changed 4 years ago by jdemeyer

Description: modified (diff)

comment:3 Changed 4 years ago by embray

Description: modified (diff)

if the latter was an existing directory

Or, it seems, a symlink pointing to a directory. That's the real problem.

comment:4 Changed 4 years ago by jdemeyer

Description: modified (diff)

comment:5 in reply to:  3 Changed 4 years ago by jdemeyer

Replying to embray:

Or, it seems, a symlink pointing to a directory. That's the real problem.

Yes, I clarified that in an edit of the description that you reverted.

comment:6 Changed 4 years ago by embray

It looks like we were just posting at the same time--sorry, I didn't notice the warning.

comment:7 Changed 4 years ago by dimpase

Dependencies: #26011
Status: newneeds_review

comment:8 Changed 4 years ago by dimpase

Do I understand it right that the branch on #26011 fixes this?

comment:9 Changed 4 years ago by embray

It would. The problem here was that I was manually calling mv on each file one-by-one, which in its own right is a bit slow when you have lots of files to move. But there are some unfortunate subtleties to mv, especially when symlinks are involved, that were not taken into account (see the problem Jeroen described in this ticket). #26011 would replace N mv calls with a single cp -r of the entire directory tree, which AFAICT handles cases like symlinks well too.

In #26011 Jeroen raised some valid concerns about that approach which I attempted to answer, but I admit I don't know what the best play is here (though most online resources I've scoured really do seem to suggest that using cp is the most robust approach in most cases, even if that means needless copying of data. But I'm definitely up for other approaches if they can be suggested...

comment:10 Changed 4 years ago by vbraun

Duplicate?

comment:11 Changed 4 years ago by embray

Resolution: fixed
Status: needs_reviewclosed

I wouldn't say it's a duplicate per se, but since #26011 has been accepted we should be able to call this fixed as well. I'm still not sure if Jeroen will be satisfied with the solution to this (I'm not entirely either) but if we revisit the issue we should just make sure this case is preserved s well.

comment:12 Changed 4 years ago by jdemeyer

It's not really "fixed" by #26011 in the sense that the condition now becomes a hard error (after #26642), failing the installation. Ideally, it should overwrite any existing directories.

That being said, I think that you can only do so much with simple shell scripts. Anything more complicated should really be done in Python.

comment:13 Changed 4 years ago by embray

Yes, ideally sage-spkg should still be rewritten mostly if not entirely in Python.

Note: See TracTickets for help on using tickets.