Permalink
Browse files

drop support of log formats < 4

Log format 4 was introduced in January.
There's been plenty of time to migrate.
  • Loading branch information...
1 parent b776143 commit 91b986fe22f327df3ed69234f4472d10e5bd883f @evmar evmar committed Jun 18, 2012
Showing with 21 additions and 23 deletions.
  1. +15 −10 src/build_log.cc
  2. +6 −13 src/build_log_test.cc
View
@@ -39,6 +39,7 @@
namespace {
const char kFileSignature[] = "# ninja log v%d\n";
+const int kOldestSupportedVersion = 4;
const int kCurrentVersion = 5;
// 64bit MurmurHash2, by Austin Appleby
@@ -229,19 +230,23 @@ bool BuildLog::Load(const string& path, string* err) {
char* line_end = 0;
while (reader.ReadLine(&line_start, &line_end)) {
if (!log_version) {
- log_version = 1; // Assume by default.
- if (sscanf(line_start, kFileSignature, &log_version) > 0)
- continue;
+ sscanf(line_start, kFileSignature, &log_version);
+
+ if (log_version < kOldestSupportedVersion) {
+ *err = "unable to extract version from build log, perhaps due to "
+ "being too old; you must clobber your build output and rebuild";
+ return false;
@nico

nico Jun 19, 2012

Collaborator

Why not just silently rebuild everything?

@evmar

evmar Jun 20, 2012

Collaborator

Can we do that? I guess we can create fake entries that always have hash=0 or something?

@nico

nico Jun 20, 2012

Collaborator

I thought that the rebuild logic was "if there's no entry in the build log, always rebuild" -- in that case, doing an early return here would be enough. But that's not true, it's "if the command is known and it changed, rebuild. If it's not known, don't rebuild".

So nevermind.

@evmar

evmar Jun 20, 2012

Collaborator

I've long suspected the latter logic might more reasonable logic to use anyway. What do you think?

@nico

nico Jun 20, 2012

Collaborator

What was the motivation for the current logic?

@evmar

evmar Jun 20, 2012

Collaborator

...I can't remember! I guess the fear that if you lose or corrupt your build log you effectively lose all your build state, but I guess that's only as likely as losing any other file.

@nico

nico Jun 21, 2012

Collaborator

With the current design, this happens:

time ~/src/ninja/ninja -C out/Release/ chrome   # Build
mv out/Release/.ninja_log .  # Move log away
GYP_DEFINES=foo bulid/gyp_chromium  # Change all build flags
time ~/src/ninja/ninja -C out/Release/ chrome

ninja decides to rebuild nothing, even though all command lines have changed (I just tried this). Conservatively assuming that commands not in the build log have changed sounds better to me than the current behavior – but I've never seen a problem with the current approach in practice either, so it's not very important either way. (It would make the code in this diff simpler though.)

+ }
}
// If no newline was found in this chunk, read the next.
if (!line_end)
continue;
- char field_separator = log_version >= 4 ? '\t' : ' ';
+ const char kFieldSeparator = '\t';
char* start = line_start;
- char* end = (char*)memchr(start, field_separator, line_end - start);
+ char* end = (char*)memchr(start, kFieldSeparator, line_end - start);
if (!end)
continue;
*end = 0;
@@ -252,21 +257,21 @@ bool BuildLog::Load(const string& path, string* err) {
start_time = atoi(start);
start = end + 1;
- end = (char*)memchr(start, field_separator, line_end - start);
+ end = (char*)memchr(start, kFieldSeparator, line_end - start);
if (!end)
continue;
*end = 0;
end_time = atoi(start);
start = end + 1;
- end = (char*)memchr(start, field_separator, line_end - start);
+ end = (char*)memchr(start, kFieldSeparator, line_end - start);
if (!end)
continue;
*end = 0;
restat_mtime = atol(start);
start = end + 1;
- end = (char*)memchr(start, field_separator, line_end - start);
+ end = (char*)memchr(start, kFieldSeparator, line_end - start);
if (!end)
continue;
string output = string(start, end - start);
@@ -293,10 +298,10 @@ bool BuildLog::Load(const string& path, string* err) {
char c = *end; *end = '\0';
entry->command_hash = (uint64_t)strtoull(start, NULL, 10);
*end = c;
- }
- else
+ } else {
entry->command_hash = LogEntry::HashCommand(StringPiece(start,
end - start));
+ }
}
fclose(file);
View
@@ -98,9 +98,9 @@ TEST_F(BuildLogTest, FirstWriteAddsSignature) {
TEST_F(BuildLogTest, DoubleEntry) {
FILE* f = fopen(kTestFilename, "wb");
- fprintf(f, "# ninja log v3\n");
- fprintf(f, "0 1 2 out command abc\n");
- fprintf(f, "3 4 5 out command def\n");
+ fprintf(f, "# ninja log v4\n");
+ fprintf(f, "0\t1\t2\tout\tcommand abc\n");
+ fprintf(f, "3\t4\t5\tout\tcommand def\n");
fclose(f);
string err;
@@ -148,23 +148,16 @@ TEST_F(BuildLogTest, Truncate) {
}
}
-TEST_F(BuildLogTest, UpgradeV3) {
+TEST_F(BuildLogTest, ObsoleteOldVersion) {
FILE* f = fopen(kTestFilename, "wb");
fprintf(f, "# ninja log v3\n");
fprintf(f, "123 456 0 out command\n");
fclose(f);
string err;
BuildLog log;
- EXPECT_TRUE(log.Load(kTestFilename, &err));
- ASSERT_EQ("", err);
-
- BuildLog::LogEntry* e = log.LookupByOutput("out");
- ASSERT_TRUE(e);
- ASSERT_EQ(123, e->start_time);
- ASSERT_EQ(456, e->end_time);
- ASSERT_EQ(0, e->restat_mtime);
- ASSERT_NO_FATAL_FAILURE(AssertHash("command", e->command_hash));
+ EXPECT_FALSE(log.Load(kTestFilename, &err));
+ ASSERT_NE(err.find("version"), string::npos);
}
TEST_F(BuildLogTest, SpacesInOutputV4) {

0 comments on commit 91b986f

Please sign in to comment.