--version returns wrong information

[GoogleCodeExporter]

$ include-what-you-use --version
Debian clang version 3.3-8 (branches/release_33) (based on LLVM 3.3)
Target: x86_64-pc-linux-gnu
Thread model: posix
error: unable to handle compilation, expected exactly one compiler job in ''

It should be about include-what-you-use, not clang!

Original issue reported on code.google.com by sylvestre.ledru on 30 Aug 2013 at 7:35

[GoogleCodeExporter]

I'd love to have this fixed, but I don't think we should prioritize it over 
functional problems.

Do you need this for infrastructure reasons (e.g. automated packaging, etc) or 
is it a cosmetic problem?

Original comment by kim.gras...@gmail.com on 30 Aug 2013 at 12:16

[GoogleCodeExporter]

I am using the output of include-what-you-use --version to generate the 
manpage. However, I have overridden the output. It is really minor...

Original comment by sylvestre.ledru on 2 Sep 2013 at 7:35

[GoogleCodeExporter]

OK, thanks for the detail. I understand the necessity better now.

I'll leave it be for now, but it'd be great if we could get this fixed 
longer-term.

Original comment by kim.gras...@gmail.com on 2 Sep 2013 at 6:41

[GoogleCodeExporter]

Here's a patch that picks up the SVN rev in CMake, and passes it on to the IWYU 
build.

Sylvestre, is this good enough version information for you?

Volodymyr, are you happy with the approach?

Original comment by kim.gras...@gmail.com on 19 Jan 2014 at 8:44

Attachments:

• version.patch

[GoogleCodeExporter]

Hmmm, will it work when not built from the svn tree?

Original comment by sylvestre.ledru on 23 Jan 2014 at 12:13

[GoogleCodeExporter]

It should work for out-of-tree builds, as in:
https://code.google.com/p/include-what-you-use/wiki/InstructionsForUsers#Buildin
g_out-of-tree

(I'll have to test this.)

But if IWYU doesn't have its svn working copy it won't pick up the svn rev., is 
that what you meant?

It's also only implemented for the CMake build, but I have a vague idea that 
should work for the Make workflow as well.

Original comment by kim.gras...@gmail.com on 23 Jan 2014 at 5:01

[GoogleCodeExporter]

To my mind there is no need to provide short option -V, it is confusing. And 
there is a little problem that we don't handle -v. By the way, we don't handle 
-help too, only --help. I think it's a nice opportunity to evaluate if llvm::cl 
is suitable for parsing IWYU options.

As you've already mentioned, Makefile support is required too.

I offer version string to be
  include-what-you-use 0.2 (520) based on clang 3.5 (199888)

I offer to move (r520) earlier, so it is clear that it corresponds to IWYU, not 
to clang. Use revision number without 'r' for consistency with clang itself. 
Use 'based on' instead of 'built on' because Apple's clang uses 'based on', for 
example, "Apple LLVM version 5.0 (clang-500.2.79) (based on LLVM 3.3svn)". I 
don't insist on specifying clang revision number, but clang revision number in 
bug reports will be helpful. Use own IWYU versioning because it looks weird 
that IWYU doesn't have explicit versioning: either the same as LLVM+Clang or 
separate. And separate versioning allows to release IWYU separately from 
LLVM+Clang which is useful for bugfix releases.

Original comment by vsap...@gmail.com on 24 Jan 2014 at 12:00

[GoogleCodeExporter]

Thanks!

As far as I can see, we do handle -v, it's the short alias for --verbose. 
Python uses -V for version, so I figured it was a better short alias/option 
value than "e".

I looked into llvm::cl a while back, but the whole Opt framework didn't really 
agree with me. Depending on which LLVM/Clang libraries are linked, options 
magically appear from nowhere, so we'd have to do filtering of the active 
options. There's also the convention with tool/clang options being separated by 
"--" instead of using a "-X" prefix, which messes up IWYU's ability to run as 
part of a make build. I discussed this at length on the Clang list, but we 
never came to any clear conclusion. I think I promised a patch, but I haven't 
gotten around to it.

I was reluctant to explicit IWYU versioning because we're more or less forced 
to follow Clang releases pretty closely anyway. But it makes more sense, 
especially to start at < 1.0. And you make a good point that we can release 
more often.

The version string you propose looks fine, I'll go back and see what is doable.

Still curious about Sylvestre's "Hmmm, will it work when not built from the svn 
tree?" question, could you explain more?

Original comment by kim.gras...@gmail.com on 25 Jan 2014 at 9:38

[GoogleCodeExporter]

I mean that "iwyu -v" prints Clang version and "iwyu -Xiwyu -V" isn't very 
useful (len("--version") == len("-Xiwyu -V" == 9). I've investigated that 
currently we cannot provide IWYU version for verbose output ("-v" flag) without 
Clang changes, so for now will print Clang version (see FIXME in 
Driver::PrintVersion).

I've looked into llvm::cl too and think it is unsuitable for our purposes. It 
doesn't handle Clang options, doesn't provide custom error handling, and 
doesn't support current implementation nicely (--help output, --long_option vs 
-long_option). So we'll use our own options' parsing.

What if we separate IWYU-specific options from intercepted options? So that 
"-Xiwyu --help", "-Xiwyu --version" are disallowed? I've attached a patch with 
such approach.

By the way, clang::getClangRevision() works with "make" but doesn't work with 
cmake+ninja.

Original comment by vsap...@gmail.com on 26 Jan 2014 at 3:08

Attachments:

• version2.patch

[GoogleCodeExporter]

Ah, I completely overlooked the -Xiwyu prefix in relation to -V.

Clang uses part of the llvm::cl library, just not the declarative framework, so 
I think we could do the same. But it takes some thinking on how to set up an 
option table, and how to have it interact with Clang's ditto. So for now I 
think our own options parsing is fine.

It's a little jarring that we put so much machinery in place for something as 
simple as options parsing :-/

I think your patch makes it clearer what's going on, but maybe we should 
consider catching "-h" and "-v" in main() just like "--help" and "--version" 
and wire them up as shortopts in ParseInterceptedCommandlineFlags?

Clang's Version.cpp seems broken under cmake/ninja, the revision number ends up 
returned from clang::getClangRepositoryPath(). I'll ask on cfe-dev.

Original comment by kim.gras...@gmail.com on 26 Jan 2014 at 4:15

[GoogleCodeExporter]

I don't want to intercept "-v" because in case of "clang -v some_file.cc" Clang 
prints invocation information, header search paths. To my mind it's useful and 
I'd like to keep this functionality despite non-printing IWYU version.

As for "-h" option I don't want to use it because Clang might start using it 
for some other purpose.

I have found how SVN revision is provided to Clang in Makefile build and have 
done the same for IWYU. Also I've renamed IWYU_SVN_REV to IWYU_SVN_REVISION to 
be more consistent with Clang.

File iwyu_version.h is introduced to store IWYU version. It is very simple and 
straightforward mechanism. Feel free to suggest something better.

Also I've introduced class OptionsParser to keep code responsible for 'which 
options are intercepted' and 'how to handle intercepted options' together. What 
do you think about this approach?

Original comment by vsap...@gmail.com on 31 Jan 2014 at 4:42

Attachments:

• version3.patch

[GoogleCodeExporter]

Ah, good point about "-v", I didn't know it was dual-purposed. Now I also 
understand your FIXME comment on ParseInterceptedCommandlineFlags better.

Overall, I like this a lot, it gives IWYU a private versioning scheme, and SVN 
information is nicely included automatically.

I feel that introducing a class for options parsing is a little heavy-handed, 
but I see that it makes memory management easier. I think I would prefer it if 
it was a function that returned clang args to pass into CreateCompilerInstance, 
something like:

std::vector<char*> ParseIwyuCommandline(int argc, char** argv) {
  // Split out intercepted args (--help, --version)
  // Split out IWYU args (-Xiwyu ...)
  // Push all others onto vector clang_args

  // Handle intercepted args
  // Handle IWYU args

  return clang_args;
}

But it's not super-important for this change, I can send a refactoring patch 
later if it bothers me too much :-)

iwyu.cc:
  // This code has various memory leaks, but we're in main, so who cares?

This comment in main() now seems outdated.

iwyu_globals.cc:
+static void PrintVersion() {
+  // Build version string.
+  string buffer;
+  llvm::raw_string_ostream ostream(buffer);
+  ostream << "include-what-you-use " << IWYU_VERSION_STRING;
+#ifdef IWYU_SVN_REVISION
+  string iwyu_svn_revision = IWYU_SVN_REVISION;
+#else
+  string iwyu_svn_revision = "";
+#endif
+  if (!iwyu_svn_revision.empty()) {
+    ostream << " (" << iwyu_svn_revision << ")";
+  }
+  ostream << " based on clang " << CLANG_VERSION_STRING;
+  string clang_svn_revision = clang::getClangRevision();
+  if (!clang_svn_revision.empty()) {
+    ostream << " (" << clang::getClangRevision() << ")";
+  }
+  
+  // Print version string.
+  llvm::outs() << ostream.str() << "\n";
+}

Is there any reason to buffer the output? Also, it turns out that Clang has a 
library function that gives you "clang version X.Y (nnnnnnn)": 
getClangFullVersion(). I propose:

  static void PrintVersion() {
    // Print version string.
    llvm::outs() << "include-what-you-use " << IWYU_VERSION_STRING;

  #ifdef IWYU_SVN_REVISION
    llvm::outs() << " (" << IWYU_SVN_REVISION << ")";
  #endif

    llvm::outs() << " based on " << clang::getClangFullVersion();
    llvm::outs() << "\n";
  }

+  iwyu_argv[iwyu_argc] = NULL;    // argv should be NULL-terminated
+  intercepted_argv[intercepted_argc] = NULL;
+  clang_argv_[clang_argc_] = NULL;

The trailing comment now applies to all three lines, so move it to its own line.

Makefile:
+        
+CPP.Defines += -DIWYU_SVN_REVISION='"$(IWYU_SVN_REVISION)"'

Line before CPP.Defines has trailing whitespace.

I don't know Make syntax, so I assume the single-quotes around the 
dobule-quotes are necessary. It looks a little over-quoted, but I'm guessing 
there's a reason.

Original comment by kim.gras...@gmail.com on 9 Feb 2014 at 3:23

[GoogleCodeExporter]

Oh, and I forgot: --version should be listed in PrintHelp.

Original comment by kim.gras...@gmail.com on 9 Feb 2014 at 3:28

[GoogleCodeExporter]

Kim, thanks for the feedback. All your comments make sense, will update the 
patch accordingly.

Original comment by vsap...@gmail.com on 10 Feb 2014 at 8:53

[GoogleCodeExporter]

A new patch is attached.

Regarding a class for options parsing, I've also looked into how to add support 
for compilation database. Currently I am inclined to use our own options 
parsing and ClangTool, which requires compilation database and source paths. In 
this case a class is more suitable than a function, I think.

There was still 1 memory leak in main(). I've fixed it and removed the comment 
about memory leaks.

> [...] PrintVersion [...] Is there any reason to buffer the output?

No reason, at some point I had locally GetVersion and PrintVersion. Buffering 
the version string is just remains of GetVersion. getClangFullVersion is good 
idea. I've left a check "if (!iwyu_svn_revision.empty())" because when there is 
no SVN info, IWYU_SVN_REVISION is defined, but equals to "".

> I don't know Make syntax, so I assume the single-quotes around the 
double-quotes are necessary.

Single quotes are used so that make doesn't consume double quotes. This way 
double quotes are present in the source code when macro is expanded.

Also I've tuned help/usage message and a few comments.

Original comment by vsap...@gmail.com on 19 Feb 2014 at 2:20

Attachments:

• version4.patch

[GoogleCodeExporter]

+#ifdef IWYU_SVN_REVISION
+  string iwyu_svn_revision = IWYU_SVN_REVISION;
+#else
+  string iwyu_svn_revision = "";
+#endif

I would prefer if the build failed if IWYU_SVN_REVISION wasn't defined, so you 
can just skip the #ifdef. Now that both the CMake and Make systems 
unconditionally defines it (maybe to an empty string, but it's still defined), 
I see no reason to allow building without it.

+#define IWYU_VERSION_STRING "0.2"

Please add include guards in iwyu_version.h

Other than that, this looks great!

Original comment by kim.gras...@gmail.com on 22 Feb 2014 at 9:46

[GoogleCodeExporter]

Committed r526 with changes according to review. Thanks, Kim.

Sylvestre, can you please confirm that the new version string is suitable for 
your purposes?

Original comment by vsap...@gmail.com on 23 Feb 2014 at 12:38

• Changed state: Fixed

[GoogleCodeExporter]

Oops, I'm now hitting an assert in llvm/include/llvm/ADT/SmallVector.h line 144 
with this patch applied on Windows.

When I revert back to r525 everything runs fine again. I'll try and debug 
through it to find the root cause.

Original comment by kim.gras...@gmail.com on 23 Feb 2014 at 9:51

[GoogleCodeExporter]

What is the current display? (I cannot build iwyu right now)
Thanks

Original comment by sylvestre.ledru on 23 Feb 2014 at 10:38

[GoogleCodeExporter]

False alarm! After cleaning and rebuilding, I don't see this anymore. 
Disconcerting, but unrelated to this patch. Sorry about the noise.

Original comment by kim.gras...@gmail.com on 23 Feb 2014 at 10:50

[GoogleCodeExporter]

@sylvestre:

$ include-what-you-use.exe --version
include-what-you-use 0.2 (527) based on clang version 3.5 (201969)

We've introduced a proper version number for IWYU to better reflect its 
maturity.

Original comment by kim.gras...@gmail.com on 23 Feb 2014 at 10:51

[GoogleCodeExporter]

Sounds perfect! Thanks!

Original comment by sylvestre.ledru on 23 Feb 2014 at 10:52