-
Notifications
You must be signed in to change notification settings - Fork 52
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
[fud2] Implement Shell Commands With Dependencies #2224
Conversation
- swapped inputs and outputs in build command - disregarding error messages inside defops
oops, my git is bad, that didn't quite do what I thought it would |
dc65f3f
to
8020b02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neato! Thanks for separating this out so it was easier to review; it was a lot easier to grok this after understanding #2203.
As you'll see, my big comment is that I don't think that the benefit of allowing mixtures of shell
and shell_deps
is worth it. It makes the logic pretty weird, it will make the Ninja harder to understand, and it doesn't seem like a feature people will actually want. I think we could trim back the conceptual complexity here if each op had to choose between the two modes of specifying commands.
The previous commit separates out In particular, this implementation tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! This all looks great as well.
Implements shell commands with dependencies. This has been split off from the origin PR for ease of review.