Minimal non-breaking changes to support nix-ninja#140
Minimal non-breaking changes to support nix-ninja#140hinshun wants to merge 5 commits intoevmar:mainfrom
Conversation
384e1ca to
0bbf908
Compare
0bbf908 to
341b511
Compare
evmar
left a comment
There was a problem hiding this comment.
This seems mostly fine. I am still worried some n2 change I make in the future will break you but I also don't expect to tinker with it much in the future.
(You should be also aware that n2 doesn't implement all of ninja's semantics, which means for some projects it will build wrong...)
src/graph.rs
Outdated
| pub cmdline: Option<String>, | ||
|
|
||
| /// Controls how dependency information is processed after compilation. | ||
| pub deps: Option<String>, |
There was a problem hiding this comment.
Given this should only ever be "gcc" or "msvc", I think this should at least be an enum. (I don't really understand why you need this, it seems the bool carried as much info as before?)
There was a problem hiding this comment.
I was thinking of keeping it open as an extension point, e.g. for python dependency inference, etc. Is this the wrong use of the deps field?
There was a problem hiding this comment.
It maybe is, but even if we ever added some new mechanism, adding it to an enum is reasonable too.
That is fine, we will handle the updates / breakages.
I understand, this is good enough as a starting point. |
Hi @evmar, this is a follow up from: #138 (comment)
We have since open sourced https://github.com/pdtpartners/nix-ninja which depends on n2, but I landed on a number of small changes necessary to get it working. I've broken it down to logical commits but I'm also happy to break it up as separate PRs to discuss them individually.
High-level:
depbut it's dropped when initializing theparse_showincludesfield