manifest_parser: remove multi-output depslog restriction

[mathstuf]

It seems to work as intended.

---

Cc: @RedBeard0531

[jhasse]

A test that it works as intended would be good (not only that it parses without error).

[mathstuf]

Here's a test file (courtesy of @RedBeard0531)

rule TOUCH
    command = touch $out

rule COPY
    command = echo 'using $in' && for file in $out; do cp $in $$file; done
    deps = msvc
    msvc_deps_prefix = using 

rule CHECK
    command = for file in $in; do if ! test -e $$file; then return 99; fi; done

build source: TOUCH
build dest1 dest2: COPY source
build test1: CHECK dest1
build test2: CHECK dest2
build test3: CHECK dest1 dest2
build test: phony test1 test2 test3
default test

ninja -t deps dest1 and ninja -t deps dest2 both output the expected depslog information. What's the best way to add this to the test suite?

[mathstuf]

Tests should also cover gcc parsing for the following two cases:

a b: in

and

a: in
b: in

[RedBeard0531]

It should also work with either of:

a: in

b: in

Especially when one of the outputs is an implicit output. This is important for cases like -gsplit-dwarf -MMD because the compiler seems to only list the "primary" output in its deps files.

That said, if it is harder to do this for make-style deps, getting it just for msvc-style would be an improvement for a few of my use cases, so I'd be happy to see msvc-style support land independently if that would get it in an earlier release.

[mathstuf]

Also found out the place to implement the tests for this (build_test.cc). I'll work on that this weekend.

[mathstuf]

@RedBeard0531 Noted, I'll add a test for that as well. Though we should get GCC to mention the .dwo file in the deps log if it doesn't already.

[mathstuf]

Tests added. @RedBeard0531 he b: in (or even b a: in) is currently treated the same as if you had a depfile not agreeing on the "primary" output name and is basically ignored. It's up to the generator to get the "primary" output correct.

[mathstuf]

Odd. Those failures aren't happening locally. Debugging.

[mathstuf]

Ugh. I wish GTest gave more information :/ . Can anyone else reproduce the test failures?

[mathstuf]

Passing on my local macOS build too. Will try with Windows later.

[jhasse]

I encourage you to consider writing a pull request that makes the configure.py script's "--with-gtest" actually do something (currently it does nothing at all, as the letters "gtest" are found exactly once in configure.py).

I think it would be better to remove that switch and always use googletest.

But probably don't start working on that until you've asked on the Ninja mailing list

Or opened an issue where you suggest the change.

as there's a decent chance (in my opinion) that such a PR would be either ignored or rejected.

PRs should no longer be ignored, I'm trying to change that. If you think there's an open one which should be looked at, just ping me :)

[jhasse]

PR for GoogleTest: #1562

You can cherry-pick the commit to get more information about the failed test.

[mathstuf]

Thanks, will look at this in the next week or so.

[mathstuf]

Ah ha. Installing re2c gets me to agree with the CI now. Tracking it down.

[mathstuf]

@jhasse Now that the cc.in file has right code too, CI is passing.

[RedBeard0531]

This is a rather severe limitation that prevents listing correct dependencies for a c++20 module scanner that outputs both a dyndeps file and a compiler flags file, when the compile task depends on both but order only for dyndeps and implicit for flags file. Additionally, the overall restriction has been lifted with the dyndeps patch because dyndeps can add implicit outputs to a deps=gcc build.

What is remaining to getting this merged?

[mathstuf]

There's the conflict to resolve, but more importantly, there is the test case I commented on above. What was labeled a "buggy" depfile is no accepted. Should it continue to be rejected? If so, further work is necessary.

[mathstuf]

Rebased and now rejects the case again.

[petrhosek]

We've the limitations around multiple outputs with deps when building Rust targets with Ninja. I've tested this change locally and it seems to be working as expected. Can we get this merged?

[mathstuf]

I've rebased on top of master.

[mathstuf]

Any chance of this for 1.10 too? :) @jhasse

[jhasse]

Let's see. It probably needs someone more experienced than me to review.

[mathstuf]

Hmm. @bradking? @nico? Thoughts on this PR?

[bradking]

The change makes sense over all. I've reviewed the logic a few times and it looks okay. The tests look thorough.

[mathstuf]

Updated to use std:: on all lines added/changed in this PR. I left std::strerror alone since <cstring> is not included anyways.