Page MenuHomePhabricator

[llvm-objcopy] Consolidate and unify version tests
ClosedPublic

Authored by alexshap on Sun, Sep 6, 2:26 PM.

Details

Summary

In this diff the tests which verify version printing functionality are refactored.
Since they are not specific to a particular format we move them into tool-version.test
and slightly unify (similarly to tool-name.test and tool-help-message.test).

Test plan:

make check-all

Diff Detail

Event Timeline

alexshap created this revision.Sun, Sep 6, 2:26 PM
Herald added a project: Restricted Project. · View Herald Transcript
alexshap requested review of this revision.Sun, Sep 6, 2:26 PM
jhenderson requested changes to this revision.Mon, Sep 7, 1:08 AM
jhenderson added inline comments.
llvm/test/tools/llvm-objcopy/ELF/strip-version.test
5

I think this is an important aspect of this test - see https://github.com/llvm/llvm-project/commit/e9af7158200643dd3ff0feb062fecd564345366c. It might make sense to add a similar check for objcopy. I kind of hinted at that in the original review, but it never got acted on.

llvm/test/tools/llvm-objcopy/tool-version.test
9

Presumably copy-paste error?

This revision now requires changes to proceed.Mon, Sep 7, 1:08 AM

To be clear, +1 to this patch. Just needs to not lose coverage in the process.

alexshap updated this revision to Diff 290205.Mon, Sep 7, 1:38 AM

Bring back the check for "GNU"

jhenderson accepted this revision.Mon, Sep 7, 1:39 AM

LGTM. It looks weird to me that llvm-install-name-tool doesn't have a -V option, but I don't think that's a relevant issue here.

This revision is now accepted and ready to land.Mon, Sep 7, 1:39 AM
alexshap added inline comments.Mon, Sep 7, 1:44 AM
llvm/test/tools/llvm-objcopy/tool-version.test
9

in fact, at the moment there is no alias "-V" for "--version" in InstallNameToolOpts.td,
i do not recall any discussions around it, if i am not mistaken nobody has ever asked for it (cctools' install_name_tool doesn't have these flags that's why we have not run into any issues), but in general we can add this alias.

This revision was automatically updated to reflect the committed changes.