Skip to content

Conversation

@hinshun
Copy link
Contributor

@hinshun hinshun commented Apr 23, 2025

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:

  • Need several modules to be public
  • Need to access the value of dep but it's dropped when initializing the parse_showincludes field
  • Need to share Build ins and outs with threads so wanted that to be easily Cloneable
  • Want to iterate over DenseMap more generally instead of just GraphFiles

@hinshun hinshun force-pushed the feature/minimal-pub branch from 384e1ca to 0bbf908 Compare April 23, 2025 00:57
@hinshun hinshun force-pushed the feature/minimal-pub branch from 0bbf908 to 341b511 Compare April 23, 2025 00:58
Copy link
Owner

@evmar evmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

@evmar evmar Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It maybe is, but even if we ever added some new mechanism, adding it to an enum is reasonable too.

@hinshun
Copy link
Contributor Author

hinshun commented Apr 23, 2025

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.

That is fine, we will handle the updates / breakages.

(You should be also aware that n2 doesn't implement all of ninja's semantics, which means for some projects it will build wrong...)

I understand, this is good enough as a starting point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants