Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ELF] Reinstate the former spelling in the version message #97942

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Jul 7, 2024

With LLVM_APPEND_VC_REV=off, the new version message after #97323
looks like:

% /tmp/out/custom2/bin/ld.lld --version
LLD 19.0.0, compatible with GNU linkers

A trailing comma after the version string might cause issues with
version detection tools that don't strip it, as seen in the Linux
kernel's scripts/ld-version.sh script.

Created using spr 1.3.5-bogner
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 7, 2024

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Author: Fangrui Song (MaskRay)

Changes

With LLVM_APPEND_VC_REV=off, the new version message after #97323
looks like:

% /tmp/out/custom2/bin/ld.lld --version
LLD 19.0.0, compatible with GNU linkers

A trailing comma after the version string might cause issues with
version detection tools that don't strip it, as seen in the Linux
kernel's scripts/ld-version.sh script.


Full diff: https://github.com/llvm/llvm-project/pull/97942.diff

2 Files Affected:

  • (modified) lld/ELF/Driver.cpp (+1-1)
  • (modified) lld/test/ELF/version.test (+1-1)
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 7800c2919a2bd..a4863d6717efb 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -631,7 +631,7 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   // of Libtool. We cannot convince every software developer to migrate to
   // the latest version and re-generate scripts. So we have this hack.
   if (args.hasArg(OPT_v) || args.hasArg(OPT_version))
-    message(getLLDVersion() + ", compatible with GNU linkers");
+    message(getLLDVersion() + " (compatible with GNU linkers)");
 
   if (const char *path = getReproduceOption(args)) {
     // Note that --reproduce is a debug option so you can ignore it
diff --git a/lld/test/ELF/version.test b/lld/test/ELF/version.test
index 383c1ac976d96..72bd2ab56a126 100644
--- a/lld/test/ELF/version.test
+++ b/lld/test/ELF/version.test
@@ -7,4 +7,4 @@
 # RUN: ld.lld -V 2>&1 | FileCheck %s
 # RUN: not ld.lld -V %t/not-exist 2>&1 | FileCheck %s
 
-# CHECK: LLD {{.*}}, compatible with GNU linkers
+# CHECK: LLD {{.+}} (compatible with GNU linkers)

Copy link
Member

@nathanchance nathanchance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, appreciate this! I will still send a v2 of the change to make scripts/ld-version.sh more robust, now that we know it can be :) but it won’t be urgent now, so thank you!

@yugier
Copy link
Contributor

yugier commented Jul 7, 2024

Thank you for making this change!

@MaskRay MaskRay merged commit 649cdfc into main Jul 7, 2024
10 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/elf-reinstate-the-former-spelling-in-the-version-message branch July 7, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants