Bug 198008

Summary: generate-xcfilelists is stranding temporary files
Product: WebKit Reporter: Keith Rollin <krollin>
Component: Tools / TestsAssignee: Keith Rollin <krollin>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, glenn, jbedard, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Keith Rollin 2019-05-17 16:46:47 PDT
generate-xcfilelists makes use of temporary files on disk. These files are opened with the OS's "temporary" bit set, causing them to get deleted when closed or the process exists. However, these temporary files actually end up persisting after the script exists. This is because `sed` is used to process the files, and is done so in a way that causes the "temporary" bit to get cleared.

Address this issue by no longer using `sed` and instead performing the equivalent processing the file content in-memory.

rdar://problem/50893659
Comment 1 Keith Rollin 2019-05-17 16:49:20 PDT
Created attachment 370169 [details]
Patch
Comment 2 EWS Watchlist 2019-05-17 16:51:58 PDT
Attachment 370169 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:368:  expected 1 blank line, found 0  [pep8/E301] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jonathan Bedard 2019-05-20 08:32:33 PDT
Comment on attachment 370169 [details]
Patch

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

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:346
> +        return set([re.sub(to_replace, replace_with, line) for line in lines])

Putting these in a set will throw out any order you have, not sure if that's an OK side-effect here....it might not have correctness problems, but I could see it making the xcfilelists less readable.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:368
> +        def get_lines(source):

The style checker is complaining about no newlines surrounding this. Given how you're using it, I wonder if this:

get_lines = lambda source: source if isinstance(source, set) else set(source) if isinstance(source, list) else self._get_file_lines(source)

would be more readable (and make the style checker happy)
Comment 4 Jonathan Bedard 2019-05-20 08:35:58 PDT
(In reply to Jonathan Bedard from comment #3)
> Comment on attachment 370169 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370169&action=review
> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:346
> > +        return set([re.sub(to_replace, replace_with, line) for line in lines])
> 
> Putting these in a set will throw out any order you have, not sure if that's
> an OK side-effect here....it might not have correctness problems, but I
> could see it making the xcfilelists less readable.

Actually, what we probably want to do is put everything in a set, then convert the set to a list, then sort the list.

> ...
Comment 5 Keith Rollin 2019-05-20 09:13:44 PDT
(In reply to Jonathan Bedard from comment #4)

> Actually, what we probably want to do is put everything in a set, then
> convert the set to a list, then sort the list.

That's exactly what I do. See _merge_added_lines.
Comment 6 Keith Rollin 2019-05-20 09:48:10 PDT
Created attachment 370257 [details]
Patch
Comment 7 Keith Rollin 2019-05-20 09:48:29 PDT
Added blank lines.
Comment 8 WebKit Commit Bot 2019-05-20 11:24:49 PDT
Comment on attachment 370257 [details]
Patch

Clearing flags on attachment: 370257

Committed r245531: <https://trac.webkit.org/changeset/245531>
Comment 9 WebKit Commit Bot 2019-05-20 11:24:50 PDT
All reviewed patches have been landed.  Closing bug.