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

Update to rustc changes #5566

Merged
merged 2 commits into from
May 6, 2020
Merged

Update to rustc changes #5566

merged 2 commits into from
May 6, 2020

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 4, 2020

changelog: none

So, turns out git subtree push dies in various interesting ways, but the source cause is that the rustc repo looks like

--- A --- B --- C ---
     \--- D ---/

where B is the commit where I added clippy to rustc and D is an arbitrary other PR and C is the master branch (or an earlier commit in it). When we now do git subtree push, it doesn't stop looking for things to merge at B as it needs to look at D, too, but then the bad thing happens, and it doesn't stop at A either, and just goes on looking at the entire history of rustc in a recursive bash script. That recursion then quickly runs into a stack overflow. While we can increase the stack size via ulimit -s 60000, that just means I was waiting for 30 minutes looking at git subtree push counting up the number of commits it has looked at. I aborted that, as a process that needs 30 mins for a push is not reasonable.

This PR cheats by just doing a cp -r ../rustc/src/tools/clippy/* . inside my clippy checkout and committing all changes. I'm working on getting us a better workflow, but until then, this workaround will work nicely. Note that this requires a git subrepo pull to have occurred in the rustc checkout. It's not necessary to merge that pull in order to update clippy, it's just necessary in order to not revert code in the clippy repo that hasn't been synced yet to the rustc repo.

@matthiaskrgr
Copy link
Member

@eddyb
Copy link
Member

eddyb commented May 4, 2020

This method loses history, doesn't it? So we should avoid merging this PR until we can fix git subtree push.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 4, 2020

This method loses history, doesn't it?

Yes, which is not worse than what we've had with submodules. There we've also just had a single PR where someone got clippy working again. The only difference is now that the work was done in the rustc repo and we just squashed that work.

So we should avoid merging this PR until we can fix git subtree push.

My research has come to the conclusion that it's unfixable unless we also include all the synthetic commits created for the clippy repo in the rustc master branch. So a git subrepo push would not just leave a commit in rustc but also leave all the synthetic commits there. Additionally it would make synchronizing a huge pain, because we'd have to be super careful around rebases and everything.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 4, 2020

hmm that's odd about the failing tests. I wonder if these tests are also failing in rustc's CI

@flip1995
Copy link
Member

flip1995 commented May 4, 2020

It may be fixable: #5565 (comment)

But the fix depends on a PR to git to get completed and merged.

@flip1995 flip1995 mentioned this pull request May 4, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented May 5, 2020

If we are ok with losing history when syncing from rustc, I think we should just create some simple tooling for ourselves that just copies over the current state.

If we do that (and start by merging this PR), then we should merge this PR back to rustc soonish and fix rustc CI so it actually blocks on clippy's test suite passing, too.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with merging this and make this the process until we figure out how to move on with subtree, but let's discuss this in #5565 and/or on Discord.

@flip1995
Copy link
Member

flip1995 commented May 6, 2020

Let's merge this to move things forward

@bors r+

@bors
Copy link
Contributor

bors commented May 6, 2020

📌 Commit 780a63a has been approved by flip1995

@bors
Copy link
Contributor

bors commented May 6, 2020

⌛ Testing commit 780a63a with merge b63868d...

@bors
Copy link
Contributor

bors commented May 6, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing b63868d to master...

@bors bors merged commit b63868d into rust-lang:master May 6, 2020
@oli-obk oli-obk deleted the sync-from-rustc branch May 8, 2020 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants