Investigate non-reproducible build

[sethp]

What we know: when @dougli1sqrd tried to build b5a9c5cb7e4175deab0a42bc59b4855bf9b6d62d (pre-#19), he observed build failures, e.g.:

failed to resolve on #[interrupt]

and

let qspi = Spi::new_quad_send_only(
   |                     ^^^^^^^^^^^^^^^^^^
   |                     |
   |                     function or associated item not found in `Spi<'_, _>`

Spi::new_quad_send_only is a function we've added in our rustbox/esp-hal fork, and the "failed to resolve" indicates an older version of the #[interrupt] proc macro than I was using, so it seemed like he was building against a stale version of the upstream repository. Moving his Cargo.lock aside, the next cargo build successfully produced an artifact.

Why it is?

[sethp]

Ah, a clue:

git checkout b5a9c5cb7e41
cp Cargo.lock.eric Cargo.lock
cargo tree | grep -E 'esp-hal-common|esp32c3-hal'

produced:

warning: Patch `esp-hal-common v0.7.1 (https://github.com/rustbox/esp-hal?rev=716934cd#716934cd)` was not used in the crate graph.
Patch `esp32c3-hal v0.7.0 (https://github.com/rustbox/esp-hal?rev=716934cd#716934cd)` was not used in the crate graph.
Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.
├── esp32c3-hal v0.5.0
│   ├── esp-hal-common v0.5.0

and, sure enough, the Cargo.lock pins e.g. esp-hal-common to v0.5.0 upstream:

[[package]]
name = "esp-hal-common"
version = "0.5.0"
source = "registry+https://github.com/rust-lang/crates.io-index"

[sethp]

Well, a more targeted solution seems to be cargo update -p esp32c3-hal, which offers:

    Removing esp-hal-common v0.5.0
      Adding esp-hal-common v0.7.1 (https://github.com/rustbox/esp-hal?rev=716934cd#716934cd)
...
    Removing esp32c3-hal v0.5.0
      Adding esp32c3-hal v0.7.0 (https://github.com/rustbox/esp-hal?rev=716934cd#716934cd)

among others

[sethp]

Still unknown: where did that v0.5.0 in the lock file come from?

[dougli1sqrd]

Wow love this trail of investigation! Yeah, also curious where v0.5.0 comes from. Did I have that in a Cargo.toml before an update to Cargo.toml, updating that dep?

[sethp]

Hmm, I wonder? I can't find a complete story that's represented in the git history: in c75f742ab4 we moved from explicitly providing the esp-hal-common transitive dependency (which confused rust-analyzer):

https://github.com/rustbox/esp32c3-vgaterm/blob/31b626fcfaed95ec08d5d2175283e123894efac5/Cargo.toml#L39-L45

to selecting version "*" (which, more on that in a minute) and using a [patch.crates-io] to override the version:

https://github.com/rustbox/esp32c3-vgaterm/blob/c75f742ab44a8c4405cf32f4997865874944c264/Cargo.toml#L39-L41 https://github.com/rustbox/esp32c3-vgaterm/blob/c75f742ab44a8c4405cf32f4997865874944c264/Cargo.toml#L59-L61

though that change didn't show up on the keyboard-help branch until https://github.com/rustbox/esp32c3-vgaterm/commit/6cb9a637896fc079c485d61a42e7dab9129a1eec (Mar 13).

I can't quite form that into a coherent narrative: from the vgaterm branch in our fork is v0.4.0, so it seems you should've jumped from v0.4.0 to v0.7.0 as the esp-hal "major" version.

The time before that we touched Cargo.toml was https://github.com/rustbox/esp32c3-vgaterm/commit/8f00fe0554801341be3389c3bc224fadbc9cda52 , which was roughly contemporaneous with 0.5.0 being the latest version of the esp-hal stuff, which ran from https://github.com/esp-rs/esp-hal/commit/d03c267084cc95662246656b66cc855d984926f8 (Jan 26th) to https://github.com/esp-rs/esp-hal/commit/4e88e48bbe7eb47ec23d26cd8437a32400ee8278 (Feb 10th), but that fact doesn't help much unless we have a way to explain why cargo would use a (really) stale version of the crates-io repository?

I guess now's as good a time as any to compare cargo versions? I'm experimenting with

cargo 1.70.0-nightly (7d3033d2e 2023-03-08)

aforementioned "more on "*""

I haven't been able to confirm this yet (experimental ideas definitely welcome), but I have a suspicion that "*" is giving us some grief. The purpose of "*" there is to say "don't pick a version, defer to the patch'd git source," but maybe it only appears to be doing that and is actually very broken.

I don't think there's any way you would've set the version to v0.5.0 on purpose, because that's also missing the Spi::new_quad_send_only function. But, cargo's default behavior is maximal version selection (see my other comment for the deets), and "*" means "any version," so there's a possible world where this happened:

0. you checked out the Cargo.toml from 6cb9a637, but since Cargo.lock was still gitignore'd at that time, your local state persisted through the tree update. Meaning the lock file you had was generated from e.g. 8f00fe0554801341be3389c3bc224fadbc9cda52 or the moral equivalent, where via cargo generate-lock-file I've confirmed that cargo seems to lock to "v0.4.0" from the vgaterm branch in our git source (as we asked it).
1. when cargo re-ran dependency resolution after introducing the new Cargo.toml, it, for whatever reason, thought v0.5.0 was the latest upstream version of the esp-hal packages (despite this happening in mid-March)
2. because we'd asked for whatever version ("*"), and cargo default to maximal, it said "ok, here's your 0.5.0!" and writes that to the lock file
3. only at this point in the story does it start to consider the [patch.crates-io], wherein it finds a new source for esp-hal, and traversing to that sees a Cargo.toml representing v0.7.0

That would produce the stable state we observed: the lock file says esp32c3-hal = "*" is v0.5.0, and because "esp32c3-hal v0.5.0" is a totally different package to cargo from "esp32c3-hal v0.7.0",[^1] it says "couldn't apply that patch to any crate I found in the graph, guess it doesn't matter!" and then immediately fail to compile.

I'd really like to validate that story before going too far down the "possible fixes" road; but it's past time to hit "comment" already.

[^1]: I still find this very confusing, but I think the key in the [patch.crates-io] section is more or less ignored by cargo. So from our perspective we said "please override the hal from the repository with ours", but from cargo's we might as well have written nom = { package="serde", git = "..."}; it'll ignore the nom string completely, and follow the git link to figure out what it should even try to replace.

[sethp]

The test procedure I've got is:

1. Replace Cargo.lock with an older version (I haven't been 100% consistent, but they're all pointing to 0.4.0 sourced from our fork)
2. Modify Cargo.toml to point to one of the commits here: https://github.com/rustbox/esp-hal/tree/test/patch-version-behind-crates-io-latest
   1. https://github.com/esp-rs/esp-hal/commit/025164552c5126a28d571ee04886dc87edd3e63f corresponds to esp32c3-hal v0.6.0 ("older" than what state my cargo is currently resolving for the crates.io repository)
   2. https://github.com/esp-rs/esp-hal/commit/25fef6ac26cda8028395d15ccfa1fbefc482cd0e corresponds to esp32c3-hal v0.0.0 ("does not exist")
   3. https://github.com/esp-rs/esp-hal/commit/6c13ec1865bd8a4ddd6111c95aefbcab6c338e67 corresponds to esp32c3-hal v0.8.0 ("newer")
3. run cargo tree, which implicitly will update the lockfile and tell me what versions it's selected.

(encoded as a script: https://gist.github.com/sethp/cc6a065fe3c73a2efa2872c40ad81489#file-test-sh ). I did turn off the patch for esp-hal-common in Cargo.toml, as the path = "../esp-hal-common" in esp32c3-hal seemed to be enough for it to get picked up.

The good news is that in each of these, "*" solves the problem it was intended to, i.e. it resolves to the patched source version in each of the above scenarios. I can't find another specifier that behaves that way; i.e. any number I pick works just as long as the patched revision is a "major"-version match and bombs out with some "unused" nonsense otherwise.

Trying again with the likely version of cargo you were using:

rustup install nightly-2023-01-12
rustup toolchain link eric $(rustc +nightly-2023-01-12 --print sysroot)
./test.sh +eric tree

Yields the same results. I didn't look too hard, but one difference that might be worth following up on between our versions is that mine is using the sparse registry protocol by default; it was noticeable in my testing just how much longer it took to update the crates.io index.

Some ideas for what to try next:

1. Re-testing with esp-hal-common set to the same version as esp32c3-hal (above it was all v0.7.1), +/- also adjusting the "version" requirement on esp32c3-hal to see what effect that has vs. path
2. Regenerating Cargo.lock.saved from Cargo.toml in a different commit, using the earlier version of cargo, to see if that's (meaningfully) different
3. Possibly, mucking with the registry / git repo state to try and reset it to model the edge transition you followed: it seems probable that you had a very stale crates.io index (circa the 0.5.0 release maybe?) that got updated concurrently with pulling down new objects from our fork as well

I suppose the good news is that 2 is v. easy, and 1 should model the most common flow we expect (i.e. esp-rs/esp-hal releasing a new version into crates.io ahead of ours). So if those results are both negative, I think we can at least say we aren't mis-using cargo in an obvious way? And that it's at least somewhat reasonable to expect this to continue to work?

Also, we'll be in a slightly better position if this happens again now that we've got an auditable history for Cargo.lock: at least we won't have to guess what that piece of the "before" state looked like! For the same reason, we'll be in an even better position yet if we tackle #14. I wonder if there are other easy wins for pinning down the big ball o' mutable state that is cargo?