1539406 - Bump Cranelift to pinned commits versions

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, task)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

... or just agree that we want to base ourselves off a pinned commit, instead of waiting for Cranelift bumps:

• this would make it easier to ensure that Spidermonkey can be used as a regression test suite for Cranelift, by reducing the amount of API changes needed on each bump.
• ditto for testing performances updates
• it would generally lower the latency between "something has been implemented/added in Cranelift" and "this Cranelift change is in Spidermonkey".

(Btw this is blocked on https://github.com/CraneStation/cranelift/pull/715, itself blocked on another dependency)

Dan said on IRC he agreed with this plan.

Nathan, can we move from a fixed release dependency scheme to pinned commits, for the Cranelift dependencies we have in tree? Any caveats we should be aware of?

Comment 1

[Nathan Froyd [:froydnj]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #0)

Nathan, can we move from a fixed release dependency scheme to pinned commits, for the Cranelift dependencies we have in tree? Any caveats we should be aware of?

I don't see why not, though I'm unclear on why this makes things happen any faster than new Cranelift versions, given who's in control of releasing new Cranelift versions. :)

You will want to make sure that the commits you pin never go away (so no pinned commits on branches that get force-pushed to); otherwise people using older versions of m-c (beta, release, esr, etc.) may run into problems when those commits disappear.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1539406: Bump Cranelift version in tree to c08d231708e49aee; r?lth

And remove the vendor patch for target-lexicon. The only interesting changes
are in .cargo/config.in, Cargo.toml and js/src/wasm/cranelift/Cargo.toml;
other changes have been automatically done by mach vendor rust.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

I don't see why not, though I'm unclear on why this makes things happen any faster than new Cranelift versions, given who's in control of releasing new Cranelift versions. :)

Currently only one person can release new Cranelift versions, and releasing a new version might imply to other Cranelift embedders that new features have been implemented in Cranelift. In fact, our goal here is just to make sure Cranelift in Spidermonkey doesn't get too outdated and follows the Cranelift API changes, so local development of Cranelift in Spidermonkey isn't too far away from the current Cranelift.

That being said, considering the impossible-to-explain build issues I'm running into (and that I can't locally reproduce), probably the simplest way forward is to bump the Cranelift version number and use this in Spidermonkey.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Nathan, Glandium, would one of you know what's going on with these build issues, or have time to investigate? Just trying to see if there's an obvious thing I could have missed here, before resorting to keeping on using fixed version numbers.

Comment 5

[Mike Hommey [:glandium]]

It looks like you updated to a version of cranelift that is not on crates.io, but .cargo/config.in and Cargo.toml don't know anything about that.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Mike Hommey [:glandium] from comment #5)

It looks like you updated to a version of cranelift that is not on crates.io, but .cargo/config.in and Cargo.toml don't know anything about that.

What should I add to these files so it works, please? I've tried cargo-culting (unintended pun) what was done for target-lexicon before my patch, but it didn't work eventually. (In the present case, I'm trying to refer to a revision of the github fork, not to a different URL or branch)

Comment 7

[Mike Hommey [:glandium]]

The dependencies in cranelift/Cargo.toml need to point to a version rather than the git repo/revision. Then the other Cargo.toml/.cargo/config changes are responsible for mapping that version to the git repo/revision.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks Mike, I'm trying this right now.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1539406: Bump Cranelift to revision 542d799dd7a3b2cc; r=lth

This is the first time we pin a specific Cranelift commit hash to use in Gecko.
The target-lexicon hack is removed and instead we introduce a vendor patch for
cranelift-codegen/cranelift-wasm themselves.

Notable changes happen in top-level Cargo.toml, .cargo/config.in and
js/src/wasm/cranelift/Cargo.toml; the rest has been generated by mach vendor rust.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

This new approach works, including for the JS rust and mozsys builds. Thanks Mike!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef00d4a58102
Bump Cranelift to revision 542d799dd7a3b2cc; r=lth

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ef00d4a58102