Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

update racer/rustfmt to complete rustc-ap* bump #1680

Closed

Conversation

@@ -193,7 +193,8 @@ fn random_file() -> Result<(File, PathBuf), Error> {

fn gen_config_file(config: &Config) -> Result<(File, PathBuf), Error> {
let (mut file, path) = random_file()?;
let toml = config.all_options().to_toml().map_err(Error::ConfigTomlOutput)?;
let toml =
config.all_options().to_toml().map_err(|e| Error::ConfigTomlOutput(e.to_string()))?;
Copy link
Member Author

Choose a reason for hiding this comment

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

@topecongiro - this means that we would technically have a breaking API change in rustfmt 1.4.18 on to_toml() (the error value is now ToTomlError instead of String) due to the backporting of rust-lang/rustfmt#3948. I actually forgot that this was public in the rustfmt lib and consumed by rls.

I've opened this as-is in case you and @Xanewok want to accept this change and proceed in order to fix the broken toolstates. Otherwise we'd probably want to yank v.14.18 of rustfmt, and then either publish v1.4.19 that still has the parser bug users keep reporting, or try to reimplement a fix using the now-deprecated failure

@calebcartwright
Copy link
Member Author

Also had to tweak the CI config a bit to include the llvm-tools-preview component and ensure that CFG_RELEASE and CFG_RELEASE_CHANNEL were flowing through (they're defined is vars in the job config but I was still getting errors like the one shown below without setting them inline in the test jobs)

https://dev.azure.com/rust-lang/rls/_build/results?buildId=32077&view=logs&j=7fcbd5f9-4927-5bd9-bd50-f9f24db86f8d&t=30fe560a-7e17-5e89-0250-d3688963a3cf

error: environment variable `CFG_RELEASE_CHANNEL` not defined
   --> /home/vsts/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/rustc-ap-rustc_attr-664.0.0/builtin.rs:655:27
    |
655 |             let channel = env!("CFG_RELEASE_CHANNEL");
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: environment variable `CFG_RELEASE` not defined
   --> /home/vsts/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/rustc-ap-rustc_attr-664.0.0/builtin.rs:657:48
    |
657 |             let rustc_version = Version::parse(env!("CFG_RELEASE")).unwrap();
    |                                                ^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 2 previous errors

error: could not compile `rustc-ap-rustc_attr`.

@@ -1,6 +1,9 @@
jobs:
# Check formatting
- job: ${{ parameters.name }}
variables:
cfgRelease: ''
cfgReleaseChannel: ${{ parameters.rust }}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think RLS will build on stable/beta in the nearest future so I think we should just globally set these variables for simplicity and call it a day

@bors
Copy link
Contributor

bors commented Jun 19, 2020

☔ The latest upstream changes (presumably fb46b91) made this pull request unmergeable. Please resolve the merge conflicts.

@Xanewok
Copy link
Member

Xanewok commented Jun 19, 2020

@calebcartwright sorry for the delay. I hope you don't mind that I merged the previous branch I had prepared (https://github.com/rust-lang/rls/tree/rustc-ap-664) so that we don't need to change back the envs in the CI config.

From what I've seen there was yet another failure on master wrt Extend changes and I believe we still need to do the rustc-ap-* upgrade dance...

Since the relevant changes has been merged already, I'll close this now.

@Xanewok Xanewok closed this Jun 19, 2020
@calebcartwright
Copy link
Member Author

@calebcartwright sorry for the delay.

No worries @Xanewok ! I probably should've checked with you or perused the branches more thoroughly first to make sure you weren't working on it already 😄

From what I've seen there was yet another failure on master wrt Extend changes and I believe we still need to do the rustc-ap-* upgrade dance...

I'm not sure I follow 🤔 racer v2.1.35 and rustfmt 1.4.18 (looks like on rls master rustfmt is still 1.4.16) have already been updated and should be good to go. Or are you saying there's additional breaking upstream rustc changes and we need to start the dance all over?

@Xanewok
Copy link
Member

Xanewok commented Jun 19, 2020

(...) Or are you saying there's additional breaking upstream rustc changes (...)

So I've tried to test the update on master Rust with this patch applied and bumps to RLS/Rustfmt and I found that there was some additional breakage which led me to postpone the update in the first place... but I did try it again just now and it seemed to work? Not sure if I did something wrong the first time around or is there something that I didn't do correctly now 🤷

In any case, I've submitted rust-lang/rust#73506; let's see if the tools job is happy and if so we can r+ and hopefully finally unbreak the toolstate.

@calebcartwright
Copy link
Member Author

Gotcha, thank you! And now we wait 🤞

@calebcartwright calebcartwright deleted the rustc-ap-bump branch October 4, 2020 22:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants