Bug 202477 - [ews] wincairo queue should use del instead of rm command
Summary: [ews] wincairo queue should use del instead of rm command
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-02 09:58 PDT by Aakash Jain
Modified: 2019-10-04 12:28 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.73 KB, patch)
2019-10-02 10:17 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff
Patch (2.50 KB, patch)
2019-10-03 14:49 PDT, Aakash Jain
jbedard: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2019-10-02 09:58:58 PDT
Windows doesn't have 'rm' command. So, windows or wincairo queue should use 'del' instead of 'rm' command in the CleanUpGitIndexLock build-step.
Comment 1 Aakash Jain 2019-10-02 10:02:44 PDT
wincairo-ews-002 bot got in an infinite loop when I cancelled a build https://ews-build.webkit.org/#/builders/12/builds/5786 because of .git/index.lock file, and failure to run the 'rm' command.

Subsequent builds on this bot after #5786:
https://ews-build.webkit.org/#/builders/12/builds/5794
https://ews-build.webkit.org/#/builders/12/builds/5795
https://ews-build.webkit.org/#/builders/12/builds/5796
Comment 2 Aakash Jain 2019-10-02 10:17:07 PDT
Created attachment 380032 [details]
Patch
Comment 3 Jonathan Bedard 2019-10-02 10:39:53 PDT
Comment on attachment 380032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380032&action=review

> Tools/BuildSlaveSupport/ews-build/steps_unittest.py:621
> +        self.setProperty('platform', 'wincairo')

What is our property in test_success by default?
Comment 4 Aakash Jain 2019-10-02 10:43:01 PDT
Comment on attachment 380032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380032&action=review

>> Tools/BuildSlaveSupport/ews-build/steps_unittest.py:621
>> +        self.setProperty('platform', 'wincairo')
> 
> What is our property in test_success by default?

platform is not set in test_success, so it will be on the steps.py code to handle it, which uses '*' as default platform (see line 126 of steps.py in this patch).
Comment 5 WebKit Commit Bot 2019-10-02 11:29:18 PDT
Comment on attachment 380032 [details]
Patch

Clearing flags on attachment: 380032

Committed r250616: <https://trac.webkit.org/changeset/250616>
Comment 6 WebKit Commit Bot 2019-10-02 11:29:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2019-10-02 11:30:19 PDT
<rdar://problem/55917272>
Comment 8 Dean Johnson 2019-10-02 11:34:05 PDT
Comment on attachment 380032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380032&action=review

> Tools/BuildSlaveSupport/ews-build/steps.py:128
> +        if platform in ('win', 'wincairo'):

Nit: I think this can be simplified by passing a tuple to platform.startswith
```py
if platform.startswith(('win', 'wincairo')):
```
Comment 9 Don Olmstead 2019-10-02 11:36:33 PDT
Comment on attachment 380032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380032&action=review

Should the commands be abstracted at all with a windows shell vs a cygwin/nix shell?

>> Tools/BuildSlaveSupport/ews-build/steps.py:128
>> +        if platform in ('win', 'wincairo'):
> 
> Nit: I think this can be simplified by passing a tuple to platform.startswith
> ```py
> if platform.startswith(('win', 'wincairo')):
> ```

Isn't AppleWin using cygwin? Also I'm not sure what Brent has in store for FTW until it potentially becomes the Windows port.
Comment 10 Aakash Jain 2019-10-02 15:35:14 PDT
(In reply to Don Olmstead from comment #9)
> Isn't AppleWin using cygwin? 
You are right. I will create a follow up patch.
Comment 11 Aakash Jain 2019-10-03 14:49:16 PDT
(In reply to Dean Johnson from comment #8)
> Nit: I think this can be simplified by passing a tuple to platform.startswith
> if platform.startswith(('win', 'wincairo')):
Agree. However, as we are not using 'win' anymore, I am going to change it to simply
platform == 'wincairo'
Comment 12 Aakash Jain 2019-10-03 14:49:29 PDT
Reopening to attach new patch.
Comment 13 Aakash Jain 2019-10-03 14:49:31 PDT
Created attachment 380162 [details]
Patch
Comment 14 Aakash Jain 2019-10-04 12:28:10 PDT
Committed r250736: <https://trac.webkit.org/changeset/250736>