[PATCH] D81803: [Support] PR42623: Avoid setting the delete-on-close bit if a TempFile doesn't reside on a local drive

Tue Mar 9 01:45:18 PST 2021

jhenderson added a comment.

In D81803#2607667 <https://reviews.llvm.org/D81803#2607667>, @rdwampler wrote:

> In D81803#2606948 <https://reviews.llvm.org/D81803#2606948>, @sbc100 wrote:
>
>> In D81803#2592409 <https://reviews.llvm.org/D81803#2592409>, @nikic wrote:
>>
>>> Just FTR, this change had to be reverted in Rust's LLVM fork because it breaks compilation on Windows 7, see https://github.com/rust-lang/rust/issues/81051.
>>
>> Emscripten users also seem to be effect by this.  Specifically windows 7 users are seeing link failure: "wasm-ld: error: failed to write the output file: permission denied".
>>
>> https://github.com/emscripten-core/emscripten/issues/13067
>
> This change was to fix a very narrow use case. Since it affecting all Windows 7 users, I think we should revert it. I can investigate if there's another way to work around the original issue. But until then,  is there still time to get this revert in the 12.0.0 release @tstellar?

FWIW, we've got a downstream user who has run into an issue on our LLVM 10 based toolchain, which looks likely to benefit from this fix as-is (we're still waiting on confirmation, as we don't have access to a Mach-O machine needed to reproduce the issue they were seeing). Consequently, there's a soft oppose from me to reverting this without an alternative fix.

Perhaps there should be a wider discussion on Windows support - it's been over a year since Windows 7 was end of life, so we can't really expect LLVM developers or build bots to verify that changes work on such machines (since they'd have inherent security issues). If we continue to support Windows 7, we're likely to break things in future releases too, which won't be picked up until potentially quite late on. I can't see anywhere obvious on the LLVM website where the minimum supported Windows version is documented unfortunately (although it's possible I've missed it).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81803/new/

https://reviews.llvm.org/D81803