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

Move rust-analyzer from a git submodule to a git subtree #12815

Closed
fasterthanlime opened this issue Jul 19, 2022 · 6 comments · Fixed by rust-lang/rust#99603
Closed

Move rust-analyzer from a git submodule to a git subtree #12815

fasterthanlime opened this issue Jul 19, 2022 · 6 comments · Fixed by rust-lang/rust#99603

Comments

@fasterthanlime
Copy link
Contributor

fasterthanlime commented Jul 19, 2022

(Related to: long-term proc macro support plan)

Before (submodule)

RA is a git submodule in src/tools. When a "sync" happens, rust-lang/rust is updated to point to a different commit (example).

The changes suggested in #12803 (use the sysroot proc_macro crate) make RA/rustc incompatibilities more visible in that the submodule could then stop building.

Fixing that would involve two PRs:

  • One to rust-analyzer, fixing proc-macro-srv to follow proc_macro bridge changes
  • One to rust, bumping rust-analyzer

If the sync cycle isn't changed, this means the rust-analyzer RA component could be missing for up to a week, or even more, if the bridge changes haven't been followed in the rust-analyzer repo.

After (subtree)

Just like clippy, rust-analyzer would be a subtree (also in src/tools).

Feature development / bug fixes to RA happen as usual in the rust-lang/rust-analyzer repository. These can be synced to rust-lang/rust whenever convenient (clippy->rust example PR).

Changes to the proc_macro bridge are followed in the rust-lang/rust repository. These can be synced to rust-lang/rust-analyzer whenever convenient (rust->clippy example commit)

proc-macro-srv never breaks again (in the rustup component version of rust-analyzer), because it's immediately fixed as part of any PRs to rust-lang/rust.

Why does this matter?

A PR was just merged to "add support for the 1.64 ABI", but there's no such thing as "the 1.64 ABI", and it's going to be instabroken, see #12806.

Prior art

Similar discussion for rustfmt: rust-lang/rust#82385 (converted to a subtree in May 2021)


Tagging:

@fasterthanlime
Copy link
Contributor Author

fasterthanlime commented Jul 19, 2022

Proposed course of action:

@lnicola
Copy link
Member

lnicola commented Jul 19, 2022

I haven't used subtrees yet, so I don't know how involved the switch would be, but I don't like git submodules very much. So this seems like a good idea, assuming the rust-lang/rust contributors are fine with doing the extra work for RA.

But will all breaking changes show up on CI? I'm not sure if the RA tests are being run in rust-lang/rust.

@fasterthanlime
Copy link
Contributor Author

But will all breaking changes show up on CI? I'm not sure if the RA tests are being run in rust-lang/rust.

@cuviper has done some work re: that in rust-lang/rust#99444, see also #12803 (comment)

The intent was to track rust-analyzer status in rust-toolstate, but I believe it could be repurposed to run the proc-macro-srv tests on rust CI, blocking PR merges until they pass. (More tests could be run, but those are the important ones imho)

@fasterthanlime
Copy link
Contributor Author

fasterthanlime commented Jul 19, 2022

I haven't used subtrees yet, so I don't know how involved the switch would be, but I don't like git submodules very much.

The rustc dev guide has a section on git subtree: https://rustc-dev-guide.rust-lang.org/contributing.html#external-dependencies-subtree

@oli-obk told me to quite literally "send y'all to clippy folks" to ask any questions you might have, they've been doing this for a while.

@fasterthanlime
Copy link
Contributor Author

So this seems like a good idea, assuming the rust-lang/rust contributors are fine with doing the extra work for RA.

@mystor seemed okay with it "provided it's relatively easy". I don't know about other proc_macro bridge maintainers.

I've done it once myself to get an idea of how much work it is (and @jonas-schievink can confirm/deny). It's mostly:

  • Removing functions that were removed from the bridge interface (in the various traits like FreeFunctions, Span, MultiSpan etc.)
  • Adding functions that were added to the bridge interface (a lot of things in proc-macro-srv are stubs right now, so this is relatively easy).
  • Following type/struct shape changes, see this diff for the recent move from Group being an assoc type to it being a struct.

@fasterthanlime
Copy link
Contributor Author

For research purposes, I opened a draft PR on which rust-analyzer is a git subtree and ./x.py test rust-analyzer runs RA tests:

It shows how the move would happen. All tests pass through ./x.py test rust-analyzer except for tidy::check_merge_commits — it might not make sense to run that one from the rust copy of rust-analyzer.

I also found clippy's actual git subtree guide, which was immensely helpful (cc @lnicola):

bors added a commit to rust-lang-ci/rust that referenced this issue Jul 25, 2022
Convert rust-analyzer to an in-tree tool

This re-adds `rust-lang/rust-analyzer` as a git subtree rather than a submodule.

Closes rust-lang/rust-analyzer#12815.

Prior attempt (research PR): rust-lang#99465

  * [x] Remove submodule: `git rm -f src/tools/rust-analyzer`
  * [x] Add subtree: `git subtree add -P src/tools/rust-analyzer https://github.com/rust-lang/rust-analyzer.git master`
  * [x] Move to `SourceType::InTree`,
  * [x] Enable `rust-analyzer/in-rust-tree` feature when built through `x.py`
  * [x] Add 'check' step
  * [x] Add 'test' step

With this PR, rust-analyzer becomes an "in-tree" tool. Syncs can happen in both directions, see [clippy's relevant book section](https://doc.rust-lang.org/nightly/clippy/development/infrastructure/sync.html).

Making sure `proc-macro-srv` doesn't break when the proc_macro bridge changes effectively becomes the responsibility of `rust-lang/rust` contributors. These days, that's mostly `@mystor,` who has been consulted throughout the process. I'm also making myself available in case there's questions / work needed that nobody else signed up for.

This doesn't change rust-analyzer's release cycle. After this PR is merged and the next nightly goes out, one can point `rust-analyzer.procMacro.server` to the rustup-provided `rust-analyzer` binary. Changes to improve the situation further (auto-discovery/install of the rust-analyzer component) will happen in `rust-lang/rust-analyzer` and be synced here eventually.
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 a pull request may close this issue.

2 participants