Skip to content
This repository was archived by the owner on Jun 15, 2023. It is now read-only.

Commit c475064

Browse files
sgrahamsgraham
committed
make all GetNode explicit, add DepsLog canonicalize test
1 parent fccce3f commit c475064

11 files changed

+135
-67
lines changed

‎src/build.cc

+5-1
Original file line numberDiff line numberDiff line change
@@ -825,7 +825,11 @@ bool Builder::ExtractDeps(CommandRunner::Result* result,
825825
result->output = parser.Parse(result->output, deps_prefix);
826826
for (set<string>::iterator i = parser.includes_.begin();
827827
i != parser.includes_.end(); ++i) {
828-
deps_nodes->push_back(state_->GetNode(*i));
828+
// ~0 is assuming that with MSVC-parsed headers, it's ok to always make
829+
// all backslashes (as some of the slashes will certainly be backslashes
830+
// anyway). This could be fixed if necessary with some additional
831+
// complexity in IncludesNormalize::Relativize.
832+
deps_nodes->push_back(state_->GetNode(*i, ~0u));
829833
}
830834
} else
831835
#endif

‎src/build_test.cc

+67-1
Original file line numberDiff line numberDiff line change
@@ -1883,7 +1883,7 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) {
18831883

18841884
Edge* edge = state.edges_.back();
18851885

1886-
state.GetNode("bar.h")->MarkDirty(); // Mark bar.h as missing.
1886+
state.GetNode("bar.h", 0)->MarkDirty(); // Mark bar.h as missing.
18871887
EXPECT_TRUE(builder.AddTarget("fo o.o", &err));
18881888
ASSERT_EQ("", err);
18891889

@@ -1901,6 +1901,72 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) {
19011901
}
19021902
}
19031903

1904+
#ifdef _WIN32
1905+
TEST_F(BuildWithDepsLogTest, DepFileDepsLogCanonicalize) {
1906+
string err;
1907+
const char* manifest =
1908+
"rule cc\n command = cc $in\n depfile = $out.d\n deps = gcc\n"
1909+
"build a/b\\c\\d/e/fo$ o.o: cc x\\y/z\\foo.c\n";
1910+
1911+
fs_.Create("x/y/z/foo.c", "");
1912+
1913+
{
1914+
State state;
1915+
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
1916+
1917+
// Run the build once, everything should be ok.
1918+
DepsLog deps_log;
1919+
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
1920+
ASSERT_EQ("", err);
1921+
1922+
Builder builder(&state, config_, NULL, &deps_log, &fs_);
1923+
builder.command_runner_.reset(&command_runner_);
1924+
EXPECT_TRUE(builder.AddTarget("a/b/c/d/e/fo o.o", &err));
1925+
ASSERT_EQ("", err);
1926+
// Note, different slashes from manifest.
1927+
fs_.Create("a/b\\c\\d/e/fo o.o.d",
1928+
"a\\b\\c\\d\\e\\fo\\ o.o: blah.h bar.h\n");
1929+
EXPECT_TRUE(builder.Build(&err));
1930+
EXPECT_EQ("", err);
1931+
1932+
deps_log.Close();
1933+
builder.command_runner_.release();
1934+
}
1935+
1936+
{
1937+
State state;
1938+
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
1939+
1940+
DepsLog deps_log;
1941+
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
1942+
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
1943+
ASSERT_EQ("", err);
1944+
1945+
Builder builder(&state, config_, NULL, &deps_log, &fs_);
1946+
builder.command_runner_.reset(&command_runner_);
1947+
1948+
Edge* edge = state.edges_.back();
1949+
1950+
state.GetNode("bar.h", 0)->MarkDirty(); // Mark bar.h as missing.
1951+
EXPECT_TRUE(builder.AddTarget("a/b/c/d/e/fo o.o", &err));
1952+
ASSERT_EQ("", err);
1953+
1954+
// Expect three new edges: one generating fo o.o, and two more from
1955+
// loading the depfile.
1956+
ASSERT_EQ(3u, state.edges_.size());
1957+
// Expect our edge to now have three inputs: foo.c and two headers.
1958+
ASSERT_EQ(3u, edge->inputs_.size());
1959+
1960+
// Expect the command line we generate to only use the original input.
1961+
// Note, slashes from manifest, not .d.
1962+
ASSERT_EQ("cc x\\y/z\\foo.c", edge->EvaluateCommand());
1963+
1964+
deps_log.Close();
1965+
builder.command_runner_.release();
1966+
}
1967+
}
1968+
#endif
1969+
19041970
/// Check that a restat rule doesn't clear an edge if the depfile is missing.
19051971
/// Follows from: https://github.com/martine/ninja/issues/603
19061972
TEST_F(BuildTest, RestatMissingDepfile) {

‎src/deps_log.cc

+8-2
Original file line numberDiff line numberDiff line change
@@ -233,14 +233,20 @@ bool DepsLog::Load(const string& path, State* state, string* err) {
233233
if (!UpdateDeps(out_id, deps))
234234
++unique_dep_record_count;
235235
} else {
236-
int path_size = size - 4;
236+
size_t path_size = size - 4;
237237
assert(path_size > 0); // CanonicalizePath() rejects empty paths.
238238
// There can be up to 3 bytes of padding.
239239
if (buf[path_size - 1] == '\0') --path_size;
240240
if (buf[path_size - 1] == '\0') --path_size;
241241
if (buf[path_size - 1] == '\0') --path_size;
242+
unsigned int slash_bits;
243+
string err;
244+
if (!CanonicalizePath(&buf[0], &path_size, &slash_bits, &err)) {
245+
read_failed = true;
246+
break;
247+
}
242248
StringPiece path(buf, path_size);
243-
Node* node = state->GetNode(path);
249+
Node* node = state->GetNode(path, slash_bits);
244250

245251
// Check that the expected index matches the actual index. This can only
246252
// happen if two ninja processes write to the same deps log concurrently.

‎src/deps_log_test.cc

+47-47
Original file line numberDiff line numberDiff line change
@@ -46,16 +46,16 @@ TEST_F(DepsLogTest, WriteRead) {
4646

4747
{
4848
vector<Node*> deps;
49-
deps.push_back(state1.GetNode("foo.h"));
50-
deps.push_back(state1.GetNode("bar.h"));
51-
log1.RecordDeps(state1.GetNode("out.o"), 1, deps);
49+
deps.push_back(state1.GetNode("foo.h", 0));
50+
deps.push_back(state1.GetNode("bar.h", 0));
51+
log1.RecordDeps(state1.GetNode("out.o", 0), 1, deps);
5252

5353
deps.clear();
54-
deps.push_back(state1.GetNode("foo.h"));
55-
deps.push_back(state1.GetNode("bar2.h"));
56-
log1.RecordDeps(state1.GetNode("out2.o"), 2, deps);
54+
deps.push_back(state1.GetNode("foo.h", 0));
55+
deps.push_back(state1.GetNode("bar2.h", 0));
56+
log1.RecordDeps(state1.GetNode("out2.o", 0), 2, deps);
5757

58-
DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o"));
58+
DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o", 0));
5959
ASSERT_TRUE(log_deps);
6060
ASSERT_EQ(1, log_deps->mtime);
6161
ASSERT_EQ(2, log_deps->node_count);
@@ -79,7 +79,7 @@ TEST_F(DepsLogTest, WriteRead) {
7979
}
8080

8181
// Spot-check the entries in log2.
82-
DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out2.o"));
82+
DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out2.o", 0));
8383
ASSERT_TRUE(log_deps);
8484
ASSERT_EQ(2, log_deps->mtime);
8585
ASSERT_EQ(2, log_deps->node_count);
@@ -101,11 +101,11 @@ TEST_F(DepsLogTest, LotsOfDeps) {
101101
for (int i = 0; i < kNumDeps; ++i) {
102102
char buf[32];
103103
sprintf(buf, "file%d.h", i);
104-
deps.push_back(state1.GetNode(buf));
104+
deps.push_back(state1.GetNode(buf, 0));
105105
}
106-
log1.RecordDeps(state1.GetNode("out.o"), 1, deps);
106+
log1.RecordDeps(state1.GetNode("out.o", 0), 1, deps);
107107

108-
DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o"));
108+
DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o", 0));
109109
ASSERT_EQ(kNumDeps, log_deps->node_count);
110110
}
111111

@@ -116,7 +116,7 @@ TEST_F(DepsLogTest, LotsOfDeps) {
116116
EXPECT_TRUE(log2.Load(kTestFilename, &state2, &err));
117117
ASSERT_EQ("", err);
118118

119-
DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out.o"));
119+
DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out.o", 0));
120120
ASSERT_EQ(kNumDeps, log_deps->node_count);
121121
}
122122

@@ -132,9 +132,9 @@ TEST_F(DepsLogTest, DoubleEntry) {
132132
ASSERT_EQ("", err);
133133

134134
vector<Node*> deps;
135-
deps.push_back(state.GetNode("foo.h"));
136-
deps.push_back(state.GetNode("bar.h"));
137-
log.RecordDeps(state.GetNode("out.o"), 1, deps);
135+
deps.push_back(state.GetNode("foo.h", 0));
136+
deps.push_back(state.GetNode("bar.h", 0));
137+
log.RecordDeps(state.GetNode("out.o", 0), 1, deps);
138138
log.Close();
139139

140140
struct stat st;
@@ -154,9 +154,9 @@ TEST_F(DepsLogTest, DoubleEntry) {
154154
ASSERT_EQ("", err);
155155

156156
vector<Node*> deps;
157-
deps.push_back(state.GetNode("foo.h"));
158-
deps.push_back(state.GetNode("bar.h"));
159-
log.RecordDeps(state.GetNode("out.o"), 1, deps);
157+
deps.push_back(state.GetNode("foo.h", 0));
158+
deps.push_back(state.GetNode("bar.h", 0));
159+
log.RecordDeps(state.GetNode("out.o", 0), 1, deps);
160160
log.Close();
161161

162162
struct stat st;
@@ -186,14 +186,14 @@ TEST_F(DepsLogTest, Recompact) {
186186
ASSERT_EQ("", err);
187187

188188
vector<Node*> deps;
189-
deps.push_back(state.GetNode("foo.h"));
190-
deps.push_back(state.GetNode("bar.h"));
191-
log.RecordDeps(state.GetNode("out.o"), 1, deps);
189+
deps.push_back(state.GetNode("foo.h", 0));
190+
deps.push_back(state.GetNode("bar.h", 0));
191+
log.RecordDeps(state.GetNode("out.o", 0), 1, deps);
192192

193193
deps.clear();
194-
deps.push_back(state.GetNode("foo.h"));
195-
deps.push_back(state.GetNode("baz.h"));
196-
log.RecordDeps(state.GetNode("other_out.o"), 1, deps);
194+
deps.push_back(state.GetNode("foo.h", 0));
195+
deps.push_back(state.GetNode("baz.h", 0));
196+
log.RecordDeps(state.GetNode("other_out.o", 0), 1, deps);
197197

198198
log.Close();
199199

@@ -216,8 +216,8 @@ TEST_F(DepsLogTest, Recompact) {
216216
ASSERT_EQ("", err);
217217

218218
vector<Node*> deps;
219-
deps.push_back(state.GetNode("foo.h"));
220-
log.RecordDeps(state.GetNode("out.o"), 1, deps);
219+
deps.push_back(state.GetNode("foo.h", 0));
220+
log.RecordDeps(state.GetNode("out.o", 0), 1, deps);
221221
log.Close();
222222

223223
struct stat st;
@@ -237,14 +237,14 @@ TEST_F(DepsLogTest, Recompact) {
237237
string err;
238238
ASSERT_TRUE(log.Load(kTestFilename, &state, &err));
239239

240-
Node* out = state.GetNode("out.o");
240+
Node* out = state.GetNode("out.o", 0);
241241
DepsLog::Deps* deps = log.GetDeps(out);
242242
ASSERT_TRUE(deps);
243243
ASSERT_EQ(1, deps->mtime);
244244
ASSERT_EQ(1, deps->node_count);
245245
ASSERT_EQ("foo.h", deps->nodes[0]->path());
246246

247-
Node* other_out = state.GetNode("other_out.o");
247+
Node* other_out = state.GetNode("other_out.o", 0);
248248
deps = log.GetDeps(other_out);
249249
ASSERT_TRUE(deps);
250250
ASSERT_EQ(1, deps->mtime);
@@ -287,14 +287,14 @@ TEST_F(DepsLogTest, Recompact) {
287287
string err;
288288
ASSERT_TRUE(log.Load(kTestFilename, &state, &err));
289289

290-
Node* out = state.GetNode("out.o");
290+
Node* out = state.GetNode("out.o", 0);
291291
DepsLog::Deps* deps = log.GetDeps(out);
292292
ASSERT_TRUE(deps);
293293
ASSERT_EQ(1, deps->mtime);
294294
ASSERT_EQ(1, deps->node_count);
295295
ASSERT_EQ("foo.h", deps->nodes[0]->path());
296296

297-
Node* other_out = state.GetNode("other_out.o");
297+
Node* other_out = state.GetNode("other_out.o", 0);
298298
deps = log.GetDeps(other_out);
299299
ASSERT_TRUE(deps);
300300
ASSERT_EQ(1, deps->mtime);
@@ -361,14 +361,14 @@ TEST_F(DepsLogTest, Truncated) {
361361
ASSERT_EQ("", err);
362362

363363
vector<Node*> deps;
364-
deps.push_back(state.GetNode("foo.h"));
365-
deps.push_back(state.GetNode("bar.h"));
366-
log.RecordDeps(state.GetNode("out.o"), 1, deps);
364+
deps.push_back(state.GetNode("foo.h", 0));
365+
deps.push_back(state.GetNode("bar.h", 0));
366+
log.RecordDeps(state.GetNode("out.o", 0), 1, deps);
367367

368368
deps.clear();
369-
deps.push_back(state.GetNode("foo.h"));
370-
deps.push_back(state.GetNode("bar2.h"));
371-
log.RecordDeps(state.GetNode("out2.o"), 2, deps);
369+
deps.push_back(state.GetNode("foo.h", 0));
370+
deps.push_back(state.GetNode("bar2.h", 0));
371+
log.RecordDeps(state.GetNode("out2.o", 0), 2, deps);
372372

373373
log.Close();
374374
}
@@ -420,14 +420,14 @@ TEST_F(DepsLogTest, TruncatedRecovery) {
420420
ASSERT_EQ("", err);
421421

422422
vector<Node*> deps;
423-
deps.push_back(state.GetNode("foo.h"));
424-
deps.push_back(state.GetNode("bar.h"));
425-
log.RecordDeps(state.GetNode("out.o"), 1, deps);
423+
deps.push_back(state.GetNode("foo.h", 0));
424+
deps.push_back(state.GetNode("bar.h", 0));
425+
log.RecordDeps(state.GetNode("out.o", 0), 1, deps);
426426

427427
deps.clear();
428-
deps.push_back(state.GetNode("foo.h"));
429-
deps.push_back(state.GetNode("bar2.h"));
430-
log.RecordDeps(state.GetNode("out2.o"), 2, deps);
428+
deps.push_back(state.GetNode("foo.h", 0));
429+
deps.push_back(state.GetNode("bar2.h", 0));
430+
log.RecordDeps(state.GetNode("out2.o", 0), 2, deps);
431431

432432
log.Close();
433433
}
@@ -448,16 +448,16 @@ TEST_F(DepsLogTest, TruncatedRecovery) {
448448
err.clear();
449449

450450
// The truncated entry should've been discarded.
451-
EXPECT_EQ(NULL, log.GetDeps(state.GetNode("out2.o")));
451+
EXPECT_EQ(NULL, log.GetDeps(state.GetNode("out2.o", 0)));
452452

453453
EXPECT_TRUE(log.OpenForWrite(kTestFilename, &err));
454454
ASSERT_EQ("", err);
455455

456456
// Add a new entry.
457457
vector<Node*> deps;
458-
deps.push_back(state.GetNode("foo.h"));
459-
deps.push_back(state.GetNode("bar2.h"));
460-
log.RecordDeps(state.GetNode("out2.o"), 3, deps);
458+
deps.push_back(state.GetNode("foo.h", 0));
459+
deps.push_back(state.GetNode("bar2.h", 0));
460+
log.RecordDeps(state.GetNode("out2.o", 0), 3, deps);
461461

462462
log.Close();
463463
}
@@ -471,7 +471,7 @@ TEST_F(DepsLogTest, TruncatedRecovery) {
471471
EXPECT_TRUE(log.Load(kTestFilename, &state, &err));
472472

473473
// The truncated entry should exist.
474-
DepsLog::Deps* deps = log.GetDeps(state.GetNode("out2.o"));
474+
DepsLog::Deps* deps = log.GetDeps(state.GetNode("out2.o", 0));
475475
ASSERT_TRUE(deps);
476476
}
477477
}

‎src/graph.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ bool Edge::use_console() const {
331331
string Node::PathDecanonicalized() const {
332332
string result = path_;
333333
unsigned int mask = 1;
334-
for (char* c = &result[0]; (c = strpbrk(c, "/\\")) != NULL;) {
334+
for (char* c = &result[0]; (c = strchr(c, '/')) != NULL;) {
335335
if (slash_bits_ & mask)
336336
*c = '\\';
337337
c++;

‎src/includes_normalize.h

+1-2
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ struct IncludesNormalize {
2929
static string Relativize(StringPiece path, const string& start);
3030

3131
/// Normalize by fixing slashes style, fixing redundant .. and . and makes the
32-
/// path relative to |relative_to|. Case is normalized to lowercase on
33-
/// Windows too.
32+
/// path relative to |relative_to|.
3433
static string Normalize(const string& input, const char* relative_to);
3534
};

‎src/manifest_parser_test.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ TEST_F(ParserTest, IgnoreIndentedComments) {
9696
ASSERT_EQ(2u, state.rules_.size());
9797
const Rule* rule = state.rules_.begin()->second;
9898
EXPECT_EQ("cat", rule->name());
99-
Edge* edge = state.GetNode("result")->in_edge();
99+
Edge* edge = state.GetNode("result", 0)->in_edge();
100100
EXPECT_TRUE(edge->GetBindingBool("restat"));
101101
EXPECT_FALSE(edge->GetBindingBool("generator"));
102102
}

‎src/state.cc

-7
Original file line numberDiff line numberDiff line change
@@ -111,13 +111,6 @@ Edge* State::AddEdge(const Rule* rule) {
111111
return edge;
112112
}
113113

114-
Node* State::GetNode(StringPiece path) {
115-
#if defined(_WIN32) && !defined(NDEBUG)
116-
assert(strpbrk(path.AsString().c_str(), "/\\") == NULL);
117-
#endif
118-
return GetNode(path, 0);
119-
}
120-
121114
Node* State::GetNode(StringPiece path, unsigned int slash_bits) {
122115
Node* node = LookupNode(path);
123116
if (node)

‎src/state.h

-1
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,6 @@ struct State {
9595

9696
Edge* AddEdge(const Rule* rule);
9797

98-
Node* GetNode(StringPiece path);
9998
Node* GetNode(StringPiece path, unsigned int slash_bits);
10099
Node* LookupNode(StringPiece path) const;
101100
Node* SpellcheckNode(const string& path);

0 commit comments

Comments
 (0)