[OS.File] OS.File.move does not remove files on MacOS X

RESOLVED FIXED in mozilla17

Status

()

Toolkit
OS.File
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla17
All
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

There is a problem with OS.File.move. Under MacOS X, it behaves as copy.
Ah, silly me, it's just a missing constant.
Created attachment 649120 [details] [diff] [review]
Adding missing constant

Given that this is a one-liner fix adding a constant, I will just self-review it.
Attachment #649120 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14c95f1a374

Should this have a test?
Flags: in-testsuite?
Keywords: checkin-needed
Good point.
Created attachment 649216 [details] [diff] [review]
Companion testsuite

Here is a test, detached from the bug that originally contained it (and which won't land before a few weeks).
Attachment #649216 - Flags: review?(nfroyd)

Comment 6

6 years ago
https://hg.mozilla.org/mozilla-central/rev/a14c95f1a374
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment on attachment 649216 [details] [diff] [review]
Companion testsuite

Review of attachment 649216 [details] [diff] [review]:
-----------------------------------------------------------------

This would be a nicer change if we had OS.File.exists or some equivalent to access(2).  Probably not worth holding up the bug for that, though.
Attachment #649216 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #7)
> Comment on attachment 649216 [details] [diff] [review]
> Companion testsuite
> 
> Review of attachment 649216 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This would be a nicer change if we had OS.File.exists or some equivalent to
> access(2).  Probably not worth holding up the bug for that, though.

Yes, I will certainly add that at some point.
Confirmed that the new test fails on Try with this bug's patch backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0127c9c41bf2
Flags: in-testsuite? → in-testsuite+
Unfortunately, this test is failing on inbound even with the patch from this bug applied :-(. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d2911bf59e

Some logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14175971&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14175991&tree=Mozilla-Inbound
Flags: in-testsuite+ → in-testsuite?
Yes, for some reason, the fix does not seem to fix the issue on 10.6. I will investigate further.

Thanks, Ryan.
Created attachment 649583 [details] [diff] [review]
More general fix

I have just realized that |copyfile| does not first attempt to rename the file instead of moving it, so my use of that function was not quite appropriate, performance-wise.

Attaching a patch that should both fix the bug on all platforms and fix the performance issue.
Attachment #649583 - Flags: review?(nfroyd)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 649583 [details] [diff] [review]
More general fix

Review of attachment 649583 [details] [diff] [review]:
-----------------------------------------------------------------

So, basically, don't use copyfile to move things, correct?  It's unfortunate that function doesn't do what you'd expect, but perhaps that's by design.
Attachment #649583 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #13)
> Comment on attachment 649583 [details] [diff] [review]
> More general fix
> 
> Review of attachment 649583 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, basically, don't use copyfile to move things, correct?  It's unfortunate
> that function doesn't do what you'd expect, but perhaps that's by design.

Basically, yes. Plus semantics of copyfile seems different between MacOS 10.6 and 10.7.
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/6be6808c2a4a - this or the bug 780604 patch caused Windows permaorange a la

https://tbpl.mozilla.org/php/getParsedLog.php?id=14210455&tree=Mozilla-Inbound
13860 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_back.xul | an unexpected uncaught JS exception reported through window.onerror - missing ; before statement at resource://gre/modules/osfile/osfile_win_back.jsm:85
13867 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_front.xul | SyntaxError: missing ; before statement
Short story: it's the bug 780604 patch, but I am innocent :)
https://hg.mozilla.org/mozilla-central/rev/d6c3c6ccfa58
https://hg.mozilla.org/mozilla-central/rev/cac33abc0c4c
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.