Skip to content

Comments

Preserve ninja=false via msbuild#124562

Merged
jkoritzinsky merged 1 commit intodotnet:mainfrom
am11:feature/build/ninja-integration
Feb 19, 2026
Merged

Preserve ninja=false via msbuild#124562
jkoritzinsky merged 1 commit intodotnet:mainfrom
am11:feature/build/ninja-integration

Conversation

@am11
Copy link
Member

@am11 am11 commented Feb 18, 2026

While investigating quoting issue, I found build.sh -ninja false is not honored via msbuild.

The actual quoting fix is also included, so if we had space in dotnet path, it will be parsed by cmake correctly.

@jkoritzinsky
Copy link
Member

/ba-g test failures unrelated to build change

@jkoritzinsky jkoritzinsky enabled auto-merge (squash) February 19, 2026 00:43
@akoeplinger
Copy link
Member

/ba-g test failures unrelated to build change

@jkoritzinsky jkoritzinsky merged commit cae181e into dotnet:main Feb 19, 2026
172 of 176 checks passed
@am11 am11 deleted the feature/build/ninja-integration branch February 19, 2026 12:17
@tmds
Copy link
Member

tmds commented Feb 21, 2026

@steveisok what is the intended behavior when building the vmr? I assume we don't want to make ninja a required build dependency/assume it is available? Does it default to Ninja=false (during vmr build)?

@am11
Copy link
Member Author

am11 commented Feb 21, 2026

Ninja could be optional for VMR, but it’s a lightweight package and improves build performance, so making it required may be worth considering.

@tmds
Copy link
Member

tmds commented Feb 21, 2026

It will only speed up a very small part of the vmr build, right?

@am11
Copy link
Member Author

am11 commented Feb 21, 2026

Yes, it's about 10-15s saving and VMR cross build (e.g. on linux-riscv64) takes like 60-70m in GitHub Actions, so it's a tiny fraction. OTOH, ninja-build's download size is < 150 KB on Ubuntu and Fedora (and 66 KB on Alpine). 🤷‍♀️

@tmds
Copy link
Member

tmds commented Feb 21, 2026

about 10-15s saving and VMR cross build (e.g. on linux-riscv64) takes like 60-70m

If you're just building runtime, it's a nice gain.

For vmr builds, it's not significant.

Also, ninja isn't available everywhere.

$ podman run centos:stream9 dnf install ninja-build
CentOS Stream 9 - BaseOS                        4.6 MB/s | 8.9 MB     00:01    
CentOS Stream 9 - AppStream                     4.3 MB/s |  27 MB     00:06    
CentOS Stream 9 - Extras packages                26 kB/s |  20 kB     00:00    
No match for argument: ninja-build
Error: Unable to find a match: ninja-build

I think it makes sense to turn this off by default for the vmr build.

@am11
Copy link
Member Author

am11 commented Feb 21, 2026

Also, ninja isn't available everywhere.

It's available in crb: dnf config-manager --enable crb && dnf install ninja-build

@tmds
Copy link
Member

tmds commented Feb 21, 2026

I don't think we should ninja a default build dependency for a 10-15s speedup on a 60-70min build.

@tmds
Copy link
Member

tmds commented Feb 21, 2026

Or alternatively, the build can automatically use ninja depending on whether it is installed.

@am11
Copy link
Member Author

am11 commented Feb 21, 2026

automatically use ninja depending on whether it is installed.

That's how it's been working on mono for a while. It was discussed in the original PR #124041 (comment) and decision was made to make ninja on by default.

@tmds
Copy link
Member

tmds commented Feb 21, 2026

I'm assuming this was added to improve quality of life for the devs that works on runtime.

I don't think it was actively considered whether or not this was meant to be a default build dependency for vmr.

I find it to be only a very limited gain on the vmr build time to make it a default dependency.

cc @jkotas @jkoritzinsky

@jkotas
Copy link
Member

jkotas commented Feb 21, 2026

This was meant to be a default dependency for vmr too. We want to make sure that we are building the same by default everywhere.

@tmds
Copy link
Member

tmds commented Feb 21, 2026

It could be an option to consistently build the vmr with Ninja=false?

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

Labels

area-Infrastructure-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants