Bug 188424 – run-bindings-tests is not Win32-compatible

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ross Kirsling]

run-bindings-tests is not Win32-compatible

[Ross Kirsling]

Created attachment 346803 [details]
Patch

[Ross Kirsling]

mac-32bit failure here seems to be a fluke? This patch should not change behavior for other platforms.

[Fujii Hironori]

If tempfile.NamedTemporaryFile(mode='r', delete=Treu) is used in this case, will the temporary file be deleted automatically when GC collects the file object?

Or, what about the idea defining own mktemp like deprecated tempfile.mktemp?

def mktemp(self):
    fd, filename = tempfile.mkstemp()
    os.close(fd)
    return filename

[Ross Kirsling]

(In reply to Fujii Hironori from comment #3)
> If tempfile.NamedTemporaryFile(mode='r', delete=Treu) is used in this case,
> will the temporary file be deleted automatically when GC collects the file
> object?

I hadn't tried it, but it certainly seems to work! (Tried on Mac and Win alike.)

[Ross Kirsling]

Created attachment 346815 [details]
Patch

[Fujii Hironori]

Comment on attachment 346815 [details]
Patch

So weird. I can't understand why this code works.
NamedTemporaryFile() returns file object.
You pass the file objects to generate_supplemental_dependency, not filenames.
Why does this code work?

[Fujii Hironori]

You can get the filename by 'supplemental_dependency_file.name'.

[Ross Kirsling]

(In reply to Fujii Hironori from comment #6)
> Comment on attachment 346815 [details]
> Patch
> 
> So weird. I can't understand why this code works.
> NamedTemporaryFile() returns file object.
> You pass the file objects to generate_supplemental_dependency, not filenames.
> Why does this code work?

Apparently it works because "<open file '<fdopen>', mode 'w+b' at 0x10102ff60>" is a valid filename, hahaha.

(In reply to Fujii Hironori from comment #7)
> You can get the filename by 'supplemental_dependency_file.name'.

Thanks.

[Ross Kirsling]

Created attachment 346820 [details]
Patch

[Ross Kirsling]

(In reply to Ross Kirsling from comment #8)
> Apparently it works because "<open file '<fdopen>', mode 'w+b' at
> 0x10102ff60>" is a valid filename, hahaha.

...and because WebCore/bindings/scripts/preprocess-idls.pl will happily create the supplemental_dependency_file if it doesn't file already exists (it just won't clean up after itself), I mean.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #10)
> ...and because WebCore/bindings/scripts/preprocess-idls.pl will happily
> create the supplemental_dependency_file if it doesn't file already exists
> (it just won't clean up after itself), I mean.

Ugh, failure to proofread. "if the file doesn't already exist".

[WebKit Commit Bot]

Comment on attachment 346820 [details]
Patch

Clearing flags on attachment: 346820

Committed r234720: <https://trac.webkit.org/changeset/234720>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43080517>

[Fujii Hironori]

It failed on the BuildBot.

https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Release%20%28Tests%29/builds/954/steps/bindings-generation-tests/logs/stdio

> Couldn't open c:\users\containeradministrator\appdata\local\temp\tmp6paapw: Permission denied
> 
> Failed to generate a supplemental dependency file.

Do you need to open as read mode?
NamedTemporaryFile(mode='r')

[Ross Kirsling]

(In reply to Fujii Hironori from comment #15)
> It failed on the BuildBot.
> 
> https://build.webkit.org/builders/WinCairo%2064-
> bit%20WKL%20Release%20%28Tests%29/builds/954/steps/bindings-generation-tests/
> logs/stdio
> 
> > Couldn't open c:\users\containeradministrator\appdata\local\temp\tmp6paapw: Permission denied
> > 
> > Failed to generate a supplemental dependency file.
> 
> Do you need to open as read mode?
> NamedTemporaryFile(mode='r')

Shoot, I only tested the last patch on Mac.

We don't even read from the supplemental dependency file from Python, so the mode doesn't matter (though I made sure just now), but as the docs say:
> Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later).

So apparently we need delete=False, which means os.remove is unavoidable after all, but NamedTemporaryFile does still free us from needing a separate os.close call.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #16)
> (In reply to Fujii Hironori from comment #15)
> > It failed on the BuildBot.
> > 
> > https://build.webkit.org/builders/WinCairo%2064-
> > bit%20WKL%20Release%20%28Tests%29/builds/954/steps/bindings-generation-tests/
> > logs/stdio
> > 
> > > Couldn't open c:\users\containeradministrator\appdata\local\temp\tmp6paapw: Permission denied
> > > 
> > > Failed to generate a supplemental dependency file.
> > 
> > Do you need to open as read mode?
> > NamedTemporaryFile(mode='r')
> 
> Shoot, I only tested the last patch on Mac.
> 
> We don't even read from the supplemental dependency file from Python, so the
> mode doesn't matter (though I made sure just now), but as the docs say:
> > Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later).
> 
> So apparently we need delete=False, which means os.remove is unavoidable
> after all, but NamedTemporaryFile does still free us from needing a separate
> os.close call.

Oops, nope -- the fact that this works is almost an accident, because we're depending on Perl closing the file for us.

Regarding your earlier suggestion about mktemp, this function was deprecated for legitimate security reasons, so we really can't justify opting into that behavior:
https://docs.python.org/2/library/tempfile.html#tempfile.mktemp

As suggested by the blog post I linked in the ChangeLog (https://www.logilab.org/blogentry/17873), calling os.close before os.remove seems to be the "right thing to do" even on POSIX, so I will reintroduce my initial approach as a fix.

[Ross Kirsling]

Committed r234726: <https://trac.webkit.org/changeset/234726>