Set ninja=true by default

[joshtriplett]

Ninja substantially improves LLVM build time. On a 96-way system, using
Make took 248s, and using Ninja took 161s, a 35% improvement.

We already require a variety of tools to build Rust. If someone wants to
build without Ninja (for instance, to minimize the set of packages
required to bootstrap a new target), they can easily set ninja=false
in config.toml. Our defaults should help people build Rust (and LLVM)
faster, to speed up development.

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

This seems fine, but it looks like our CI does not have ninja installed -- presumably, that might mean that we could speed up CI builds as well, though they're all cached anyway (using sccache).

I suspect we can at least get ninja relatively easily on the linux builders by adding a bunch of ninja lines to all of the Dockerfiles in src/ci/docker/. Would you be up for doing that? Alternatively we can add a --set llvm.ninja=false to here which should fix CI entirely.

[joshtriplett]

@Mark-Simulacrum Our CI should test the default configuration to the extent possible, so I think we should add ninja to the Dockerfiles. I can do that.

[Mark-Simulacrum]

Okay, sounds good.

I don't know how easy it is to obtain Ninja on macOS or Windows. It might be good to change the default for those platforms, at least for now. It doesn't look like Ninja is included in the GHA environments on either platform (https://github.com/actions/virtual-environments/blob/main/images/macos/macos-10.15-Readme.md or https://github.com/actions/virtual-environments/blob/main/images/win/Windows2019-Readme.md).

[joshtriplett]

@Mark-Simulacrum Our CI for Windows, at least, appears to install Ninja by default, so it should work fine there.

[joshtriplett]

@Mark-Simulacrum I disabled it for macOS CI (via the configure invocation), and installed it in all the CI Dockerfiles.

[joshtriplett]

Fixing...

[mati865]

We do install ninja on Windows builders but really old version: https://github.com/rust-lang/rust/blob/b497e18995d6b6992f97512c6b86b5cb3f2f34f5/src/ci/scripts/install-ninja.sh

[joshtriplett]

@mati865 I saw that, yeah. We should probably update that, but if it works, we can do that separately.

[joshtriplett]

Looks like it works now.

@Mark-Simulacrum Does this seem reasonable now that it passes CI?

[Mark-Simulacrum]

There was some discussion on Zulip about how Ninja may be somewhat hard to obtain on older linux (e.g. RHEL7) and the story on macOS and Windows seems like it'd be similar.

I would like to change two things:

• Modify the check to emit nicer errors if it fails, something like "Ninja is required by default, but the default can be adjusted by setting llvm.ninja to false in config.toml".
• Disable the on-by-default nature for Windows and macOS, since we're not sure about the landscape there. I think it makes more sense to enable it for Linux, since there it is (usually) relatively easy to obtain, when coupling with the previous bulletpoint.

[mati865]

Disable the on-by-default nature for Windows and macOS, since we're not sure about the landscape there. I think it makes more sense to enable it for Linux, since there it is (usually) relatively easy to obtain, when coupling with the previous bulletpoint.

Within windows-gnu environment it's as simple as on Linux: pacman -S mingw-w64-x86_64-ninja.

[joshtriplett]

@Mark-Simulacrum Building on older enterprise distributions seems like a case where it'd be reasonable for people to need to explicitly set ninja=false, but it absolutely seems reasonable to provide a better diagnostic message steering people that way.

On Windows at least, the comment immediately after where you linked indicates that we very much want to use ninja on Windows if at all possible.

Ninja provides downloadable binaries for Windows, macOS, and Linux. I have no objection to making the default different for different host platforms, if it would otherwise be an imposition to people rather than a help. Is it actually hard to install Ninja on Windows or Mac, sufficiently so that most people would rather set ninja=false on those platforms and get slower builds?

[mati865]

In fact Windows has even more reasons to use Ninja than other platforms:

rust/src/bootstrap/sanity.rs

Lines 108 to 119 in 8611e52

	// If ninja isn't enabled but we're building for MSVC then we try
	// doubly hard to enable it. It was realized in #43767 that the msbuild
	// CMake generator for MSVC doesn't respect configuration options like
	// disabling LLVM assertions, which can often be quite important!
	//
	// In these cases we automatically enable Ninja if we find it in the
	// environment.
	if !build.config.ninja && build.config.build.contains("msvc") {
	if cmd_finder.maybe_have("ninja").is_some() {
	build.config.ninja = true;
	}
	}

[Mark-Simulacrum]

Downloadable binaries sounds about right to me. I'm happy with leaving the default on-by-default but let's definitely add a better error message. Maybe on Windows we'd want to give a message that's more cautionary (though maybe those problems have since been fixed, too).

[Mark-Simulacrum]

r=me with nit fixed, thanks!

(As an aside, I wish we had an exit! macro that did essentially a panic! but with different, more user-facing, default formatting).

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 67897629fa74bd53b2de64282ce09d13539d3ba3 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 67897629fa74bd53b2de64282ce09d13539d3ba3 with merge 9ffd86d7dc3810b3a7a5ce5cc4e75effb03de661...

[bors]

💔 Test failed - checks-actions

[Mark-Simulacrum]

Ah, that is indeed a reasonable theory. @bors r-

@joshtriplett I am not sure of the best way to fix this. I suspect the easy fix is to just not do the ninja check if builder.config.dry_run is true...

[Mark-Simulacrum]

Ah, that does look like a better fix. Hopefully it works :)

@bors r+

[bors]

📌 Commit 8b501e3 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 8b501e3 with merge dac556e3380a47863b5797405312361e1ef7bd1f...

[rust-log-analyzer]

The job dist-x86_64-msvc of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

[bors]

💔 Test failed - checks-actions

[joshtriplett]

Looks like a spurious download failure.

@bors retry

[bors]

⌛ Testing commit 8b501e3 with merge d8424f6...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing d8424f6 to master...

[pickfire]

@joshtriplett Why not use samurai instead of ninja, it is ninja-compatible but simpler so maybe it is faster?

[mati865]

Ninja is more popular and available to quickly install basically everywhere.
Samurai is not available in repositories of Ubuntu and many other distributions.