Closed Bug 1539406 Opened 7 months ago Closed 6 months ago

Bump Cranelift to pinned commits versions

Categories

(Core :: Javascript: WebAssembly, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

... 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?

Flags: needinfo?(nfroyd)

(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.

Flags: needinfo?(nfroyd)
Type: defect → enhancement

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.

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.

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.

Flags: needinfo?(nfroyd)
Flags: needinfo?(mh+mozilla)

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.

Flags: needinfo?(nfroyd)
Flags: needinfo?(mh+mozilla)

(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)

Flags: needinfo?(mh+mozilla)

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.

Flags: needinfo?(mh+mozilla)

Thanks Mike, I'm trying this right now.

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.

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

Attachment #9056515 - Attachment is obsolete: true
Attachment #9057922 - Attachment description: Bug 1539406: Bump Cranelift to revision 542d799dd7a3b2cc; r?lth → Bug 1539406: Bump Cranelift to revision 542d799dd7a3b2cc; r=lth
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef00d4a58102
Bump Cranelift to revision 542d799dd7a3b2cc; r=lth
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee: nobody → bbouvier
You need to log in before you can comment on or make changes to this bug.