port windows clobberer fixes to mozharness

RESOLVED FIXED

Status

Release Engineering
Applications: MozharnessCore
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aki, Assigned: aki)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozharness][pending new win32 test slaves])

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Updated

5 years ago
Blocks: 858797
No longer blocks: 848797
(Assignee)

Comment 1

5 years ago
Created attachment 738818 [details] [diff] [review]
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.
(Assignee)

Comment 2

5 years ago
Oops, I see a bug.  s,dir,path,
(Assignee)

Comment 3

5 years ago
Created attachment 738839 [details] [diff] [review]
windows clobber -- working!!
Attachment #738818 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 738843 [details] [diff] [review]
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.
Attachment #738839 - Attachment is obsolete: true
Attachment #738843 - Flags: review?(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)
Attachment #738843 - Flags: review?(jhopkins) → review-
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 739327 [details] [diff] [review]
review comments addressed, sans additional tests
Attachment #738843 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
(comment 6)
Flags: needinfo?(jhopkins)
Created attachment 740299 [details]
sample script to create very long path on Windows

Aki: I used the attached script for my own testing of long path removal.
Flags: needinfo?(jhopkins)
(Assignee)

Comment 10

5 years ago
Created attachment 740416 [details] [diff] [review]
with tests
Attachment #739327 - Attachment is obsolete: true
Attachment #740416 - Flags: review?(jhopkins)
Comment on attachment 740416 [details] [diff] [review]
with tests

Looks good!
Attachment #740416 - Flags: review?(jhopkins) → review+
(Assignee)

Comment 13

5 years ago
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.

:'(
Attachment #740416 - Flags: checked-in+ → checked-in-
(Assignee)

Comment 14

5 years ago
The new win32 test slaves (iX) will have pywin32 installed on py27.
I can land this then.
Depends on: 770578, 770579
Whiteboard: [mozharness] → [mozharness][pending new win32 test slaves]
(Assignee)

Comment 15

5 years ago
As :catlee noted, I can also do a try import/except, and fall back to rmdir /s /q.
(Assignee)

Comment 16

5 years ago
Created attachment 740594 [details] [diff] [review]
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).
Attachment #740594 - Flags: review?(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)
Attachment #740594 - Flags: review?(jhopkins) → review+
(Assignee)

Comment 18

5 years ago
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
Attachment #740594 - Flags: checked-in+
(Assignee)

Comment 19

5 years ago
Merged m-c to Cedar to verify before merging to production.
(Assignee)

Comment 20

5 years ago
In mozharness production.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 867029
No longer depends on: 867029
Product: mozilla.org → Release Engineering

Updated

4 years ago
Component: General Automation → Mozharness
You need to log in before you can comment on or make changes to this bug.