Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Added a check to prevent installing Delve in most unsupported platforms. #2195

Merged
merged 1 commit into from
Jan 23, 2019
Merged

Added a check to prevent installing Delve in most unsupported platforms. #2195

merged 1 commit into from
Jan 23, 2019

Conversation

aggressivepixels
Copy link
Contributor

@aggressivepixels aggressivepixels commented Dec 18, 2018

This will prevent Delve from being installed in 32-bit machines (e.g. arm, i686) where is not supported.
The mips, mipsel, s390 and s390x entries are there because Node defines them as possible values and it's not clear if they are 64-bit or not, so the extension will try to install it anyway.
This closes #2191.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

@aggressivepixels Looks like this is your first PR to this project, Thanks & Welcome!

@ramya-rao-a
Copy link
Contributor

@jhendrixMSFT The current possible values for process.arch are: 'arm', 'arm64', 'ia32', 'mips', 'mipsel', 'ppc', 'ppc64', 's390', 's390x', 'x32', and 'x64'.

See https://nodejs.org/api/process.html#process_process_arch

I am not familiar with 'mips', 'mipsel', 's390', 's390x'.
Any idea if it is useful to include these as supported platforms for delve?

@jhendrixMSFT
Copy link
Member

s390x and ppc64 are natively supported by Go (i.e. there are binary packages for it) so we should probably include those two.

@ramya-rao-a
Copy link
Contributor

s390x and ppc64 are natively supported by Go (i.e. there are binary packages for it) so we should probably include those two.

But that doesnt necessarily mean delve is supported too right?

@jhendrixMSFT
Copy link
Member

jhendrixMSFT commented Jan 22, 2019

I couldn't find any documentation about it but given that there are Linux distributions (e.g. Ubuntu) that support these architectures it seems likely that delve would work there too. That said, these architectures are pretty uncommon so it's probably fine to omit them (we can always add them later if it's an issue).

@aggressivepixels
Copy link
Contributor Author

Sorry about the inactivity, I don't have good internet access right now.
Anyway, I couldn't find anything about which platforms are supported by Delve, the only thing I found was that it doesn't work on 32-bit machines. Those other platforms are pretty uncommon and also there are no official Code builds for them AFAIK, so this should work well for now, and just like @jhendrixMSFT said, we can always add them later if it's an issue.

@ramya-rao-a ramya-rao-a merged commit a628a16 into microsoft:master Jan 23, 2019
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.

The extension tries to install Delve in 32-bit machines (and that's not supported)
3 participants