[PATCH] D159223: [llvm-windres] Implement the windres flag --use-temp-file

Fri Sep 1 08:26:41 PDT 2023

mstorsjo added inline comments.

================
Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:496-497
   }
+  // TODO: If --use-temp-file is set, we shouldn't be unescaping
+  // the --preprocessor argument either, only splitting it.
   if (InputArgs.hasArg(WINDRES_preprocessor))
----------------
alvinhochun wrote:
> I presume you plan to resolve this soon? Since you mentioned that GNU windres changed its behaviour in a backwards incompatible
> way do you have a reference to any upstream bug reports or commits regarding this?
I'm kinda unsure about what to do about it.

It broke, unintentionally, in this binutils commit: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=21c33bcbe36377abf01614fb1b9be439a3b6de20

There were bug reports about it filed here: https://github.com/msys2/MSYS2-packages/issues/2379 and here: https://sourceware.org/bugzilla/show_bug.cgi?id=27594

But there was no acknowledgement that this indeed broke the existing uses of the option according to the documentation, instead the documentation was updated to document how the option works now: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=5edb8e3f5ad8d74a83fc0df7f6e4514eed0aa77f;hp=3abbafc2aacc6706fea3e3e326e2f08d107c3672

Before this, llvm-windres was implemented mimicing GNU windres behaviour from this change - to make llvm-windres work with projects that were using it according to the old behaviour, e.g. in ffmpeg. Due to this breakage, it's hard for third party projects to sensibly use this option at all - so in ffmpeg I had to opt to change it to use another option instead: https://github.com/ffmpeg/ffmpeg/commit/f9626d1065c43f1d51afe66bdf988b9f33729440 Not sure how other projects that were affected dealt with it.

As 2 years has passed since, and I guess most projects that were using the option have had to adapt (i.e. stop using it? or switch to the new form and not work with llvm-windres), I guess we could/should update. I feel we didn't really get proper closure on those bug reports though - that why I haven't taken action on matching this behaviour yet.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159223