[PATCH] D92902: [llvm-elfabi] Add flag to keep timestamp when output is the same
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 9 01:17:08 PST 2020
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
More information about the llvm-commits
mailing list