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

[llvm] Include LLVM_REPOSITORY and LLVM_REVISION in tool version #84990

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

JDevlieghere
Copy link
Member

Include the LLVM_REPOSITORY and LLVM_REVISION in the version output of tools using cl::PrintVersionMessage() such as dwarfdump and dsymutil.

Before:

$ llvm-dwarfdump --version
LLVM (http://llvm.org/):
  LLVM version 19.0.0git
  Optimized build with assertions.

After:

$ llvm-dwarfdump --version
LLVM (http://llvm.org/):
  LLVM version 19.0.0git (git@github.com:llvm/llvm-project.git 8467457afc61d70e881c9817ace26356ef757733)
  Optimized build with assertions.

rdar://121526866

Include the LLVM_REPOSITORY and LLVM_REVISION in the version output of
tools using cl::PrintVersionMessage().
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 12, 2024

@llvm/pr-subscribers-llvm-support

Author: Jonas Devlieghere (JDevlieghere)

Changes

Include the LLVM_REPOSITORY and LLVM_REVISION in the version output of tools using cl::PrintVersionMessage() such as dwarfdump and dsymutil.

Before:

$ llvm-dwarfdump --version
LLVM (http://llvm.org/):
  LLVM version 19.0.0git
  Optimized build with assertions.

After:

$ llvm-dwarfdump --version
LLVM (http://llvm.org/):
  LLVM version 19.0.0git (git@<!-- -->github.com:llvm/llvm-project.git 8467457afc61d70e881c9817ace26356ef757733)
  Optimized build with assertions.

rdar://121526866


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

1 Files Affected:

  • (modified) llvm/lib/Support/CommandLine.cpp (+10-1)
diff --git a/llvm/lib/Support/CommandLine.cpp b/llvm/lib/Support/CommandLine.cpp
index c076ae8b843179..42dbc4de200303 100644
--- a/llvm/lib/Support/CommandLine.cpp
+++ b/llvm/lib/Support/CommandLine.cpp
@@ -39,6 +39,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VCSRevision.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstdlib>
@@ -2538,7 +2539,15 @@ class VersionPrinter {
 #else
     OS << "LLVM (http://llvm.org/):\n  ";
 #endif
-    OS << PACKAGE_NAME << " version " << PACKAGE_VERSION << "\n  ";
+    OS << PACKAGE_NAME << " version " << PACKAGE_VERSION;
+#ifdef LLVM_REPOSITORY
+    OS << " (" << LLVM_REPOSITORY;
+#ifdef LLVM_REVISION
+    OS << ' ' << LLVM_REVISION;
+#endif
+    OS << ')';
+#endif
+    OS << "\n  ";
 #if LLVM_IS_DEBUG_BUILD
     OS << "DEBUG build";
 #else

Copy link
Collaborator

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

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

That will make triaging a lot easier, and produce better version strings for downstream users.

@JDevlieghere
Copy link
Member Author

The linux failure (DiagnosticTest.RespectsDiagnosticConfig) is unrelated to my change. It fails locally with and without my changes.

@JDevlieghere JDevlieghere merged commit 6885810 into llvm:main Mar 13, 2024
3 of 4 checks passed
@JDevlieghere JDevlieghere deleted the llvm-repository-revision branch March 13, 2024 22:33
JDevlieghere added a commit to apple/llvm-project that referenced this pull request Mar 13, 2024
…m#84990)

Include the `LLVM_REPOSITORY` and `LLVM_REVISION` in the version output
of tools using `cl::PrintVersionMessage()` such as dwarfdump and
dsymutil.

Before:

```
$ llvm-dwarfdump --version
LLVM (http://llvm.org/):
  LLVM version 19.0.0git
  Optimized build with assertions.
```

After:

```
$ llvm-dwarfdump --version
LLVM (http://llvm.org/):
  LLVM version 19.0.0git (git@github.com:llvm/llvm-project.git 8467457)
  Optimized build with assertions.
```

rdar://121526866
(cherry picked from commit 6885810)
@bogner
Copy link
Contributor

bogner commented Mar 20, 2024

Making Support depend on llvm_vcsrevision_h is a very bad idea. After this change a git commit basically causes a full rebuild, making incremental development and things like git bisect more or less completely unusable.

Try it out - run ninja llc, then git commit --allow-empty -m 'whoops rebuild everything', and then run ninja llc again. 2100 targets are out of date on my machine. Before this change (or after reverting it), the same rebuilds 4 targets.

We should probably revert this and if you want the feature in production compilers or something make it an explicit opt in.

@JDevlieghere
Copy link
Member Author

@bogner Good point, I didn't consider that. Let me see how I can work around that. Reverting this in the meantime is definitely the right thing to do.

JDevlieghere added a commit that referenced this pull request Mar 20, 2024
…ion" (#85879)

Reverts #84990 because this causes a full rebuild after
the commit hash changes.
@adrian-prantl
Copy link
Collaborator

Could you make the strings an external global defined in a separate library that Support just links against?

@rnk
Copy link
Collaborator

rnk commented Mar 20, 2024

I suppose this is a good time to remind folks that the LLVM_APPEND_VC_REV CMake option exists for faster developer builds. It's also worth pointing out that the unsupported gn and Bazel builds do not add the git revision by default, for a faster developer build experience.

I think it's not good enough to move this to another library that Support links against because llvm-tblgen will still depend on it. Going back deep into the past, Zachary Turner recommended that we make a more minimal support library, which is a curated subset of Support that exists juts for tablegen and any other C++ tools that run as part of the build. Support would just depend on this "SupportLite" library, so it would be transparent to most Support users.

@dwblaikie
Copy link
Collaborator

Sort of seems like it's talked around - would this LLVM_REPOSITORY/LLVM_REVISION inclusion being opt-in/off-by-default still address your needs, @JDevlieghere ?

@JDevlieghere
Copy link
Member Author

Possibly, but I actually don't really care about the LLVM_REPOSITORY/LLVM_REVISION all that much. We just happen to abuse that downstream to pass the clang version and including the repo/hash in the version output seemed like a nice improvement upstream as well. The real problem I was trying to address is that dwarfdump and dsymutil currently print:

$ dwarfdump --version
Apple LLVM version 15.0.0

while previously they would print:

$ dwarfdump --version
Apple LLVM version 15.0.0 (clang-1500.3.9.4)

I traced the issue back to https://reviews.llvm.org/D126977 where @rnk removed LLVM_VERSION_INFO and actually predicted the mistake I made here of adding a dependency on llvm_vcsrevision_h. To solve my concrete problem, appending to PACKAGE_VERSION would be the right thing to do, but it's used in a bunch of places and I don't have the bandwidth to qualify that won't break anything. I ended up partially reverting Reid's patch internally so I can include the version number for just the tools.

chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
…ion" (llvm#85879)

Reverts llvm#84990 because this causes a full rebuild after
the commit hash changes.
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.

None yet

6 participants