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

Improve RA version display #3247

Merged
merged 6 commits into from
Feb 21, 2020
Merged

Conversation

edwin0cheng
Copy link
Member

@edwin0cheng edwin0cheng commented Feb 20, 2020

There are 2 problems of current implementation for displaying current version of RA binary:

  1. If that binary is coming from built by source, the REV may not be updated somehow. (See discussion in Zuilp)

  2. We must go through the VSCode debugger console to see the output of console.log.

This PR implemented a new VSCode command "Show RA Version" to display the version, which fixed the first problem. And added some rerun-if flags in build.rs to fix the second one.

@edwin0cheng
Copy link
Member Author

PS: Thanks @bjorn3 for idea of git head rerun flags.

crates/rust-analyzer/build.rs Outdated Show resolved Hide resolved
@@ -108,6 +110,8 @@ function isBinaryAvailable(binaryPath: string): boolean {
console.log("Checked binary availablity via --version", res);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can stop logging to console if we add that command?

Copy link
Member

Choose a reason for hiding this comment

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

Keeping it leaves all information necessary for debugging in one place.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but I feel we should log less stuff to console, possibly in favor of the output channel.

Copy link
Member Author

@edwin0cheng edwin0cheng Feb 20, 2020

Choose a reason for hiding this comment

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

I don’t have any strong opinion about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Logging infrastructure is not yet designed well. @matklad made some proposals around that here. Let's leave it for the debugging purposes for now (it does help a lot)

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI just created an issue about logging #3250

@@ -8,6 +8,8 @@ import { BinarySource } from "./interfaces";
import { fetchArtifactReleaseInfo } from "./fetch_artifact_release_info";
import { downloadArtifact } from "./download_artifact";

export var ServerVersion = "";
Copy link
Contributor

@Veetaha Veetaha Feb 20, 2020

Choose a reason for hiding this comment

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

Hmm, var is deprecated.

Now we use the following analogies
Rust let ->TypeScript const
Rust let mut -> TypeScript let
Also variables in TypeScript are generally camelCase...

Anyway, I'd like to propose the following:
Let's make getServerVersion() public and add logic for obtaining the commit hash to command handler (to keep it simple).

export function serverVersion(): Cmd {
    return ctx => {
        const { serverSource } = ctx.config;
     
        if (!serverSource) { 
            /* no prebuilt binary for this platform and no explicit path to binary specified */
        }  
        
        if (serverSource.type === BinarySource.GithubRelease) {
             const releaseName = getServerVersion(serverSource.storage);
             ...
        }

        const serverPath = serverSource.type === BinarySource.ExplicitPath
            ? serverSource.path
            : path.join(serverSource.dir, serverSource.file);

        const commitHash = spawnSync(serverPath, ["--version"], { encoding: "utf8" }).stdout;
        ...
    };
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, var is deprecated.

Oh.. My main programming language in my current day job is C# 4.0 (Unity). It is so easy to mix up all different languages syntax.. T_T

@@ -108,6 +110,8 @@ function isBinaryAvailable(binaryPath: string): boolean {
console.log("Checked binary availablity via --version", res);
Copy link
Contributor

Choose a reason for hiding this comment

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

Logging infrastructure is not yet designed well. @matklad made some proposals around that here. Let's leave it for the debugging purposes for now (it does help a lot)

editors/code/src/installation/server.ts Outdated Show resolved Hide resolved
editors/code/src/commands/server_version.ts Outdated Show resolved Hide resolved
@matklad
Copy link
Member

matklad commented Feb 21, 2020

bors r+

@edwin0cheng
Copy link
Member Author

Seem like bors is stuck. Rebased.

bors retry

@bors
Copy link
Contributor

bors bot commented Feb 21, 2020

Build succeeded

  • Rust (macos-latest)
  • Rust (ubuntu-latest)
  • Rust (windows-latest)
  • TypeScript

@bors bors bot merged commit db1bbb1 into rust-lang:master Feb 21, 2020
@edwin0cheng edwin0cheng deleted the improve-show-version branch February 21, 2020 10:42
@matklad
Copy link
Member

matklad commented Feb 22, 2020

Also, big 👍 from me on solving the general problem of "how do we find which rust-analyzer is used by this user" :)

@Veetaha
Copy link
Contributor

Veetaha commented Feb 22, 2020

What about adding the release name to the message if the user utilizes prebuilt binaries?

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