Alex Crichton (Feb 26 2020 at 21:56): Alex Crichton (Feb 26 2020 at 21:56):Hey all wanted to follow-up here to make a topic to talk about https://github.com/bytecodealliance/cranelift/issues/1408 if necessary. I'm thinking that if it still sounds good to everyone we could shoot for pulling the trigger perhaps closer to the weekend, say this Friday, to iron out some stuff over the weekend if necessary and avoid interrupting too many workflows?
Joey Gouly (Feb 27 2020 at 10:53):What is the top level repo going to be called? Your example looks weird with it being called wasmtime, with a cranelift repo inside of it.
Till Schneidereit (Feb 27 2020 at 12:43):the top-level repo will indeed be called Wasmtime. I agree that the naming is a bit unfortunate, but the two alternatives—inverting things and having Wasmtime under Cranelift, or having both in some generically-named repo—seem strictly worse
Till Schneidereit (Feb 27 2020 at 12:43):See also my comment in the issue for the reasoning behind the merge
Joey Gouly (Feb 27 2020 at 13:37):I read your comment. I just wish there was another way to do it, without merging the repos. It feels like cranelift is going to be 'less' of a standalone project.
Till Schneidereit (Feb 27 2020 at 13:54):we looked at alternatives quite extensively, but unfortunately nobody has good solutions for multi-repo setups with circular dependencies.
I don't think there's a risk of Cranelift actually becoming less of a standalone project: just within the Bytecode Alliance we have multiple non-wasmtime use cases that are way too important. Perception is a different matter, I admit, so we should do what we can to make it clear that Cranelift very much has its own, independent identity.
Speaking of which, @Alex Crichton, how bad would it be to move the cranelift directory to the top-level? I understand that it's normal for dependencies to live under crates, but from a visibility perspective it'd be better to have it at the top-level
Joey Gouly (Feb 27 2020 at 14:01):Yes, even a top level directory would give better perception / perspective.
Till Schneidereit (Feb 27 2020 at 14:04):I agree with this, yes. If the downsides aren't too significant, it'd be good to make this change
Joey Gouly (Feb 27 2020 at 14:17):And people can still use a git dependency on cranelift? I'm not doing it, but I think people are
bjorn3 (Feb 27 2020 at 14:21):Joey Gouly said:
And people can still use a git dependency on cranelift? I'm not doing it, but I think people are
I am using a git dep on cranelift.
Till Schneidereit (Feb 27 2020 at 14:22):Yes, that still works. Here's what the cargo docs have to say on that matter:
Cargo will fetch the git repository at this location then look for a Cargo.toml for the requested crate anywhere inside the git repository (not necessarily at the root - for example, specifying a member crate name of a workspace and setting git to the repository containing the workspace).
Alex Crichton (Feb 27 2020 at 15:12):Yes git deps will still work and top level dir is totally doable
Alex Crichton (Feb 27 2020 at 15:56):in terms of crate organization, how do folk sfeel about a crates dir inside of a top-level cranelift dir? So you'd have something like cranelift/crates/codegen? Or would y'all prefer to stick with cranelift/cranelift-codegen (or cranelift/codegen)?
Alex Crichton (Feb 27 2020 at 15:56):sort of whether to carry over https://github.com/bytecodealliance/cranelift/pull/1359
Till Schneidereit (Feb 27 2020 at 16:03):@Alex Crichton so https://github.com/alexcrichton/wasmtime/tree/cranelift-merge/crates/cranelift/src would become https://github.com/alexcrichton/wasmtime/tree/cranelift-merge/cranelift/src, and all sub-crates of cranelift would move under cranelift/crates, and lose the cranelift- prefix?
Till Schneidereit (Feb 27 2020 at 16:03):if so, that seems good to me
Alex Crichton (Feb 27 2020 at 16:04):right yeah, although we have the option of cranelift/crates/$crate, cranelift/$crate, or cranelift/cranelift-$crate
Alex Crichton (Feb 27 2020 at 16:04):(I'd personally advocate for the first)
Till Schneidereit (Feb 27 2020 at 16:06):I guess the first is closest to "this really is its own thing and would in a better world be its own, normally organized, repository", so I agree that that makes sense
Alex Crichton (Feb 28 2020 at 16:21):@Benjamin Bouvier @Dan Gohman do y'all feel ok about performing the merge today? With the final crate structure being craneflit/crates/$crate?
Alex Crichton (Feb 28 2020 at 16:22):or should we postpone to gather more feedback?
bjorn3 (Feb 28 2020 at 16:23):I prefer cranelift/$crate.
Benjamin Bouvier (Feb 28 2020 at 16:26):@Alex Crichton There's one suggestion that shouldn't be too hard to implement, and maybe you've already done it: could we make sure that transferred issues are marked with a new "cranelift" label in wasmtime? with that, i don't have any objections left!
Alex Crichton (Feb 28 2020 at 16:27):@Benjamin Bouvier certainly yeah, I was planning on tweaking the script to do that
Alex Crichton (Feb 28 2020 at 16:27):and we'll have a bit of label-reorganization to do after the merge, but I don't think it'll be too hard
Dan Gohman (Feb 28 2020 at 16:28):@Alex Crichton makes sense to me too.
Dan Gohman (Feb 28 2020 at 16:28):On cranelift/$crate vs cranelift/crates/$crate, are we antitipating having non-crate things under cranelift/?
Alex Crichton (Feb 28 2020 at 16:28):Ok so in terms of logistics, I don't mind taking the afternoon today to actually do the merge. Are there opinions though on how to like "close the tree" for cranelift and such?
Alex Crichton (Feb 28 2020 at 16:29):otherwise I can just "do the steps" and then clean up afterwards if anything was merged in the meantime
Dan Gohman (Feb 28 2020 at 16:30):It seems ok to just announce it's happening here in Zulip, and add a comment in https://github.com/bytecodealliance/cranelift/issues/1408
Alex Crichton (Feb 28 2020 at 16:32):mk, sounds good. As for the final crate organization, I don't really have a horse in this race, I'm happy to implement whatever
Dan Gohman (Feb 28 2020 at 16:32):We can also mark the Cranelift repo as archived in Guthub, which makes it readonly
Alex Crichton (Feb 28 2020 at 16:32):yeah I don't think I'll do that this afternoon since it may be a bit too soon (and we may want to work with authors on PRs), but as a long-term goal that sounds good
Dan Gohman (Feb 28 2020 at 16:33):Yeah, it's not urgent. If people do commit stuff to Cranelift after the merge, it shouldn't be hard to move the patches over.
Dan Gohman (Feb 28 2020 at 16:41):Ok, I'm ok either way, but I also vote for cranelift/$crate
Benjamin Bouvier (Feb 28 2020 at 16:45):I'm all in favor of fewer indirection levels, so cranelift/$crate is my preferred choice too, but no strong opinions here.
Benjamin Bouvier (Feb 28 2020 at 16:45):thanks @Alex Crichton for doing it, either way
Alex Crichton (Feb 28 2020 at 17:16):I've posted a PR for this to work on at https://github.com/bytecodealliance/wasmtime/pull/1019
Joey Gouly (Feb 28 2020 at 17:23):@Chris Fallin in case you missed this ^
Chris Fallin (Feb 28 2020 at 17:34):Thanks, Joey, yep definitely aware! Re: the arm64 side-branch, I think I'm actually going to try some git filter-branch magic to rewrite our commits on top of the rewritten Cranelift base; this will keep us sane when we merge from upstream. (The plan is still to collapse into a stack of logical patch commits when we finally merge...)
Alex Crichton (Feb 28 2020 at 17:38):@Chris Fallin btw the script I ended up with to rebase PRs should actually work for you I think
Alex Crichton (Feb 28 2020 at 17:38):it'll keep the history and everything, and in theory won't require any intervention from your part as long as it rebases on master
Chris Fallin (Feb 28 2020 at 17:57):@Alex Crichton you mean, once we get to the point that we're merging and have a PR? Or to use now to rewrite our branch? If you could share the script that would be very useful, thanks!
Alex Crichton (Feb 28 2020 at 18:07):@Chris Fallin the interim script I was testing looks like https://gist.github.com/alexcrichton/8cb3f4ef7c25317ba6824ee31e3e53aa
Alex Crichton (Feb 28 2020 at 18:08):although for the actual merge I'd need to tweak a few variables
Alex Crichton (Feb 28 2020 at 18:08):do you have a branch I could poke around on to see how the script works?
Chris Fallin (Feb 28 2020 at 18:09):@Alex Crichton our WIP branch is at repo https://github.com/cfallin/cranelift, branch new-isa-def-2 (we should really rename that...).
Thanks, this script is a good starting point, I'm happy to play around with it!
Alex Crichton (Feb 28 2020 at 18:10):ok cool, thanks!
Alex Crichton (Feb 28 2020 at 18:10):I'll confirm it works as expected on that branch
Alex Crichton (Feb 28 2020 at 18:53):@Chris Fallin ok so the updated script -- https://gist.github.com/alexcrichton/8cb3f4ef7c25317ba6824ee31e3e53aa -- has the variables configured and should work as-is
Alex Crichton (Feb 28 2020 at 18:53):I'm realizing though it's best to execute the commands manually one-by-one
Alex Crichton (Feb 28 2020 at 18:53):because at least for your branch there were a number of rebase things to take care of
Alex Crichton (Feb 28 2020 at 18:53):(and a rebase failure bailed out on the script and it's not great at resuming in the middle)
Alex Crichton (Feb 28 2020 at 18:54):I ran the script though and produced https://github.com/alexcrichton/wasmtime/tree/new-isa-def-2 which should be your branch, but rebased on top of the wasmtime merge
Chris Fallin (Feb 28 2020 at 18:56):Fantastic, thank you for that!
Alex Crichton (Feb 28 2020 at 21:09):Ok I think everything should be ready to go, @Dan Gohman do you want review and/or hit the merge button on https://github.com/bytecodealliance/wasmtime/pull/1019?
Alex Crichton (Feb 28 2020 at 21:09):(would recommend making sure it's a merge and not a squash-and-merge button)
Alex Crichton (Feb 28 2020 at 21:10):also once we do the actual merge into wasmtime I'll start the transfer of issues
Alex Crichton (Feb 28 2020 at 21:11):and I'll comment leaving a link to https://gist.github.com/alexcrichton/8cb3f4ef7c25317ba6824ee31e3e53aa on all existing PRs
Alex Crichton (Feb 28 2020 at 21:11):but I figure it's better to leave PRs open until authors get around to moving/closing
Dan Gohman (Feb 28 2020 at 21:16):I'm in a video call atm, but I'll take a look as soon as I'm done
Dan Gohman (Feb 28 2020 at 21:16):I'm in a meeting atm, but I'll take a look as soon as I'm done.
Dan Gohman (Feb 28 2020 at 21:17):Don't multitask and Zulip at the same time, apparently.
Dan Gohman (Feb 28 2020 at 22:58):Reviewed!
Alex Crichton (Feb 28 2020 at 23:13):Ok I've merged now! Gonna do lots of clean up now at this point
Alex Crichton (Feb 28 2020 at 23:15):@Dan Gohman wanna merge https://github.com/bytecodealliance/cranelift/pull/1414 to prevent future cranelift merges?
Dan Gohman (Feb 28 2020 at 23:16):+3 −201,782
Andrew Brown (Feb 28 2020 at 23:20):@Alex Crichton it must be sort of satisfying to delete that many lines of code
Dan Gohman (Feb 28 2020 at 23:21):Uh oh, am I getting emailed for every open Cranelift issue?
Dan Gohman (Feb 28 2020 at 23:23):But only the original posts, and not all the comments?
Dan Gohman (Feb 28 2020 at 23:23):Oh, " This issue is being transferred. Timeline may not be complete until it finishes. "
Andrew Brown (Feb 28 2020 at 23:25):Also, can someone transfer over the user permissions? @Dan Gohman?
Alex Crichton (Feb 28 2020 at 23:25):oh uh oh
Alex Crichton (Feb 28 2020 at 23:26):@Dan Gohman are you getting a lot of emails?
Alex Crichton (Feb 28 2020 at 23:26):I didn't think issue transfers did emails
Dan Gohman (Feb 28 2020 at 23:26):I am getting a notification for each issue
Alex Crichton (Feb 28 2020 at 23:26):oh hey there's the hundreds of emails for me too
Alex Crichton (Feb 28 2020 at 23:26):may wanna unwatch for a moment...
Alex Crichton (Feb 28 2020 at 23:26):I can take care of permissions @Andrew Brown
Dan Gohman (Feb 28 2020 at 23:27):Well, that's what filters are for ;-)
Alex Crichton (Feb 28 2020 at 23:29):ok that's all issues transferred
Alex Crichton (Feb 28 2020 at 23:33):ok permissions should be copied over
Andrew Brown (Feb 28 2020 at 23:34):cool, looks like it worked!
Alex Crichton (Feb 28 2020 at 23:37):ok and PRs have been commented
Alex Crichton (Feb 28 2020 at 23:38):I tried to preserve the labels as much as possible for issues, although many imported into wasmtime now have a cranelift: prefix
Alex Crichton (Feb 28 2020 at 23:39):folks should feel free to edit those as they see fit though
Alex Crichton (Feb 28 2020 at 23:39):that was just to make sure we know what's what
fitzgen (he/him) (Feb 28 2020 at 23:44):do we have docs on how to rebase wasmtime PRs (not cranelift PRs) for this merge?
Alex Crichton (Feb 28 2020 at 23:46):@fitzgen (he/him) that should just be a typical git rebase
Alex Crichton (Feb 28 2020 at 23:46):wasmtime didn't move around with this merge
fitzgen (he/him) (Feb 28 2020 at 23:46):cool
Andrew Brown (Feb 28 2020 at 23:50):should branches before-merge and after-merge already exist?
Andrew Brown (Feb 28 2020 at 23:50):or do we provide them to the helper script?
Andrew Brown (Feb 28 2020 at 23:51):^ @Alex Crichton
Alex Crichton (Feb 28 2020 at 23:52):@Andrew Brown hm the branches should already exist
Alex Crichton (Feb 28 2020 at 23:53):https://github.com/alexcrichton/cranelift/tree/after-move and https://github.com/alexcrichton/cranelift/tree/before-merge
Andrew Brown (Feb 28 2020 at 23:53):Maybe you should push those to bytecodealliance/cranelift? https://github.com/bytecodealliance/cranelift/branches/all
Alex Crichton (Feb 28 2020 at 23:54):sure I can do that
Andrew Brown (Feb 28 2020 at 23:54):or should I not have changed CRANELIFT_REMOTE to my own remote?
Andrew Brown (Feb 28 2020 at 23:56):ok, I see; we should just use your repo and then at the end push that branch to our fork of wasmtime
Alex Crichton (Feb 28 2020 at 23:57):it does appear I messed this up
Alex Crichton (Feb 28 2020 at 23:57):in my repo
Alex Crichton (Feb 28 2020 at 23:57):I'll push correct things to the cranelift repo, sorry about that
Alex Crichton (Feb 28 2020 at 23:58):ok updated gist
Alex Crichton (Feb 28 2020 at 23:58):and branches are in the cranelift repo
Alex Crichton (Feb 28 2020 at 23:59):er, the bytecodealliance repo
Andrew Brown (Feb 29 2020 at 00:00):I'll try it out
Chris Fallin (Feb 29 2020 at 00:02):Hmm, it looks like the default build doesn't include clif-util anymore? Was that intended?
Chris Fallin (Feb 29 2020 at 00:02):(I haven't done very much with large trees of subcrates so I dunno if just cargo build is insufficient)
Alex Crichton (Feb 29 2020 at 00:04):@Chris Fallin it depends on where you build sorta, if you cargo build inside of cranelift/ it should build it
Alex Crichton (Feb 29 2020 at 00:04):but if you cargo build at the root that's building the wasmtime CLI
Chris Fallin (Feb 29 2020 at 00:05):Ah -- root Cargo.toml needs a tweak it seems?:
[cfallin@cfallin-laptop cranelift]$ cargo build error: current package believes it's in a workspace when it's not: current: /home/cfallin/wasmtime/cranelift/Cargo.toml workspace: /home/cfallin/wasmtime/Cargo.toml this may be fixable by adding `cranelift` to the `workspace.members` array
Andrew Brown (Feb 29 2020 at 00:06):Hm, I'm trying out the new gist and it is having a hard time with the rename from, e.g. cranelift-codegen to cranelift/cranelift-codegen
Dan Gohman (Feb 29 2020 at 00:06):While we're reorganizing things, does renaming the clif-util program to either cranelift or clif resonate with anyone?
Andrew Brown (Feb 29 2020 at 00:06)::+1: for clif
Andrew Brown (Feb 29 2020 at 00:07):actually, I change my vote: I think cranelift makes more sense now that it is in the wasmtime repo
Alex Crichton (Feb 29 2020 at 00:07):@Chris Fallin oh oops, I'll fix that
Alex Crichton (Feb 29 2020 at 00:08):@Andrew Brown what sort of issues are you having?
Alex Crichton (Feb 29 2020 at 00:08):or got a branch I can try out?
Alex Crichton (Feb 29 2020 at 00:09):@Chris Fallin should be fixed in https://github.com/bytecodealliance/wasmtime/pull/1190
Alex Crichton (Feb 29 2020 at 00:12):@Andrew Brown oh if you add a brand new file it has no idea what to do with that (git)
Alex Crichton (Feb 29 2020 at 00:12):in terms of renames that is
Alex Crichton (Feb 29 2020 at 00:12):you'll have to manually rename new files
Andrew Brown (Feb 29 2020 at 00:12):@Alex Crichton, I sorted it now: my branch was messed up from my previous run
Andrew Brown (Feb 29 2020 at 00:12):once I hard-reset it to what I had on my fork, then the script works great!
Alex Crichton (Feb 29 2020 at 00:12):ah ok nice
Andrew Brown (Feb 29 2020 at 00:13):but yeah, new files we will have to move around before creating the new PR: https://github.com/bytecodealliance/wasmtime/pull/1191/checks?check_run_id=476064308
Joey Gouly (Mar 03 2020 at 12:55):@Alex Crichton I get this warning now: $ cargo test -p cranelift-codegen
warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package: /home/joey/src/wasmtime/cranelift/Cargo.toml
workspace: /home/joey/src/wasmtime/Cargo.toml
Should I be running the tests another way?
Joey Gouly (Mar 03 2020 at 12:59):also how can I 'cargo run', to run clif-util? I tried cargo run -p cranelift and cargo run -p clif-util
Alex Crichton (Mar 03 2020 at 14:46):@Joey Gouly you may need to rebase to get the profile thing to go away
Alex Crichton (Mar 03 2020 at 14:46):I'm not getting that locally
Alex Crichton (Mar 03 2020 at 14:46):I fixed an issue with that I believe
Alex Crichton (Mar 03 2020 at 14:47):you should also be albe to run clif-util with cargo run -p cranelift-tools --bin clif-util
Alex Crichton (Mar 03 2020 at 14:47):oh ou can drop the --bin too
Joey Gouly (Mar 03 2020 at 14:48):Ah thanks, got confused with the package name
Joey Gouly (Mar 03 2020 at 14:48):I'm on @Chris Fallin's branch, so guess I'll see the warning until it's updated
Alex Crichton (Mar 03 2020 at 14:54):FWIW the fix was pretty small an you can probably apply it locally
Joey Gouly (Mar 03 2020 at 14:54):Ah thanks
Chris Fallin (Mar 05 2020 at 15:38):Just a heads-up re: the merge -- it seems that there's a latent possibility of confusion. In https://github.com/bytecodealliance/wasmtime/issues/1174 just now, someone had found a link to cranelift#1174 (which was a PR) via the PR's commit message, but because of some confusing combination of redirects, ended up at wasmtime#1174, which is a completely unrelated issue. I'm not sure what can be done about this, other than to be aware...
Last updated: Dec 23 2024 at 13:07 UTC