[PATCH] D92902: [llvm-elfabi] Add flag to keep timestamp when output is the same

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: gbreynoo.
jhenderson added a comment.

The test is failing on the pre-merge bots. Please fix.

Not in this change, but this tool has enough going on now that it should have its own CommandGuide page under llvm/docs like other tools.



================
Comment at: llvm/test/tools/llvm-elfabi/keep-timestamp.test:1
+# RUN: llvm-elfabi --elf %p/Inputs/gnu_hash.so --emit-tbe=%t --keep-timestamp
+# RUN: touch -m -d "1970-01-01 00:00:00" %t
----------------
grimar wrote:
> You don't need `--keep-timestamp` for the first call, I believe?
A top-level test comment would be useful here.

There have historically been some issues with getting timestamps checking correct in other tools. I'd take a look at things like the --preserve-dates option in llvm-objcopy as well as testing of deterministic/non-deterministic archive timestamp options of llvm-ar and llvm-objcopy. @gbreynoo may have a bit more insight here as he's worked heavily on llvm-ar testing in the past, and I think ran into some of this before.


================
Comment at: llvm/test/tools/llvm-elfabi/keep-timestamp.test:4
+# RUN: llvm-elfabi --elf %p/Inputs/gnu_hash.so --emit-tbe=%t --keep-timestamp
+# RUN: stat %t | FileCheck %s
+
----------------
Maybe `ls -l` instead? That seems to be what's used in other places. I don't see any usage of `stat` anywhere else.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:62
+    "keep-timestamp", cl::desc("Keep the timestamp of the output file "
+                               "unchanged if the content is not changed"));
 
----------------
grimar wrote:
> `keep-timestamp` probably sounds like it will always keep the timestamp.
> I think the name can be better.
> 
> Perhaps `write-unchanged`/`update-unchanged` or something else.
llvm-objcopy uses `-p`/`--preserve-dates`. How about using that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92902