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.

Sign up for GitHub

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

Jump to bottom

Allow automatically creating vscode settings.json with x setup #107757

Merged
merged 1 commit into from
Feb 8, 2023
Merged

Allow automatically creating vscode settings.json with x setup #107757

merged 1 commit into from
Feb 8, 2023

Conversation

clubby789
Copy link
Contributor

Closes #107703

@rustbot
Copy link
Collaborator

rustbot commented Feb 7, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot 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 Feb 7, 2023
jyn514
jyn514 reviewed Feb 7, 2023
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

😍

src/bootstrap/setup.rs Outdated Show resolved Hide resolved
src/bootstrap/setup.rs Outdated Show resolved Hide resolved
SkiFire13
SkiFire13 reviewed Feb 7, 2023
View reviewed changes
src/bootstrap/setup.rs Outdated Show resolved Hide resolved
jyn514
jyn514 reviewed Feb 7, 2023
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me :) I would like to change https://rustc-dev-guide.rust-lang.org/building/suggested.html#configuring-rust-analyzer-for-rustc to point here instead of telling people to copy-paste the config - do you mind making a PR with that change? Ideally it would also point people to src/etc/vscode_settings.json.

I mentioned versioning the config in the original issue - is that something you're interested in working on? It's ok if the answer is no :) I think we can add it in a backwards-compatible way if we go by the hash of the settings.json file instead of using stamp files.

src/bootstrap/setup.rs Outdated Show resolved Hide resolved
src/etc/vscode_settings.json Show resolved Hide resolved
@clubby789
Copy link
Contributor Author

clubby789 commented Feb 7, 2023 •
edited
Loading

What would be best for the versioning? Maybe this:

  1. Store hash of current file as is Actually we can just hash the bundled settings.json, makes updating it easier
  2. Skip the prompt entirely if settings.json exists and has a matching hash
  3. If it exists and the hash does not match, print a note that the file is outdated
  4. Continue with the current logic in the PR (maybe we can overwrite outdated settings.json, copying the current one to a backup)

And I'm happy to make a PR to the dev guide once this one is ready.

@jyn514
Copy link
Member

jyn514 commented Feb 7, 2023

Store hash of current file as is Actually we can just hash the bundled settings.json, makes updating it easier

I think we need a hash of all historical versions if we want to be able to update old versions without prompting. Otherwise we can't tell apart user-defined configs from old versions of managed configs. That said we don't need a list of hashes right away because for now we can just hash src/etc/vscode_settings.json.

Everything else you said sounds great :)

@clubby789
Copy link
Contributor Author

clubby789 commented Feb 7, 2023 •
edited
Loading

I decided to add a list of a single hash initially so we can make sure settings.json isn't accidentally updated without adding a new hash.

@clubby789 clubby789 mentioned this pull request Feb 7, 2023
Merged
jyn514
jyn514 reviewed Feb 7, 2023
View reviewed changes
src/bootstrap/setup.rs Outdated Show resolved Hide resolved
src/bootstrap/setup.rs Outdated Show resolved Hide resolved
src/bootstrap/setup.rs Show resolved Hide resolved
src/bootstrap/setup.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 assigned jyn514 and unassigned Mark-Simulacrum Feb 7, 2023
@jyn514 jyn514 changed the title Allow automatically creating vscode settings.json from bootstrap Allow automatically creating vscode settings.json with x setup Feb 7, 2023
@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2023
jyn514
jyn514 approved these changes Feb 7, 2023
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

this is great, thanks! r=me with the last nit fixed :)

src/bootstrap/setup.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Feb 7, 2023

@bors r+

thank you!!!

@bors
Copy link
Contributor

bors commented Feb 7, 2023

📌 Commit eb18293 has been approved by jyn514

It is now in the queue for this repository.

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 7, 2023
@RossSmyth
Copy link
Contributor

:ferrisHmm: related
rust-lang/rustc-dev-guide#1545

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 8, 2023
@matthiaskrgr
Rollup merge of rust-lang#107757 - clubby789:setup-settings-json, r=j…
608cdc2 
…yn514

Allow automatically creating vscode `settings.json` with `x setup`

Closes rust-lang#107703
This was referenced Feb 8, 2023
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2023
@bors
Auto merge of rust-lang#107788 - matthiaskrgr:rollup-mw10sli, r=matth…
6eb9f2d 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#107656 (Bump rust-installer)
 - rust-lang#107757 (Allow automatically creating vscode `settings.json` with `x setup`)
 - rust-lang#107769 (Rename `PointerSized` to `PointerLike`)
 - rust-lang#107770 (rustdoc: use a newline instead of `<br>` to format code headers)
 - rust-lang#107771 (Tweak ICE message)
 - rust-lang#107773 (Clearly signal purpose of the yaml template)
 - rust-lang#107776 (Docs: Fix format of headings in String::reserve)
 - rust-lang#107779 (Remove astconv usage in diagnostic)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@albertlarsan68
Copy link
Member

Is there a check somewhere that verifies that the hash is correctly updated?

@bors bors merged commit b16a321 into rust-lang:master Feb 8, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 8, 2023
@clubby789
Copy link
Contributor Author

@albertlarsan68 src/bootstrap/setup/tests.rs tests the last hash matches the current bundled file.

@jyn514 jyn514 mentioned this pull request Feb 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SkiFire13 SkiFire13 SkiFire13 left review comments

@jyn514 jyn514 jyn514 approved these changes

Assignees

@jyn514 jyn514

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.

Create .vscode directory in x.py setup
8 participants
@clubby789 @rustbot @jyn514 @bors @RossSmyth @albertlarsan68 @SkiFire13 @Mark-Simulacrum