bootstrap: Consolidate editor setup into ./x setup editor & add support for vim, emacs & helix #131075

mrkajetanp · 2024-09-30T21:44:49Z

Add support for automatically setting up the recommended
LSP config for Vim (coc-nvim), Emacs (eglot) and Helix.

Additionally, refactor setup.rs to make it easier to add support
for more editors in the future.

As suggested,
r? @jieyouxu

rustbot · 2024-09-30T21:44:59Z

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

mrkajetanp · 2024-10-01T06:51:30Z

Should be good now, I ended up with pretty substantial refactoring but I think it's cleaner this way. Returning editor-specific stuff from a method at runtime seems better than holding a ton of static variables on the other end of the file regardless of whether they actually end up being used or not.

jieyouxu

I briefly looked through this and it looks good to me in general, thanks! I'm not a bootstrap reviewer though so I'll hand it off to T-bootstrap reviewers.

src/bootstrap/src/core/build_steps/setup.rs

jieyouxu · 2024-10-01T09:17:00Z

r? bootstrap

src/etc/rust_analyzer_eglot.el

Add support for automatically setting up the recommended LSP config for Emacs. Additionally, refactor setup.rs to make it easier to add support for more editors in the future.

Kobzol

Left a few nits, but in terms of the bootstrap implementation, it looks fine to me.

I don't use or know any of these IDEs, so it's hard for me to validate the actual functionality though.

Onur has suggested that we might create a subcommand, e.g. x setup editor <editor-name>, given that we now support multiple editors.

Once this is merged, we should also update https://rustc-dev-guide.rust-lang.org/building/quickstart.html?highlight=vscode#quickstart and https://rustc-dev-guide.rust-lang.org/building/suggested.html#configuring-rust-analyzer-for-rustc.

src/bootstrap/src/core/build_steps/setup.rs

Kobzol · 2024-10-03T10:30:37Z

When running x setup, it would be nicer to ask the user which IDE they want to preconfigure (if any). After this PR, it will cycle through all four variants. This might be easier to do in combination with introducing a separate editor Step implementation.

Consolidate LSP setup for different editors into one `./x setup editor`.

mrkajetanp · 2024-10-03T23:14:12Z

Working as intended now. I don't think we can avoid having an Editor struct for the Step separate from EditorKind, looks like putting them together would require a larger rework of the logic given that it's not the same case as with Profile. We can call ./x setup [compiler|library|whatever] but not ./x setup profile while in this case we want to call ./x setup editor but not ./x setup vscode. I think it's probably fine as it's currently structured.

Kobzol

Thank you, I think this is much better.

@onur-ozkan Just to be sure, are you okay with the x setup editor approach?

If yes, this is ready to be merged I think.

Kobzol · 2024-10-05T09:38:46Z

src/bootstrap/src/core/build_steps/setup.rs

+        match EditorKind::prompt_user() {
+            Ok(editor_kind) => {
+                if let Some(editor_kind) = editor_kind {
+                    while !t!(create_editor_settings_maybe(config, editor_kind.clone())) {}


Nit: The logic would be simpler to follow if the loop was directly inside create_editor_settings_maybe, allowing to print the preview without having to call the function again.

But this was pre-existing, so it doesn't need to be changed in this PR.

onur-ozkan · 2024-10-05T18:12:46Z

@onur-ozkan Just to be sure, are you okay with the x setup editor approach?

Looks good to me, thanks for the ping.

Kobzol · 2024-10-05T18:16:18Z

@bors r+

bors · 2024-10-05T18:16:21Z

📌 Commit 456be10 has been approved by Kobzol

It is now in the queue for this repository.

Kobzol · 2024-10-05T18:16:57Z

@mrkajetanp Could you please also update the rustc-dev-guide? :) E.g. here.

mrkajetanp · 2024-10-05T18:24:33Z

Could you please also update the rustc-dev-guide? :)

Will do, no problem! Thanks for the reviews :)

bors · 2024-10-06T07:43:56Z

⌛ Testing commit 456be10 with merge 68301a6...

bors · 2024-10-06T10:26:31Z

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing 68301a6 to master...

rust-timer · 2024-10-06T12:00:51Z

Finished benchmarking commit (68301a6): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.4%	[0.4%, 0.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 2.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[2.8%, 3.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -2.8%, secondary -3.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.8%	[-2.8%, -2.8%]	1
Improvements ✅ (secondary)	-3.5%	[-3.5%, -3.5%]	1
All ❌✅ (primary)	-2.8%	[-2.8%, -2.8%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 774.957s -> 775.25s (0.04%)
Artifact size: 329.49 MiB -> 329.52 MiB (0.01%)

rustbot assigned jieyouxu Sep 30, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 30, 2024

mrkajetanp force-pushed the bootstrap-editors branch from f8c65f9 to ec46b41 Compare September 30, 2024 21:50

This comment has been minimized.

Sign in to view

mrkajetanp force-pushed the bootstrap-editors branch 3 times, most recently from fd097d0 to dea665f Compare October 1, 2024 06:35

jieyouxu approved these changes Oct 1, 2024

View reviewed changes

rustbot assigned Kobzol and unassigned jieyouxu Oct 1, 2024

mrkajetanp commented Oct 1, 2024

View reviewed changes

src/etc/rust_analyzer_eglot.el Outdated Show resolved Hide resolved

mrkajetanp added 3 commits October 1, 2024 12:59

bootstrap: Add support for ./x setup emacs

c53d4c7

Add support for automatically setting up the recommended LSP config for Emacs. Additionally, refactor setup.rs to make it easier to add support for more editors in the future.

bootstrap: Add support for ./x setup helix

dfc7e39

bootstrap: Add support for ./x setup vim

9b24fae

mrkajetanp force-pushed the bootstrap-editors branch from dea665f to 9b24fae Compare October 1, 2024 12:01

Kobzol reviewed Oct 3, 2024

View reviewed changes

src/bootstrap/src/core/build_steps/setup.rs Outdated Show resolved Hide resolved

src/bootstrap/src/core/build_steps/setup.rs Outdated Show resolved Hide resolved

bootstrap: Consolidate editor LSP setup

456be10

Consolidate LSP setup for different editors into one `./x setup editor`.

mrkajetanp changed the title ~~bootstrap: Add support for ./x setup [vim|emacs|helix]~~ bootstrap: Consolidate editor setup into ./x setup editor & add support for vim, emacs & helix Oct 3, 2024

Kobzol reviewed Oct 5, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 6, 2024

bors merged commit 68301a6 into rust-lang:master Oct 6, 2024
7 checks passed

rustbot added this to the 1.83.0 milestone Oct 6, 2024

bors mentioned this pull request Oct 6, 2024

Compiletest: Custom differ #131181

Merged

Kobzol mentioned this pull request Nov 18, 2024

Added project-specific Zed IDE settings #127793

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootstrap: Consolidate editor setup into ./x setup editor & add support for vim, emacs & helix #131075

bootstrap: Consolidate editor setup into ./x setup editor & add support for vim, emacs & helix #131075

mrkajetanp commented Sep 30, 2024

rustbot commented Sep 30, 2024

This comment has been minimized.

mrkajetanp commented Oct 1, 2024

jieyouxu left a comment •

edited

Loading

jieyouxu commented Oct 1, 2024

Kobzol left a comment

Kobzol commented Oct 3, 2024

mrkajetanp commented Oct 3, 2024

Kobzol left a comment

Kobzol Oct 5, 2024

onur-ozkan commented Oct 5, 2024

Kobzol commented Oct 5, 2024

bors commented Oct 5, 2024

Kobzol commented Oct 5, 2024

mrkajetanp commented Oct 5, 2024

bors commented Oct 6, 2024

bors commented Oct 6, 2024

rust-timer commented Oct 6, 2024

bootstrap: Consolidate editor setup into ./x setup editor & add support for vim, emacs & helix #131075

bootstrap: Consolidate editor setup into ./x setup editor & add support for vim, emacs & helix #131075

Conversation

mrkajetanp commented Sep 30, 2024

rustbot commented Sep 30, 2024

This comment has been minimized.

mrkajetanp commented Oct 1, 2024

jieyouxu left a comment • edited Loading

Choose a reason for hiding this comment

jieyouxu commented Oct 1, 2024

Kobzol left a comment

Choose a reason for hiding this comment

Kobzol commented Oct 3, 2024

mrkajetanp commented Oct 3, 2024

Kobzol left a comment

Choose a reason for hiding this comment

Kobzol Oct 5, 2024

Choose a reason for hiding this comment

onur-ozkan commented Oct 5, 2024

Kobzol commented Oct 5, 2024

bors commented Oct 5, 2024

Kobzol commented Oct 5, 2024

mrkajetanp commented Oct 5, 2024

bors commented Oct 6, 2024

bors commented Oct 6, 2024

rust-timer commented Oct 6, 2024

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

jieyouxu left a comment •

edited

Loading