CI: test with commited lock files

[tcharding]

A while back we added two lock files, one for testing with recent dependency versions and one for testing with minimal dependency versions but at the time we never used them in CI.

Move the two current lock files and use them in CI (mirroring what is done in rust-bitcoin).

[tcharding]

The WASM test does not play nicely with the recent/minimal lock files. I'm not sure right now what is the solution, called through to contrib/_test.sh for now. hmmm, needs more thought.

[apoelstra]

@tcharding could you move the rename of the lockfiles to the first commit (you can do it in its own commit, or combine it, it just needs to be the first one). That would make my local CI easier to tweak.

Then the WASM thing I think is about paths being somehow screwed up by the new script indirection. You can see that in the DO_WASM branch we edit Cargo.toml using echo, and after this PR, we're getting some sort of error related to parsing Cargo.toml.

[tcharding]

The wasm thing was that now we run the ci script twice and in the wasm test we modify the toml file it was getting modified twice so had duplicate [lib] section. Another classic case of tunnel vision as we discussed yesterday

Caused by:
  TOML parse error at line 77, column 1
     |
  77 | [lib]
     | ^
  invalid table header
  duplicate key `lib` in document root

I saw the "invalid table header" line and immediately focused on that, I rekon I did not even read the the next line, today I read the "duplicate key" line and realised the problem immediately.

I'd hate to have to make life threatening decisions with this brain :)

[tcharding]

Hit another issue with the wasm-bingen-backend dependency. While debugging I noticed that the lock file gets updated by, I assume, calls to wasm-pack. So I think we should just skip testing against the minim/recent lock files for WASM. Is that acceptable?

[tcharding]

On ice for another day.

[apoelstra]

Hit another issue with the wasm-bingen-backend dependency. While debugging I noticed that the lock file gets updated by, I assume, calls to wasm-pack. So I think we should just skip testing against the minim/recent lock files for WASM. Is that acceptable?

I'll spend a little bit of time seeing if I can get it to run inside nix, and if so, what's involved. But I think it's fine if we just can't test this stuff properly.

[apoelstra]

So I think we should just skip testing against the minim/recent lock files for WASM. Is that acceptable?

I was able to get wasm-pack to work inside nix (sorta, because of rustwasm/wasm-pack#814 I've had to hack up some paths; and the unit tests aren't working because of "missing env" errors that our C patches should be preventing), and (a) it doesn't need to connect to the Internet, if you prod it enough, so it's not doing anything gratuitously non-deterministic; and (b) it doesn't appear to be modifying lockfiles, at least when run in a hermetic container where it can't access new versions of anything from crates.io.

I think we should investigate further.

[apoelstra]

oo https://rustwasm.github.io/wasm-pack/book/commands/build.html suggests we can add -- --locked to the wasm-pack commands. @tcharding can you try that?

Edit: I'm unconvinced that --locked actually does anything. If you run a normal cargo --locked command without a lockfile, or with an empty lockfile, it'll fail. But wasm-pack build -- --locked seems to continue to work. If you provide a corrupt lockfile, e.g. by copying Cargo.toml to Cargo.lock, you will get an error Error during execution of cargo metadata: error: failed to parse lock file at: /build/source/Cargo.lock. So it does sound like wasm-pack is doing something illegitimate with lockfiles.

But we could try --offline and see if that prevents them changing. Or we could just copy the old lockfiles again after running wasm commands.

[apoelstra]

Oh, I see, the issue is that cargo options are not actually passed to cargo metadata. See https://github.com/rustwasm/wasm-pack/blob/7d6501d3cd272b7ea27034158d88bfd5c0005913/src/manifest/mod.rs#L408-L410

So indeed, wasm-pack is unconditionally running cargo metadata with no flags to prevent it from modifying the lockfile.

We should use diff or something to check that it is, but it sounds like we need to patch the lockfile afterward.

[apoelstra]

I filed a bug at rustwasm/wasm-pack#1316. Meanwhile I think the workaround is easy: we copy the lockflies, run the wasm-pack commands, then copy them again. I don't think wasm-pack will actually respect the copied lockfile, but we can live with that for now.

[tcharding]

Awesome, thanks for digging into it. I'll push it forward from here.

[tcharding]

This needs rebasing on #641 to pass CI.

[tcharding]

Rebase, no other changes.

[apoelstra]

I don't think this PR actually uses the committed lockfiles anywhere :)

[tcharding]

I don't think this PR actually uses the committed lockfiles anywhere :)

Epic fail. I'm confident this was a rebase fail and not me being totally retarded.

[apoelstra]

In f4078f8:

This warning is harmless enough, but "cargo update yields later dependencies" is neither necessary nor sufficient for us to want to update Cargo-recent.lock. I expect this warning to literally always trigger because it finds later dependencies that are incompatible with our MSRV.

[tcharding]

It only runs for "recent" but I agree, and thought at the time that we added it to rust-bitcoin that it was not that useful. Didn't seem to hurt though, and since its informational I acked it. I'm happy to remove it everywhere but I'd prefer to wait till @Kixunil is back because he added it.

[Kixunil]

You have to open the file in GH to see the warning so it at least won't be intrusive.

[apoelstra]

Could you re-order the first commit so that the "rename the lockfiles" commit comes first? It is much easier for me to handle PRs if every commit has the lockfiles in the same place.

[tcharding]

Can do, my bad - you asked that before once.

[apoelstra]

ACK 3da39c6