Detect whether OS is Windows Server

[dlon]

We do not officially support Windows Server, but we could still distinguish between it and the desktop OSes. Currently, we incorrectly assume that the user is necessarily on the desktop version.

This code simply returns "Windows Server" and does not bother to detect the specific version/year. This is fine because we log the build number as well, and the main point is to distinguish between desktop and server.

---

This change is 

[linear]

DES-339 Include "Server" in problem report for Windows server

Currently the problem reports on Windows does not convey if the user is running the server or desktop version of Windows. The vast majority of users of course run desktop and no change should be made there. But if we detect that the user runs the server version it could be nice if the string "Server" was added somewhere in the platform string.

Should be possible to identify with the wProductType field we already have access to.

[faern]

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @dlon)

---

talpid-platform-metadata/src/windows.rs line 32 at r1 (raw file):

        .map(|version| version.windows_version_string())
        .unwrap_or("N/A".into());
    format!("Windows {}", version_string)

This is IMHO a bit harder to understand than the old version. Because now the reader has to check in more places to know if the prefix "Windows" is part of the string or not. This PR currently goes from hardcoding "Windows" in two places to having it in three places.

Personally I think I would have preferred to keep "Windows" out of windows_version_string and just have it return "Server" when it detects server. Not going to block the PR on this, but it's my suggestion for an improvement.

---

talpid-platform-metadata/src/windows.rs line 42 at r1 (raw file):

}

pub struct WindowsVersion {

Is there any reason this struct is public? I can't find any usage of it outside of this module. I'm asking because the currenly public facing method WindowsVersion::windows_version_string now changed format (The word "Windows" was added to the start) and I was unsure what parts of the app this would affect.

If this struct is indeed not used outside of this module, let's make it private to better show that.

[faern]

Reviewed 1 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dlon)

[dlon]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @faern)

---

talpid-platform-metadata/src/windows.rs line 32 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This is IMHO a bit harder to understand than the old version. Because now the reader has to check in more places to know if the prefix "Windows" is part of the string or not. This PR currently goes from hardcoding "Windows" in two places to having it in three places.

Personally I think I would have preferred to keep "Windows" out of windows_version_string and just have it return "Server" when it detects server. Not going to block the PR on this, but it's my suggestion for an improvement.

Done.

---

talpid-platform-metadata/src/windows.rs line 42 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Is there any reason this struct is public? I can't find any usage of it outside of this module. I'm asking because the currenly public facing method WindowsVersion::windows_version_string now changed format (The word "Windows" was added to the start) and I was unsure what parts of the app this would affect.

If this struct is indeed not used outside of this module, let's make it private to better show that.

Nope, we do not use it elsewhere. Done.

[faern]

Sweet! Overall way fewer changes and IMHO nicer code. This is perfect

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved