Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue for merging the cranelift and wasmtime repositories #1185

Closed
alexcrichton opened this issue Feb 25, 2020 · 8 comments
Closed
Labels
cranelift Issues related to the Cranelift code generator

Comments

@alexcrichton
Copy link
Member

Discussed at yesterday's meeting I wanted to open an issue tracking progress for merging the wasmtime/cranelift repositories. I think there are three major work items that need to happen:

Performing the actual merge

There's a few different ways to do the merge here, but I'd like to propose that the following steps are done.

  • Run git filter-branch --tree-filter $HOME/rewrite-cranelift.sh HEAD in the cranelift repository on the commit we want to merge in. The script looks like this.
  • Next create a merge commit with the wasmtime head commit and this new commit. The state at this point would look like https://github.com/alexcrichton/wasmtime/tree/cranelift-merge
    • This needs the --allow-unrelated-histories flag
  • Next do a bunch of manual work including:
    • Merge CI systems
    • Tidy up readmes/top-level markdowns/licenses/etc
    • Tidy up workspaces so there's only one
    • Update various scripts for testing
    • You can see a preview of this work and what all got merged/moved

If folks have workflows though that go beyond cargo test and want to make sure any scripts and such keep working, let me know! I'll try to make sure they work in the new repository.

Transferring open issues

This will involve creating a script to migrate all open issues from the bytecodealliance/cranelift repository to bytecodealliance/wasmtime.

I think the general guidelines for this will be:

  • Audit wasmtime labels to ensure that they contain all relevant cranelift labels. We should have a mapping from all cranelift labels to wasmtime labels.
  • Use GitHub's transfer issue feature to transfer all issues, using the above label mapping, to transfer issues to the wasmtime repository.

script: https://gist.github.com/alexcrichton/ef102cb315e548c92fd671c198da9aa2

Making it easier to migrate open PRs

I've created a script which should ideally make it easy to transfer PRs from one repository to another. The general gist of that script is something like this:

  • We'll pick a commit to merge cranelift into wasmtime. We'll push two commits into this cranelift repository at that time: before-merge and after-move. The before-merge commit is the one we decided to merge into wasmtime. The after-move commit is a single extra commit with the rewrite script from the first step applied.
  • You'll check out your local branch and run the provided script here, that will do the following...
  • First it will fetch cranelift remote information
  • Next it will rebase on before-merge, to make sure your PR is up-to-date. You may want to do this manually first.
  • Next it rebase onto the after-move commit. Git should automatically detect all your renamed files, and everything should get moved around automatically.
  • Next it'll fetch wasmtime's information (currently my own temporary repository and temporary branch)
  • Finally it'll cherry-pick your changes, which are changes at the right paths, onto the wasmtime repository.

At that point you should have a local git commit you can push up to your wasmtime fork and send as a PR. We'll need to, however, close PRs as they're reopened on wasmtime.

Once this all actually happens I can drop a comment on each PR with precise instructions with an updated script that has all the variables sorted out.

@iximeow
Copy link
Contributor

iximeow commented Feb 25, 2020

Has there been discussion of why we want to merge the Cranelift and Wasmtime repos somewhere? I know it's been on the cranelift meeting agenda for the past few weeks but I never saw context for it there either.

@bjorn3
Copy link
Contributor

bjorn3 commented Feb 25, 2020

  • Can the crates be moved to cranelift/* instead of crates/cranelift/*, to avoid a level of indirection?

  • Re-opening PR's on the new repo will scatter the discussion between the old cranelift repo and the new wasmtime + cranelift repo.

  • Merging Cranelift and Wasmtime doesn't make much sense to me. Cranelift is independent of Wasmtime, and is a dependency of many other projects, like wasmer, lucet, rustc_codegen_cranelift, ... By merging the Cranelift and Wasmtime repo's people will likely think that Cranelift is tightly coupled to Wasmtime.

  • By merging Cranelift and Wasmtime, I either have to accept that I will get notifications about Wasmtime issues and PR's, which I am not interested in, or I have to unwatch the combined repo, in which case I will miss Cranelift issues and PR's, which I am interested in.

@tschneidereit
Copy link
Member

The merge was discussed a few months ago. Originally we had wanted to do this as part of the move to the Bytecode Alliance GitHub org, but then had to put it on the backburner because it became clear that the complications @alexcrichton mentions above would take longer to work through.

I'll give a longer explanation below, but to put one thing front-and-center, the merge is not in any way about tightly coupling Cranelift to Wasmtime. Speaking with my Mozilla hat on, that doesn't make sense, because SpiderMonkey is, if anything, the more important embedding. And neither does it make sense from a Bytecode Alliance perspective: as @bjorn3 points out, with Lucet we have another embedding that's in production and really important. And we're also big fans of rustc_codegen_cranelift, and certainly don't want that to not be supported.

The current setup causes us to have significant inefficiencies in our workflows, because we often have to do things in Cranelift and a runtime in parallel, and coordinate landing and CI runs, etc. We realized this will become much worse over time, because we need to be able to fully test features that have tight compiler/runtime interactions.

Right now, we have somewhat rudimentary support for testing these kinds of features in Cranelift, but with various upcoming features—such as Interface Types, Linking, and GC—we need something much more sophisticated. We came to the conclusion that by far the best way to do this is to have that runtime be Wasmtime, because otherwise we'd end up duplicating a lot of functionality. And we'd not really have something that's production-quality, meaning that the quality of the integration would always be worse than what it could be with Wasmtime.

Unfortunately, this is currently not possible: circular dependencies are hugely painful across separate repositories, and just doesn't seem viable. Hence the plan to merge the repositories.

We believe the downsides this brings are solidly outweighed by the advantage of improving the productivity of people working on both Cranelift and Wasmtime—and thus ultimately also benefit other embeddings of Cranelift.

We’ll of course continue doing individual crate releases on crates.io, and we’ll make sure to have good Cranelift documentation independent from Wasmtime. We’ll provide more info about good workflows (including things around notification handling) based on these merged repositories once we get closer to actually doing the merge.

@alexcrichton
Copy link
Member Author

Ok I've created a sample repository of what the final merge looks like -- https://github.com/alexcrichton/wasmtime/commits/cranelift-merge-afterwards -- which has merges of CI, fuzzers, documentation, etc. Feel free to play around in that repo and let me know if you find any issues!

I've also created a script -- https://gist.github.com/alexcrichton/ef102cb315e548c92fd671c198da9aa2 -- to transfer all issues from one repository to another, while preserving labels. Tomorrow I'll work on how to transfer PRs in an "easy" fashion.

@alexcrichton
Copy link
Member Author

Ok I've now also created a script -- https://gist.github.com/alexcrichton/8cb3f4ef7c25317ba6824ee31e3e53aa -- to rebase a PR on the wasmtime repository. The tl;dr; is that you use git rebase to move all the files for you, then you cherry-pick all the commits onto the wasmtime repo.

I've updated the description above, and that should make it so we're all set from a tooling perspective. I think now we just need a good time to pull the trigger!

@alexcrichton
Copy link
Member Author

I've created a zulip stream if folks want to chat about this as well, and also as a less "email the world on everything" way to coordinate.

@alexcrichton
Copy link
Member Author

Ok unless there's any objects that come up, I plan on tackling this in the afternoon today and performing the actual merge. If you've got any concerns it's best to bring them up in zulip and @-mention me.

One final question being considered is the final crate organization. The example above has crates/cranelift/$crate, but it sounds like we're now leaning towards cranelift/$crate or cranelift/crates/$crate

@alexcrichton alexcrichton transferred this issue from bytecodealliance/cranelift Feb 28, 2020
@alexcrichton alexcrichton added the cranelift Issues related to the Cranelift code generator label Feb 28, 2020
@alexcrichton
Copy link
Member Author

And the merge is now done!

Code has been moved, issues transferred, permissions sync'd, and PRs notified. If there are any remaining questsions feel free to reach out on Zulip!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

No branches or pull requests

4 participants