Skip to content

Conversation

@lnicola
Copy link
Member

@lnicola lnicola commented Dec 22, 2020

There's also a slight behavior change here: we no longer download our 64-binaries on 32-bit Darwin and Linux. We still do that on Windows, as I don't know how to detect 32-bit Node on 64 Windows.

But some people install the 32-bit Code by mistake, I doubt 32-bit Windows is that popular in the Rust crowd.

@lnicola lnicola changed the title Add support for downloading aarch64-apple-darwin binaries, change naming convention Add support for downloading aarch64-apple-darwin binaries Dec 22, 2020
@lnicola
Copy link
Member Author

lnicola commented Dec 22, 2020

CC @richiksc, @phungleson

@lnicola
Copy link
Member Author

lnicola commented Dec 22, 2020

Should we wait until tomorrow so that nightlies are available?

Actually, I don't even know how to test this. My installed-from-source extension never tries to download the LSP binary. I think there's some magic that happens with the published versions.

@matklad
Copy link
Contributor

matklad commented Dec 22, 2020

I mean, nightly will be available together with the extension. I think its fine mering this earlier, so that we have more time to fix build process until the next monday

@lnicola
Copy link
Member Author

lnicola commented Dec 22, 2020

bors r=matklad

@matklad
Copy link
Contributor

matklad commented Dec 22, 2020 via email

@bors
Copy link
Contributor

bors bot commented Dec 22, 2020

@bors bors bot merged commit 94f661c into rust-lang:master Dec 22, 2020
@lnicola lnicola deleted the arm64-download branch December 22, 2020 16:44
@richiksc
Copy link

@lnicola Sorry I wasn't able to respond to this before it got merged.

A couple things I wanted to comment:
When do you just outright drop support for 32-bit Node? The actual rust-analyzer binaries aren't built for 32-bit OSes anyway, and I think there should just be a simple if clause for arch == 'ia32' || arch == 'x32' that simply warns the user that rust-analyzer does not suppor 32-bit systems and that they should make sure they have the 64-bit version of Node installed.

Also, I feel like the code could be simplified, especially if you make the previous change.

let platform: string | undefined;
if (process.arch === "ia32" || process.arch == "x32") {
    showErrorMessage("Sorry, .... make sure you're not using 32-bit Node");
} else if (process.arch === "x64") {
    platform = "x86_64-";
    if (process.platform === "linux") platform += "unknown-linux-gnu";
    else if (process.platform === "win32") platform += "pc-windows-msvc";
    else if (process.platform === "darwin") platform += "apple-darwin";
}  else if (process.arch === "arm64") {
    platform = "aarch64-";
    if (process.platform === "darwin") platform += "apple-darwin";
}

Hmm, just realized that this change would break if the platform is x86_64 based but not linux, win32, or darwin, and it would break on all non-macOS ARM platforms. You could create a triple object with arch and platform attributes and make sure both of them aren't undefined.

@lnicola
Copy link
Member Author

lnicola commented Dec 22, 2020

When do you just outright drop support for 32-bit Node?

I don't know. We've had a user file an issue because they were running a 32-bit Code, but I don't know how common that is. On the other hand, the cost of keeping that code is quite low (and we don't need 64-bit Node anyway).

Also, I feel like the code could be simplified, especially if you make the previous change.

Yeah, I considered doing something like that (not with nested ifs, but rather adding arch and platform tuple parts), but I'm not sure it's worth the hassle. We'll revisit that when we add more targets.

@richiksc
Copy link

@lnicola Alright, makes sense to wait until more targets are added. I understand people running 32-bit Code.

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.

3 participants