unicode-org / icu4x

Solving i18n for client-side and resource-constrained environments.
https://icu4x.unicode.org
Other
1.38k stars 176 forks source link

Test building ICU without lockfile #2966

Open robertbastian opened 1 year ago

robertbastian commented 1 year ago

I've been told we have the lock file in order for CI to be stable. However, isn't that hiding errors? If we need a certain lock file configuration in order for icu, icu_capi, or icu_datagen to build, then they might fail for our clients without us detecting that.

Current scenarios which I think are masked by Cargo.lock:

In both cases we need to pin that dependency in Cargo.toml so that it's also pinned for our clients.

If removing Cargo.lock is considered too radical, we could have a CI job (MSRV?) that builds without it, or at least add building without it to the release checklist (this will delay potential work to release time, however).

The cargo guide says libraries should not check it in.

sffc commented 1 year ago

Stable CI is the most important outcome, but I agree that our code base is starting to get detached from ecosystem evolution; we've seen issues come up in cargo-make and elsewhere.

I would support more frequent updates of the Cargo.lock file. Once we start updating the project's rust toolchain every 6 weeks for Rust releases, we could make a lockfile bump part of that process.

It would be cool if a bot could generate the PR automatically.

sffc commented 1 year ago

Decision:

Manishearth commented 1 year ago

Some tips for someone who wants to work on this:

We might want to be a bit pickier about the matrix. I think it's fine to only run the lockfile tests on a single platform, and we can achieve that with exclude rules.