862017 - port windows clobberer fixes to mozharness

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Aki Sasaki (not active)]

http://hg.mozilla.org/build/tools/rev/92c703e1c26d
http://hg.mozilla.org/build/tools/rev/1c40178a3527

Comment 1

[Aki Sasaki (not active)]

Attachment: windows clobber -- hangs :(

Ported to mozharness.
Unfortunately, this hangs when running on w64-ix-slave22 (via ssh, at least).
Needs investigation.  I'm going to hold off on this for now.

Comment 2

[Aki Sasaki (not active)]

Oops, I see a bug.  s,dir,path,

Comment 3

[Aki Sasaki (not active)]

Attachment: windows clobber -- working!!

Comment 4

[Aki Sasaki (not active)]

Attachment: windows clobber, renamed

This patch:

* ports your clobberer fixes to mozharness, with some changes
** allows for non-existent paths or files to be sent to _rmdir_windows(), since that's how mozharness' rmtree() works (more analogous to rm -rf)

Passes nosetests on mac and windows.

Let me know if you have any questions.

Comment 5

[John Hopkins (:jhopkins)]

Comment on attachment 738843 [details] [diff] [review]
windows clobber, renamed

>@@ -63,92 +67,91 @@ class ScriptMixin(object):
> 
>     def rmtree(self, path, log_level=INFO, error_level=ERROR,
>                exit_code=-1):
>         """
>         Returns None for success, not None for failure
>         """
>         self.log("rmtree: %s" % path, level=log_level)
>         if os.path.exists(path):

Keep in mind that this os.path.exists check will fail if the path is longer than MAX_PATH.
As long as nobody passes in an initial path that long then you won't have problems.

If you do want to handle that condition, you'd need to:
- check for os='nt' first
 - change the path to an absolute path
 - do os.path.exists('\\\\?\\' + path)

>+    def _rmtree_windows(self, path):
>+        """ Windows-specific rmtree that handles path lengths longer than MAX_PATH.
>+            Ported from clobberer.py.
>+        """
>+        self.info("Using _rmtree_windows ...")
>+        assert self._is_windows()
>+        path = os.path.realpath(path)
>         if not os.path.exists(path):
>             return

Tested that this os.path.exists fails on a very long path because it is missing the '\\\\?\\' prefix like the other system calls.

Other tests you should consider adding to your test suite:

- test deletion of paths long than MAX_PATH (which would find missing instances of '\\\\?\\')
- test deletion of read-only files (ensuring SetFileAttributesW works as expected)

Comment 6

[Aki Sasaki (not active)]

(In reply to John Hopkins (:jhopkins) from comment #5)
> Other tests you should consider adding to your test suite:
> 
> - test deletion of paths long than MAX_PATH (which would find missing
> instances of '\\\\?\\')
> - test deletion of read-only files (ensuring SetFileAttributesW works as
> expected)

Do you know how I should go about doing this?

I tried:

1) mkdir -p REALLY_LONG_PATH (got error)
2) unzip ZIPFILE (with a really long path, got error)

I could try adding a really long path into mozharness itself, but if the rmtree code ever broke that would effectively burn our entire windows pool.

Comment 7

[Aki Sasaki (not active)]

Attachment: review comments addressed, sans additional tests

Comment 8

[Aki Sasaki (not active)]

(comment 6)

Comment 9

[John Hopkins (:jhopkins)]

Attachment: sample script to create very long path on Windows

Aki: I used the attached script for my own testing of long path removal.

Comment 10

[Aki Sasaki (not active)]

Attachment: with tests

Comment 11

[John Hopkins (:jhopkins)]

Comment on attachment 740416 [details] [diff] [review]
with tests

Looks good!

Comment 12

[Aki Sasaki (not active)]

Comment on attachment 740416 [details] [diff] [review]
with tests

https://hg.mozilla.org/build/mozharness/rev/0308cdea2f32

Comment 13

[Aki Sasaki (not active)]

Comment on attachment 740416 [details] [diff] [review]
with tests

Backed out, because this adds a pywin32 dependency to mozharness, and our test slaves don't all have pywin32 installed for newer python.

:'(

Comment 14

[Aki Sasaki (not active)]

The new win32 test slaves (iX) will have pywin32 installed on py27.
I can land this then.

Comment 15

[Aki Sasaki (not active)]

As :catlee noted, I can also do a try import/except, and fall back to rmdir /s /q.

Comment 16

[Aki Sasaki (not active)]

Attachment: fall back to rmdir /s /q if there isn't any pywin32

Sorry for all the reviews; the previous patch failed on test boxes due to lack of pywin32.

This passed tests on both a win64 build machine and an xp test machine (no pywin32 on py27).

Comment 17

[John Hopkins (:jhopkins)]

Comment on attachment 740594 [details] [diff] [review]
fall back to rmdir /s /q if there isn't any pywin32

One nit pick - the '\\\\?\\' prefix isn't needed when shelling out to DOS since it can't handle long paths anyway.

+        full_path = '\\\\?\\' + path
+        if not os.path.exists(full_path):
             return
+        if not PYWIN32:
+            if not os.path.isdir(full_path):
+                return self.run_command('del /F /Q "%s"' % full_path)
+            else:
+                return self.run_command('rmdir /S /Q "%s"' % full_path)

Comment 18

[Aki Sasaki (not active)]

Comment on attachment 740594 [details] [diff] [review]
fall back to rmdir /s /q if there isn't any pywin32

http://hg.mozilla.org/build/mozharness/rev/d25a66667956

Comment 19

[Aki Sasaki (not active)]

Merged m-c to Cedar to verify before merging to production.

Comment 20

[Aki Sasaki (not active)]

In mozharness production.