Skip to content

Conversation

@Colecf
Copy link
Contributor

@Colecf Colecf commented Dec 20, 2023

These were added to regular ninja in:
ninja-build/ninja@04c410b

Android uses them, so they're needed to port android to n2. (which is something I'm exploring but not committing to)

These were added to regular ninja in:
ninja-build/ninja@04c410b

Android uses them, so they're needed to port android to n2. (which is
something I'm exploring but not committing to)
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.

Wow, this appears completely perfect -- thanks for writing the comments and the test, and also the logic seems right to me!

@evmar evmar merged commit c1fcd4a into evmar:main Dec 21, 2023
@evmar
Copy link
Owner

evmar commented Dec 21, 2023

This is so cool! I'm very curious about whether n2 works out for Android.

I think there's likely some easy low hanging perf fruit but I haven't tried n2 yet on any reasonably large build -- the app I worked on had like 1k edges total. I think the architecture is mostly in the right to be able to scale, but there are surely some simple things I overlooked, like maybe some O(n) in the number of ready edges or something silly like that.

And further I think it would be relatively easy to add larger ideas like multithreading for build.ninja parsing if that's useful.

In general, if you have any build.ninja files to share for investigating, or feature requests or observations etc., I'd love to hear them and help out where I can!

@evmar
Copy link
Owner

evmar commented Dec 21, 2023

Also, if for Android purposes you wanted to explore more invasive changes like a binary file format, I would be happy to explain any part of n2 you need. But from this PR it feels like you already have pretty much gotten it.

@Colecf
Copy link
Contributor Author

Colecf commented Dec 21, 2023

Thanks! I'm glad you're open to supporting android. Upstream backing is definitely a factor in the switch.

There's still a lot of work/investigation to port android to n2, but some other thoughts I had while looking into it:

  • Currently we hit the "used generated file, but has no dependency path to it" error, which is probably an issue in our ninja file
  • We rely on a number of flags that are currently unsupported in n2, but maybe they're not all necessary. The frontend_file flag is pretty important though.
  • Our ninja fork does have multithreaded parsing. I haven't benchmarked the difference between it and n2 yet.
  • I'm not sure a binary format would be feasible for us, because all of our build system rules emit ninja code as mostly raw strings. So if there was a binary format, we'd have to either do a ton of work to rewrite all the rules or parse the text and convert it to binary.
  • I'm also investigating adding action sandboxing (probably with nsjail) and remote build (RBE) support to ninja. I was thinking maybe instead of shoehorning that into ninja/n2 (particuarly RBE), it might be better if n2 had some kind of plugin api to offload executing the actions to another program. And that program could do the RBE calls.
  • Some other minor features that our ninja fork has are an ETA based on prior runs, and the ability to tag build actions for later analysis.

@evmar
Copy link
Owner

evmar commented Dec 22, 2023

Now that we're on the subject, didn't I see some rumor about Android switching to bazel? Is that still happening?

@Colecf
Copy link
Contributor Author

Colecf commented Dec 22, 2023

Unfortunately the migration to bazel was recently cancelled. :/

@evmar
Copy link
Owner

evmar commented Dec 22, 2023

Is it worth opening bugs about the remaining issues?
Re the frontend_file, I had experimented a bit with #34 which feels pretty similar, and had ideas like https://www.figma.com/file/ewO2MBU5MXlt5i3cZsoBrC/Ninja-VSCode-integration?type=design&node-id=0%3A1&mode=design&t=taX8UFxmqs7Xo0II-1

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