This is an archive of the discontinued LLVM Phabricator instance.

Differential D87211

[llvm-objcopy] Consolidate and unify version tests
ClosedPublic

Authored by alexander-shaposhnikov on Sep 6 2020, 2:26 PM.

Details

Reviewers
jhenderson
MaskRay
smeenai
• espindola
rupprecht
Commits
rG10af5bad443d: [llvm-objcopy] Consolidate and unify version tests
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

alexander-shaposhnikov created this revision.Sep 6 2020, 2:26 PM
Herald added a reviewer: • espindola. · View Herald TranscriptSep 6 2020, 2:26 PM
Herald added a reviewer: rupprecht. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: abrachet, emaste. · View Herald Transcript
alexander-shaposhnikov requested review of this revision.Sep 6 2020, 2:26 PM
Harbormaster completed remote builds in B70771: Diff 290151.Sep 6 2020, 2:55 PM
jhenderson requested changes to this revision.Sep 7 2020, 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.Sep 7 2020, 1:08 AM
jhenderson added a comment.Sep 7 2020, 1:09 AM

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

alexander-shaposhnikov updated this revision to Diff 290205.Sep 7 2020, 1:38 AM

Bring back the check for "GNU"

jhenderson accepted this revision.Sep 7 2020, 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.Sep 7 2020, 1:39 AM
alexander-shaposhnikov added inline comments.Sep 7 2020, 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.

Harbormaster completed remote builds in B70799: Diff 290205.Sep 7 2020, 2:08 AM
Closed by commit rG10af5bad443d: [llvm-objcopy] Consolidate and unify version tests (authored by alexander-shaposhnikov). · Explain WhySep 7 2020, 6:44 PM
This revision was automatically updated to reflect the committed changes.
alexander-shaposhnikov added a commit: rG10af5bad443d: [llvm-objcopy] Consolidate and unify version tests.

Revision Contents

Diff 290388

llvm/test/tools/llvm-objcopy/ELF/objcopy-version.test

  • This file was deleted.
# RUN: llvm-objcopy --version | FileCheck %s
# RUN: llvm-objcopy -V | FileCheck %s
# CHECK: {{ version }}

llvm/test/tools/llvm-objcopy/ELF/strip-version.test

  • This file was deleted.
# RUN: llvm-strip --version | FileCheck %s
# RUN: llvm-strip -V | FileCheck %s
# CHECK-DAG: {{ version }}
# CHECK-DAG: GNU strip
jhendersonUnsubmitted
Not Done
ReplyInline Actions

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.

jhenderson: I think this is an important aspect of this test - see https://github.com/llvm/llvm…

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-version.test

  • This file was deleted.
# RUN: llvm-install-name-tool --version | FileCheck %s
# CHECK: {{ version }}

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

  • This file was added.
# RUN: llvm-objcopy --version | FileCheck --check-prefix=OBJCOPY %s
# RUN: llvm-objcopy -V | FileCheck --check-prefix=OBJCOPY %s
# RUN: llvm-strip --version | FileCheck --check-prefix=STRIP %s
# RUN: llvm-strip -V | FileCheck --check-prefix=STRIP %s
# RUN: llvm-install-name-tool --version | FileCheck %s
# OBJCOPY-DAG: {{ version }}
jhendersonUnsubmitted
Not Done
ReplyInline Actions
# RUN: llvm-install-name-tool --version | FileCheck %s
- # RUN: llvm-strip -V | FileCheck %s
+ # RUN: llvm-install-name-tool -V | FileCheck %s
# CHECK: {{ version }}

Presumably copy-paste error?

jhenderson: Presumably copy-paste error?
alexander-shaposhnikovAuthorUnsubmitted
Done
ReplyInline Actions

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.

alexander-shaposhnikov: in fact, at the moment there is no alias "-V" for "--version" in InstallNameToolOpts.td, i do…
# OBJCOPY-DAG: GNU objcopy
# STRIP-DAG: {{ version }}
# STRIP-DAG: GNU strip
# CHECK: {{ version }}
llvm/test/tools/llvm-objcopy/ELF/objcopy-version.test