Closed
Bug 971525
Opened 11 years ago
Closed 11 years ago
Make FileCopier only delete symlinked directories that it needs to replace
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla30
People
(Reporter: nalexander, Assigned: nalexander)
Details
Attachments
(1 file, 2 obsolete files)
At the moment, if FileCopier is writing to dest/, and dest/link_to_directory is a symlink to a directory, it will be deleted. (And not noted as a deleted file or directory.) This happens even if remove_unaccounted=False.
I propose to make FileCopier more conservative and only remove symlinks to directories that get in its way. That is, dest/link_to_directory will be deleted if remove_unaccounted=True, or if dest/link_to_directory/foo is a path in the container, but will be left alone otherwise.
Conceptually, this treats dest/link_to_directory like a file that will be deleted and replaced with a directory if it is a directory containing a path.
Assignee | ||
Comment 1•11 years ago
|
||
This springs out of https://bugzilla.mozilla.org/show_bug.cgi?id=853045#c42 and https://bugzilla.mozilla.org/show_bug.cgi?id=853045#c51.
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #8374617 -
Flags: review?(gps)
Comment 3•11 years ago
|
||
Comment on attachment 8374617 [details] [diff] [review]
Make FileCopier only delete symlinked directories that it needs to replace. r=gps
Review of attachment 8374617 [details] [diff] [review]:
-----------------------------------------------------------------
Please put this behind a flag and add comments indicating it should only be used in exceptional circumstances.
This code was written to deal with files, not directories. Tricking things into believing a directory symlink is a file is a nice hack. But it seems fragile to me.
If you want to do this right, I'd add a new registry entry for directory symlinks. But that's a lot of code. I don't think you want to write that now. Best to hack around it with an optional, non-default flag to unblock you methinks.
Attachment #8374617 -
Flags: review?(gps) → review-
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #3)
> Comment on attachment 8374617 [details] [diff] [review]
> Make FileCopier only delete symlinked directories that it needs to replace.
> r=gps
>
> Review of attachment 8374617 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Please put this behind a flag and add comments indicating it should only be
> used in exceptional circumstances.
Will do.
> This code was written to deal with files, not directories. Tricking things
> into believing a directory symlink is a file is a nice hack. But it seems
> fragile to me.
>
> If you want to do this right, I'd add a new registry entry for directory
> symlinks. But that's a lot of code. I don't think you want to write that
> now. Best to hack around it with an optional, non-default flag to unblock
> you methinks.
It is my understanding that on disk, a symlink is a file, no matter what it points to. That's why stat and lstat both exist. To my eye the File instance for a directory symlink would just be the code for a regular symlink. It's the same, since the resulting file is the same, and the resulting over-write is the same.
But I just want movement, so I'll put this behind a flag. Thanks for looking at it!
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8374617 -
Attachment is obsolete: true
Attachment #8375692 -
Flags: review?(gps)
Assignee | ||
Comment 6•11 years ago
|
||
Forget to fold small fixes in, and updated that comment I flagged.
Attachment #8375692 -
Attachment is obsolete: true
Attachment #8375692 -
Flags: review?(gps)
Attachment #8375698 -
Flags: review?(gps)
Updated•11 years ago
|
Attachment #8375698 -
Flags: review?(gps) → review+
Assignee | ||
Comment 7•11 years ago
|
||
Comment 8•11 years ago
|
||
Assignee: nobody → nalexander
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•