#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: |
Description (last modified by )
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
comment:2 Changed 4 years ago by
Description: | modified (diff) |
---|
comment:3 follow-up: 5 Changed 4 years ago by
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
Description: | modified (diff) |
---|
comment:5 Changed 4 years ago by
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
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
Dependencies: | → #26011 |
---|---|
Status: | new → needs_review |
comment:9 Changed 4 years ago by
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:11 Changed 4 years ago by
Resolution: | → fixed |
---|---|
Status: | needs_review → closed |
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
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
Yes, ideally sage-spkg should still be rewritten mostly if not entirely in Python.
Note: This is probably fixed by #26011