internal: Use josh for subtree syncs #17025
Conversation
rustbot
added
the
S-waiting-on-review
|
Untested, and still need to update r? @RalfJung |
lnicola
force-pushed
the
josh
branch
3 times, most recently
from
2310b52
to
4c54702
Compare
|
@RalfJung do you know if Josh can use SSH remotes? EDIT: somewhat, it looks like, https://josh-project.github.io/josh/reference/container.html and josh-project/josh#1323 |
|
Doesn't seem to work: Sounds like I didn't delete the branch on my fork, but I did. |
|
I'm using josh via SSH, it's working fine for me. Also see the git config hint at the end of this section. |
|
It's hard to review this. The code seems to be mostly exactly like in Miri. So you'll just have to try it. First try pulling, and see if it produces only a few new commits compared to RA master. Opening a PR is a good way to check that as github lists all new commits. Then try pushing and see that that also only produces a few new commits. Then try changing something in the "pulled" branch and try pushing that, and try changing something in the pushed branch and try pulling that. |
| .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.
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.
I managed to do it:
$ git fetch https://github.com/rust-lang/rust 4e431fad67b46c480f1833119cd368fa33df95f7
remote: Enumerating objects: 161, done.
remote: Counting objects: 100% (120/120), done.
remote: Compressing objects: 100% (15/15), done.
remote: Total 161 (delta 107), reused 109 (delta 105), pack-reused 41
Receiving objects: 100% (161/161), 72.28 KiB | 7.23 MiB/s, done.
Resolving deltas: 100% (108/108), completed with 76 local objects.
From https://github.com/rust-lang/rust
* branch 4e431fad67b46c480f1833119cd368fa33df95f7 -> FETCH_HEAD
$ git push https://github.com/lnicola/rust 4e431fad67b46c480f1833119cd368fa33df95f7:refs/heads/sync-from-ra
Pushing rust-analyzer changes...
$ git push http://localhost:42042/lnicola/rust.git:rev(f5a9250147f6569d8d89334dc9cca79c0322729f:prefix=src/tools/rust-analyzer):/src/tools/rust-analyzer.git HEAD:sync-from-ra
Enumerating objects: 1335, done.
Counting objects: 100% (851/851), done.
Delta compression using up to 32 threads
Compressing objects: 100% (293/293), done.
Writing objects: 100% (500/500), 97.33 KiB | 48.67 MiB/s, done.
Total 500 (delta 332), reused 351 (delta 194), pack-reused 0 (from 0)
remote: Resolving deltas: 100% (332/332), completed with 143 local objects.
remote: josh-proxy
remote: response from upstream:
remote: To https://github.com/lnicola/rust.git
remote: 4e431fad67b..cc7848da115 JOSH_PUSH -> sync-from-ra
remote: REWRITE(1455ee0766bcb6075e6fdf797d24953f0af52b20 -> bf5c131477627b5e6273d5338d6b3ea137db4d41)
remote:
remote:
To http://localhost:42042/lnicola/rust.git:rev(f5a9250147f6569d8d89334dc9cca79c0322729f:prefix=src/tools/rust-analyzer):/src/tools/rust-analyzer.git
16b22118c3..1455ee0766 HEAD -> sync-from-ra
Error: Josh created a non-roundtrip push! Do NOT merge this into rustc!
$ git rev-parse HEAD
1455ee0766bcb6075e6fdf797d24953f0af52b20
$ git rev-parse FETCH_HEAD
56021a0d6123ced47c4db622f5b7f56a5f9ae97f
$ git log
commit 1455ee0766bcb6075e6fdf797d24953f0af52b20 (HEAD -> josh)
Merge: a2dfd9785f 16b22118c3
Author: Laurențiu Nicola
Date: Sun Apr 7 19:23:56 2024 +0300
Merge from downstream
commit a2dfd9785f167413355f98cc06e7cc7cb6b6a94b
Author: Laurențiu Nicola
Date: Sun Apr 7 19:23:54 2024 +0300
Preparing for merge from downstream
commit 56021a0d6123ced47c4db622f5b7f56a5f9ae97f (mine/josh)
Author: Laurențiu Nicola
Date: Sun Apr 7 11:41:39 2024 +0300
Use josh for subtree syncs
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.
Very strange. I don't know what to take from your git rev-parse, what do those show?
You marked the PR as "ready for review" -- did you make more progress here?
I can take a look eventually but probably not this week.
There was a problem hiding this comment.
Very strange. I don't know what to take from your git rev-parse, what do those show?
HEAD is the "Merge from downstream" commit, FETCH_HEAD is the josh change. Or maybe I misunderstood the question?
You marked the PR as "ready for review" -- did you make more progress here?
No, but I think it's a good start, and the FETCH_HEAD thing could conceivably be explained by my disk space problems. And it will be a while before I can try it again (since I have to reinstall and all that).
Well, I did. Pulling doesn't produce any commits, which is normal. But pushing fails as above. |
|
Hm, either the rust-version file needs to use a different commit then or the first push is magic and needs to be done by hand somehow... I forgot what exactly we did initially in Miri to get this started. What if you put the current rustc HEAD into the rust-version file? Are you sure pulling from the current rustc HEAD produces no new commits? There are usually some merge commits being added. It may be that you can only push after those have been integrated. |
|
Yes, I also tried that. Maybe we should do a pull first? |
|
Yeah try doing the push on a branch where a pull already happened. That also avoids having to hand craft that rust-version file.
|
|
Hmm, interesting:
|
xtask/src/release.rs
Outdated
| .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 downstream"; |
There was a problem hiding this comment.
So "downstream" is rustc? I think I've seen clippy use "upstream" for rustc. This is going to be confusing.^^
There was a problem hiding this comment.
Yeah, for us rust-lang/rust is the downstream since most of the development happens in this repo. I think that's the standard usage of the terms.
There was a problem hiding this comment.
It's standard if you know how the projects work, yeah. Not obvious to an outsider though. 🤷 In Miri I don't think of either one as being upstream or downstream, it's just two repos that sync back and forth.
Currently the two commits this adds are getting inconsistent messages, one says "merge from rustc" and the other "merge from downstream".
|
That's a lot of commits, but they seem to all be merge commits? Every merge commit in rustc that spans an RA change will show up here, this is needed for josh to be able to match the repo structures against each other. However, that still seems like too many commits. I don't think we got 1600 commits in Miri when we did the initial josh sync. |
|
Oh, I hate this: Besides the space needed for the rustc checkout for push (miri-script says 1 GB, but it's about 13 GB for me, including the submodules?), josh keeps another copy and a half of the repo in its cache: EDIT: Cursed thing broke my file system: |
Just give it a path to an existing rustc checkout you anyway use for rustc work. Then it takes 0 extra space. Also submodules are not needed. It doesn't need a checkout, just the git objects to work on. 1GB is the size increase that occurs when But yes the cache takes a bunch of space. And it seems to have hit some bug in btrfs? That's unfortunate. :/ |
|
I can reproduce the strange result for rustc-pull, where it generates an enormous amount of merge commits. This is not something we saw when switching Miri to Josh. I wonder if it's related to the fact that the git subtree syncs here look different than what I remember from Miri, with these huge commits created by rustc pushes. I spot-checked some of these commits and they all make sense, in a sense. For instance, |
|
However, pushing in the other direction seems to work just right for me. I did a pull, and then I changed something in the readme and did a push, and the result is this branch which does round-trip properly and which only adds 4 commits to rustc. I then tried changing something there and pulling that back into RA, which also worked fine. Not sure what went wrong when your push ended up non-roundtripping. If you pushed on a branch that didn't previously get a rustc-pull then that is probably the reason; anything you try to push should be based on what josh produces when pulling so that josh has a chance of matching up the histories and reconstructing a suitable RA history on the rustc side. |
|
I guess I must be doing something really wrong then. Here are my steps:
|
That is extremely strange. How did you determine that it only adds two commits? When I look at the history of my branch after doing this, I see a lot of new commits -- and you also saw these at least once, since at some point you pushed to this PR a branch that had all these commits. FWIW my version of josh is I have pushed the result of running $ git diff 2228677b93
diff --git a/crates/parser/src/lexed_str.rs b/crates/parser/src/lexed_str.rs
index 48e4c8a622..e5fec67de7 100644
--- a/crates/parser/src/lexed_str.rs
+++ b/crates/parser/src/lexed_str.rs
@@ -178,7 +178,7 @@ impl<'a> Converter<'a> {
rustc_lexer::TokenKind::Ident => {
SyntaxKind::from_keyword(token_text).unwrap_or(IDENT)
}
- rustc_lexer::TokenKind::InvalidIdent => {
+ rustc_lexer::TokenKind::InvalidPrefix | rustc_lexer::TokenKind::InvalidIdent => {
err = "Ident contains invalid characters";
IDENT
}
diff --git a/rust-version b/rust-version
index e2a22b2395..e1d07306ff 100644
--- a/rust-version
+++ b/rust-version
@@ -1 +1 @@
-688c30dc9f8434d63bddb65bd6a4d2258d19717c
+9c7b1f4848263b7a39486a2cd482db0d632884e8That's the same rustc commit as in your diff. But the change in rustc is missing. Something is seriously borked... maybe try clearing your josh cache? |
You're right, I didn't pay attention.
I have the same one. When I tried
It was fresh, I just reinstalled my OS. |
This is mine: https://github.com/lnicola/rust-analyzer/tree/josh-plus-rustc-pull |
|
Is there a way to tell what the last rustc commit is that got synced into RA? #16983 is the PR that did it but I can't see which rustc commits this includes (and which it does not include). |
|
The main issue, I think, is that doing rustc-pull adds a new root commit (8a7cde4). Ideally we'd add a programmatic check to ensure there are no new root commits, but I haven't yet found a good way to do that. (EDIT: Ah, found it: I don't quite understand why this happens. The root commit is the synced version of rust-lang/rust@43acb50 -- but then, for Miri there exists rust-lang/rust@f45b570 and somehow that does not show up in the josh-extracted history at all, and I don't understand why. |
We used to do this in pairs, and that sync was probably rust-lang/rust#123258, so it should be rust-lang/rust@688c30dc9f8. |
|
Sigh, perhaps submodules aren't that bad after all... |
|
I think the issues we are having here are due to migrating from one subtree tool to another. If we started fresh in |
josh rustc-pull: check that no new root commits get created A second root was a bad sign in Miri (judging from the description in #2583) and seems to be a [bad sign in RA](rust-lang/rust-analyzer#17025 (comment)). So let's add this to the sanity checks.
josh rustc-pull: check that no new root commits get created A second root was a bad sign in Miri (judging from the description in rust-lang/miri#2583) and seems to be a [bad sign in RA](rust-lang/rust-analyzer#17025 (comment)). So let's add this to the sanity checks.
|
Using a patched josh with josh-project/josh#1326, I have done a pull-and-push successfully |
josh rustc-pull: check that no new root commits get created A second root was a bad sign in Miri (judging from the description in rust-lang/miri#2583) and seems to be a [bad sign in RA](rust-lang/rust-analyzer#17025 (comment)). So let's add this to the sanity checks.
|
josh-project/josh#1329 landed, so a josh patch shouldn't be required any more (but it'd be good to re-test this with the latest josh master to make sure that the final PR that landed still works as intended). |
|
Oof, I wanted to give this another try today, but.. |
|
Is it a timeout issue? josh can take a bit to start so I sometimes had to increase the timeout in |
|
No, it actually shows up in the logs:
This is the same error as in #17025 (comment). |
No description provided.