Closed Bug 859422 Opened 7 years ago Closed 5 years ago

document why mozfile.remove works better than shutil.rmtree

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: k0scist, Assigned: parkouss)

References

Details

Attachments

(2 files, 1 obsolete file)

https://github.com/mozilla/mozbase/blob/master/mozfile/mozfile/mozfile.py#L95

Our docstring:

    """Removes the specified directory tree

This is a replacement for shutil.rmtree that works better under
windows."""
    # (Thanks to Bear at the OSAF for the code.)

We should document:
* why it works better under windows
** including the precise errors that can be encountered via windows
   with shutil.rmtree that are avoided here
* what errors mozfile.rmtree does *not* fix on windows (that work on
  other platforms)
** and file bugs on them
* and of course we should have tests illustrating all of this

https://bugzilla.mozilla.org/show_bug.cgi?id=859178#c1
mozfile.rmtree() is depricated in favor of mozfile.remove() and will be removed in the future. So updating the summary.
Summary: document why mozfile.rmtree works better than shutil.rmtree → document why mozfile.remove works better than shutil.rmtree
I've updated the documentation and added one test case.

Some related tests were already present in mozbase/mozfile/tests/test_remove.py.

Unfortunately I don't know what errors mozfile.rmtree does *not* fix on windows, so this part may be not fixed.
Attachment #8503623 - Flags: review?(k0scist)
Assignee: nobody → j.parkouss
Comment on attachment 8503623 [details] [diff] [review]
document why mozfile.remove works better than shutil.rmtree

I am no longer at Mozilla and so am not comfortable reviewing this change. Superficially, it looks good
Attachment #8503623 - Flags: review?(k0scist)
Ok Jeff, thank you. :)
Posted this again to ask review by William this time.
Attachment #8503623 - Attachment is obsolete: true
Attachment #8503630 - Flags: review?(wlachance)
Comment on attachment 8503630 [details] [diff] [review]
document why mozfile.remove works better than shutil.rmtree

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

Looks great. I assume you ran the unit tests to make sure they pass? If so, please say so and mark this as "checkin-needed".
Attachment #8503630 - Flags: review?(wlachance) → review+
I ran the tests on my own laptop but not pushed to try (never done that yet). I'm not sure either there is a push try for mozbase, looking at http://trychooser.pub.build.mozilla.org/.

So I'll add the keyword. Please tell me if a push try is needed, and maybe if you have some time help me on this.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c0d968dc9a29
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Shouldn't we also update the documentation itself? Less people will check the docstrings in the Python code, but http://mozbase.readthedocs.org/en/latest/mozfile.html is still the first place to look. And there we do not even have .remove() but still .rmtree() :/ 

So I checked more and I think we only have to change the following line to replace the former method:
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/docs/mozfile.rst#9

Julian, mind updating it so that the documentation is completely done?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I didn't notice the docs folder yet containing sphinx dox sources; Do you want another patch, or should I fix and replace the first one ? Also, is a review necessary here for that (assuming I only add the new one line patch) ?

Thanks Henrik !
Given that the patch already landed, a follow-up patch would be fine. Not sure how patches for docs are handled in mozbase, so maybe better ask for review from William or Ahal.
Attachment #8504118 - Flags: review?(wlachance)
Comment on attachment 8504118 [details] [diff] [review]
update sphinx documentation for mozfile

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

Looks good. Thanks for the update, and thanks :whimboo for noticing that the documentation needed this change.
Attachment #8504118 - Flags: review?(wlachance) → review+
This is a documentation only change, so we can probably mark this as DONTBUILD if you want to save a few cycles.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/575cbb47203d
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla35 → mozilla36
William, do we have to manually trigger an update to refresh the docs at http://mozbase.readthedocs.org/en/latest/mozfile.html?
Flags: needinfo?(wlachance)
(In reply to Henrik Skupin (:whimboo) from comment #19)
> William, do we have to manually trigger an update to refresh the docs at
> http://mozbase.readthedocs.org/en/latest/mozfile.html?

Annoyingly, yes, you do need to do this because we have no post-commit hook set up. I kicked off a new run, for future reference you should be authorized to do the same (I see your name in the owners list)
Flags: needinfo?(wlachance)
You need to log in before you can comment on or make changes to this bug.