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

See also https://bugzilla.mozilla.org/show_bug.cgi?id=859426

see also https://bugzilla.mozilla.org/show_bug.cgi?id=860426 and https://bugzilla.mozilla.org/show_bug.cgi?id=861271

mozfile.rmtree() is depricated in favor of mozfile.remove() and will be removed in the future. So updating the summary.

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.

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

Ok Jeff, thank you. :)

Posted this again to ask review by William this time.

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".

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.

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?

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.

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.

This is a documentation only change, so we can probably mark this as DONTBUILD if you want to save a few cycles.

William, do we have to manually trigger an update to refresh the docs at http://mozbase.readthedocs.org/en/latest/mozfile.html?

(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)