-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
internal: Use josh for subtree syncs #17025
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
688c30dc9f8434d63bddb65bd6a4d2258d19717c |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,12 @@ | ||
mod changelog; | ||
|
||
use std::process::{Command, Stdio}; | ||
use std::thread; | ||
use std::time::Duration; | ||
|
||
use anyhow::{bail, Context as _}; | ||
use directories::ProjectDirs; | ||
use stdx::JodChild; | ||
use xshell::{cmd, Shell}; | ||
|
||
use crate::{codegen, date_iso, flags, is_release_tag, project_root}; | ||
|
@@ -71,26 +78,154 @@ impl flags::Release { | |
} | ||
} | ||
|
||
impl flags::Promote { | ||
// git sync implementation adapted from https://github.com/rust-lang/miri/blob/62039ac/miri-script/src/commands.rs | ||
impl flags::RustcPull { | ||
pub(crate) fn run(self, sh: &Shell) -> anyhow::Result<()> { | ||
let _dir = sh.push_dir("../rust-rust-analyzer"); | ||
cmd!(sh, "git switch master").run()?; | ||
cmd!(sh, "git fetch upstream").run()?; | ||
cmd!(sh, "git reset --hard upstream/master").run()?; | ||
sh.change_dir(project_root()); | ||
let commit = self.commit.map(Result::Ok).unwrap_or_else(|| { | ||
let rust_repo_head = | ||
cmd!(sh, "git ls-remote https://github.com/rust-lang/rust/ HEAD").read()?; | ||
rust_repo_head | ||
.split_whitespace() | ||
.next() | ||
.map(|front| front.trim().to_owned()) | ||
.ok_or_else(|| anyhow::format_err!("Could not obtain Rust repo HEAD from remote.")) | ||
})?; | ||
// Make sure the repo is clean. | ||
if !cmd!(sh, "git status --untracked-files=no --porcelain").read()?.is_empty() { | ||
bail!("working directory must be clean before running `cargo xtask pull`"); | ||
} | ||
// Make sure josh is running. | ||
let josh = start_josh()?; | ||
|
||
let date = date_iso(sh)?; | ||
let branch = format!("rust-analyzer-{date}"); | ||
cmd!(sh, "git switch -c {branch}").run()?; | ||
cmd!(sh, "git subtree pull -m ':arrow_up: rust-analyzer' -P src/tools/rust-analyzer rust-analyzer release").run()?; | ||
// Update rust-version file. As a separate commit, since making it part of | ||
// the merge has confused the heck out of josh in the past. | ||
// We pass `--no-verify` to avoid running any git hooks that might exist, | ||
// in case they dirty the repository. | ||
sh.write_file("rust-version", format!("{commit}\n"))?; | ||
const PREPARING_COMMIT_MESSAGE: &str = "Preparing for merge from rust-lang/rust"; | ||
cmd!(sh, "git commit rust-version --no-verify -m {PREPARING_COMMIT_MESSAGE}") | ||
.run() | ||
.context("FAILED to commit rust-version file, something went wrong")?; | ||
|
||
if !self.dry_run { | ||
cmd!(sh, "git push -u origin {branch}").run()?; | ||
cmd!( | ||
sh, | ||
"xdg-open https://github.com/matklad/rust/pull/new/{branch}?body=r%3F%20%40ghost" | ||
) | ||
// Fetch given rustc commit. | ||
cmd!(sh, "git fetch http://localhost:{JOSH_PORT}/rust-lang/rust.git@{commit}{JOSH_FILTER}.git") | ||
.run() | ||
.map_err(|e| { | ||
// Try to un-do the previous `git commit`, to leave the repo in the state we found it it. | ||
cmd!(sh, "git reset --hard HEAD^") | ||
.run() | ||
.expect("FAILED to clean up again after failed `git fetch`, sorry for that"); | ||
e | ||
}) | ||
.context("FAILED to fetch new commits, something went wrong (committing the rust-version file has been undone)")?; | ||
|
||
// Merge the fetched commit. | ||
const MERGE_COMMIT_MESSAGE: &str = "Merge from rust-lang/rust"; | ||
cmd!(sh, "git merge FETCH_HEAD --no-verify --no-ff -m {MERGE_COMMIT_MESSAGE}") | ||
.run() | ||
.context("FAILED to merge new commits, something went wrong")?; | ||
|
||
drop(josh); | ||
Ok(()) | ||
} | ||
} | ||
|
||
impl flags::RustcPush { | ||
pub(crate) fn run(self, sh: &Shell) -> anyhow::Result<()> { | ||
let branch = self.branch.as_deref().unwrap_or("sync-from-ra"); | ||
let rust_path = self.rust_path; | ||
let rust_fork = self.rust_fork; | ||
|
||
sh.change_dir(project_root()); | ||
let base = sh.read_file("rust-version")?.trim().to_owned(); | ||
// Make sure the repo is clean. | ||
if !cmd!(sh, "git status --untracked-files=no --porcelain").read()?.is_empty() { | ||
bail!("working directory must be clean before running `cargo xtask push`"); | ||
} | ||
// Make sure josh is running. | ||
let josh = start_josh()?; | ||
|
||
// Find a repo we can do our preparation in. | ||
sh.change_dir(rust_path); | ||
|
||
// Prepare the branch. Pushing works much better if we use as base exactly | ||
// the commit that we pulled from last time, so we use the `rust-version` | ||
// file to find out which commit that would be. | ||
println!("Preparing {rust_fork} (base: {base})..."); | ||
if cmd!(sh, "git fetch https://github.com/{rust_fork} {branch}") | ||
.ignore_stderr() | ||
.read() | ||
.is_ok() | ||
{ | ||
bail!( | ||
"The branch `{branch}` seems to already exist in `https://github.com/{rust_fork}`. Please delete it and try again." | ||
); | ||
} | ||
cmd!(sh, "git fetch https://github.com/rust-lang/rust {base}").run()?; | ||
cmd!(sh, "git push https://github.com/{rust_fork} {base}:refs/heads/{branch}") | ||
.ignore_stdout() | ||
.ignore_stderr() // silence the "create GitHub PR" message | ||
.run()?; | ||
println!(); | ||
|
||
// Do the actual push. | ||
sh.change_dir(project_root()); | ||
println!("Pushing rust-analyzer changes..."); | ||
cmd!( | ||
sh, | ||
"git push http://localhost:{JOSH_PORT}/{rust_fork}.git{JOSH_FILTER}.git HEAD:{branch}" | ||
) | ||
.run()?; | ||
println!(); | ||
|
||
// Do a round-trip check to make sure the push worked as expected. | ||
cmd!( | ||
sh, | ||
"git fetch http://localhost:{JOSH_PORT}/{rust_fork}.git{JOSH_FILTER}.git {branch}" | ||
) | ||
.ignore_stderr() | ||
.read()?; | ||
let head = cmd!(sh, "git rev-parse HEAD").read()?; | ||
let fetch_head = cmd!(sh, "git rev-parse FETCH_HEAD").read()?; | ||
if head != fetch_head { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW this is pretty crucial. If you continue and merge this into rustc despite this check failing, the RA repo may need a force-push to fix the situation. That's what happened in Miri. I haven't seen this check fail in over a year. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I managed to do it:
So it's exactly the two pull commits that are missing. (This time I tried a pull, then push on the same branch). Yet here they are: https://github.com/lnicola/rust/commits/sync-from-ra/. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very strange. I don't know what to take from your You marked the PR as "ready for review" -- did you make more progress here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, but I think it's a good start, and the |
||
bail!("Josh created a non-roundtrip push! Do NOT merge this into rustc!"); | ||
} | ||
println!("Confirmed that the push round-trips back to rust-analyzer properly. Please create a rustc PR:"); | ||
// https://github.com/github-linguist/linguist/compare/master...octocat:linguist:master | ||
let fork_path = rust_fork.replace('/', ":"); | ||
println!( | ||
" https://github.com/rust-lang/rust/compare/{fork_path}:{branch}?quick_pull=1&title=Subtree+update+of+rust-analyzer&body=r?+@ghost" | ||
); | ||
|
||
drop(josh); | ||
Ok(()) | ||
} | ||
} | ||
|
||
/// Used for rustc syncs. | ||
const JOSH_FILTER: &str = | ||
":rev(55d9a533b309119c8acd13061581b43ae8840823:prefix=src/tools/rust-analyzer):/src/tools/rust-analyzer"; | ||
const JOSH_PORT: &str = "42042"; | ||
|
||
fn start_josh() -> anyhow::Result<impl Drop> { | ||
// Determine cache directory. | ||
let local_dir = { | ||
let user_dirs = ProjectDirs::from("org", "rust-lang", "rust-analyzer-josh").unwrap(); | ||
user_dirs.cache_dir().to_owned() | ||
}; | ||
|
||
// Start josh, silencing its output. | ||
let mut cmd = Command::new("josh-proxy"); | ||
cmd.arg("--local").arg(local_dir); | ||
cmd.arg("--remote").arg("https://github.com"); | ||
cmd.arg("--port").arg(JOSH_PORT); | ||
cmd.arg("--no-background"); | ||
cmd.stdout(Stdio::null()); | ||
cmd.stderr(Stdio::null()); | ||
let josh = cmd.spawn().context("failed to start josh-proxy, make sure it is installed")?; | ||
// Give it some time so hopefully the port is open. (100ms was not enough.) | ||
thread::sleep(Duration::from_millis(200)); | ||
|
||
Ok(JodChild(josh)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you create this file by hand? I would advise against that. This branch hasn't actually had a rustc-pull with the given commit. The file ideally is only ever written by rustc-pull.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was created by hand, but stale. I replaced it with an empty one.
It still needs to be there for the first
rustc-pull
, otherwisegit commit
will fail.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rustc-pull should create the file, so creating an empty one should not be needed.
In Miri this happens here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will, but it needs a
git add
.git commit rust-version
won't add it when it's new.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, damn... that's unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we do not. Pulls happen basically automatically: every morning a CI workflow tries to build and test Miri against the latest rustc HEAD, and if that fails it does a rustc-pull and creates a PR for that. (It is rare for Miri to change in rustc without some related internal rustc change that would make our CI fail.) Pushes I do manually every weekend.
It is definitely fine to rustc-pull as often as you want.
In the beginning I was worried about pushing twice without an intermediate pull. But josh seems to be deterministic -- when you do that, the first push will be perfectly re-constructed as part of the second push, so no duplicate commits are created. That said, we do pulls a lot more often than pushes, so we rarely have double-push without intermediate pull, so it's possible that under rare circumstances, this causes issues. I don't think I have seen that ever since we introduced the
rust-version
file to keep track of which commit to base the push on.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks. We'd normally do a push every week (on every release). Pulls are less important, since there's maybe one "downstream" change every 4-6 weeks.
But we did these in pairs, even when pulling didn't bring any changes, because
git-subtree
can get a little confused otherwise.How do you avoid empty pulls? Looking at https://github.com/rust-lang/miri/blob/master/.github/workflows/ci.yml#L193, I don't see anything that prevents them, but the job sometimes shows up as skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The job is skipped when nightly CI succeeds.
You're not directly using rustc internal APIs, I assume? Yeah then things look quite differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, that explains it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When doing a push without intermediate pull, it might be worth checking the commits that the github PR lists just to make sure there are no duplicates with the previous push. At least the first few times, until you are confident the tool works correctly. :)
Also, upgrading josh can sometimes mean the algorithm changes a bit. So it may be worth doing a "safety pull" when changing josh versions. They don't release very often though so that's not going to be coming up very much.