rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.79k stars 2.42k forks source link

Detect CI-platforms and incremental compilation #11853

Open lukaslueg opened 1 year ago

lukaslueg commented 1 year ago

Problem

It seems to be consensus that using incremental compilation on CI platforms outweighs the benefits: A blank build starts from a blank cache, and the bookkeeping for incremental compilation is a cost we pay that never gets amortized, as the next build starts from a blank cache again or trashes the cache. People using GH-actions infrastructure like dtolnay/rust-toolchain already have incremental compilation disabled for them automatically due to this (see also).

Proposed Solution

It seems easy enough for Cargo to detect CI platforms, issue a warning if incremental compilation is enabled, and suggest to set CARGO_INCREMENTAL=0.

Notes

We should make sure that CI-platforms are detected reliably before issuing a warning. For example, a single CI-environment-variable will suffer from false positives and useless warnings.

Also, the warning issued by cargo might not reach the user, if the logs on a successful build are never reviewed. Yet, there is no cost to this problem.

weihanglo commented 1 year ago

It sounds great that Cargo knows when to enable incremental compilation. However, I feel like it falls into the same category of issue as this one: How many vendor-specific things do we want to include?

The conclusion of SSH key issue is that the Cargo team wants to remove all vendored keys once sparse registry becomes the default. Also, if we see this behaviour as a stable feature people can rely on, we cannot remove any one of them nearly forever.

In addition, people may have different cache policies in their CI workflow. Also highlight what dtolnay said,

However, workflows that cache the target directory across runs might be benefiting from incremental compilation.

Warnings might be fine as they won't break somebody's workflow. However, warnings could also be misleading and noisy if they are not applicable to their cache policy.

Given the configuration is easy to tweak, and you also created an awesome tool for the community, I lean to not adding this feature or even warnings. This is not a final decision though.