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

Modify OS.isWindows to check platform before version #2407

Merged
merged 2 commits into from May 20, 2018

Conversation

alejzeis
Copy link

First time contributor checklist:

Contributor checklist:

Description

Previously OS.isWindows checked if the windows version matched the one required. This worked fine, except for the fact that it would end up comparing a linux kernel version to a windows version as it didn't check if the platform was Windows in the first place before.

This caused issues as it would throw an error when comparing with non-semver Linux kernels (such as Fedora). Tests would fail and notably the settings/preferences dialogs would fail to open. Now it checks if the current platform is Windows first, and if not, immediately returns false.

All tests pass now (previously on first checkout they failed on my system), and the preferences dialog successfully opens. No errors can be found in the console.

My OS is Fedora 28 x64.

Fixes: #2396

…ersion.

Previously OS.isWindows checked if the windows version matched the one required. This worked fine, except for the fact that it would end up comparing a linux kernel version to a windows version as it didn't check if the platform was Windows in the first place before.

This caused issues as it would throw an error when comparing with non-semver linux kernels (such as Fedora). Now it checks if the current platform is Windows first, and if not, immediately returns false.

Resolves: signalapp#2396
@scottnonnenberg-signal
Copy link
Contributor

Hey there - thanks for this change! What happens if you run yarn format? This project (via that command and editor integration) use a code auto-formatter called prettier, and it makes it easier on all of us, since we don't have to give PR feedback about code formatting. :0)

@alejzeis
Copy link
Author

Sorry about that, I ran yarn format and pushed the changes. I'm not sure if I'm supposed to squash the two commits so I just left them separate.

@scottnonnenberg-signal scottnonnenberg-signal merged commit 84759d8 into signalapp:development May 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Non-semver Linux kernel versions prevent startup
3 participants