Ignore the language server if the platform is not supported. #2351

ericsnowcurrently · 2018-08-07T21:00:54Z

(fixes #2245)

Title summarizes what is changing
Includes a news entry file (remember to thank yourself!)
Unit tests & code coverage are not adversely affected (within reason)
Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)
Dependencies are pinned (e.g. "1.2.3", not "^1.2.3")
package-lock.json has been regenerated if dependencies have changed

DonJayamanne · 2018-08-07T22:55:04Z

src/client/common/platform/platformService.ts

    }
    public get pathVariableName() {
        return this.isWindows ? WINDOWS_PATH_VARIABLE_NAME : NON_WINDOWS_PATH_VARIABLE_NAME;
    }
    public get virtualEnvBinName() {
        return this.isWindows ? 'scripts' : 'bin';
    }
+
+    // XXX Drop the following (in favor of osType).
+    public get isWindows(): boolean {


👌👌👌👌👌👌👌👏👏👏

I'll do it in a separate PR.

DonJayamanne · 2018-08-07T22:55:54Z

src/client/common/platform/platformService.ts

    }
-    public get isMac(): boolean {
-        return this._isMac;
+    public get osType(): OSType {


Why not just os() instead of osType?

DonJayamanne · 2018-08-07T22:57:01Z

src/client/common/platform/types.ts

@@ -9,6 +9,13 @@ export enum Architecture {
    x86 = 2,
    x64 = 3
 }
+export enum OSType {
+    Unsupported,


Unknown is better. This could be used on other contexts too.

good point

fixed

DonJayamanne · 2018-08-07T22:57:26Z

src/client/common/platform/platformService.ts

+
+    let distro = '';
+    let version = '';
+    const data = fs.readFileSync(filename, 'utf-8');


Please use IFileSystem
The logic in this class needs to be testable.

With exception handling

fixed (wasn't trivial)

DonJayamanne · 2018-08-07T23:00:41Z

src/client/common/platform/platformService.ts

+    }
+}
+
+function linux_info_from_file(filename: string = LINUX_INFO_FILE): LinuxInfo {


Functions are verbs, hence should be named as such. That's how it is in js and .net world.
Please change to get Linux....
Also please do not use underscores in method names. User Pascal casing.

E.g. a better name would be getLinuxInformationFromFile

Try avoiding abbreviations.

codecov · 2018-08-07T23:08:28Z

Codecov Report

Merging #2351 into master will increase coverage by 0.22%.
The diff coverage is 88.05%.

@@            Coverage Diff             @@
##           master    #2351      +/-   ##
==========================================
+ Coverage   75.38%   75.61%   +0.22%     
==========================================
  Files         309      310       +1     
  Lines       14331    14506     +175     
  Branches     2537     2567      +30     
==========================================
+ Hits        10804    10969     +165     
- Misses       3518     3529      +11     
+ Partials        9        8       -1

Flag	Coverage Δ
#MacOS	`74.39% <83.01%> (+0.19%)`	⬆️
#Windows	`74.52% <83.01%> (+0.22%)`	⬆️

Impacted Files	Coverage Δ
src/client/common/platform/types.ts	`100% <100%> (ø)`	⬆️
src/client/telemetry/constants.ts	`100% <100%> (ø)`	⬆️
src/client/common/platform/platformService.ts	`100% <100%> (+10.52%)`	⬆️
src/client/common/platform/osinfo.ts	`82.65% <82.65%> (ø)`
src/client/activation/activationService.ts	`95.83% <93.54%> (-1.9%)`	⬇️
src/client/common/utils.ts	`74.68% <0%> (-1.64%)`	⬇️
src/client/activation/languageServer.ts	`25.68% <0%> (-0.41%)`	⬇️
src/client/common/types.ts	`100% <0%> (ø)`	⬆️
...nt/unittests/common/services/testResultsService.ts	`92.85% <0%> (ø)`	⬆️
src/client/common/serviceRegistry.ts	`100% <0%> (ø)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6707eaa...ab4e333. Read the comment docs.

DonJayamanne · 2018-08-07T23:14:54Z

src/test/activation/activationService.unit.test.ts

+                const osType: OSType = values[0];
+                const osVer: string = values[1];
+                const engine: string = values[2];
+                test(`LS is ${engine === 'ls' ? '' : 'not '}supported (${osType} ${osVer})`, async () => {


Or you can:
const [osType, osVer, engine] = values
I'm certain that feel a little bit more pythonic too
(Please don't change, it's merely a suggestion).

MikhailArkhipov · 2018-08-09T00:19:19Z

src/client/activation/activationService.ts

-        const jedi = this.useJedi();
+        let jedi = this.useJedi();
+        if (!jedi && !isLSSupported(this.serviceContainer)) {
+            this.appShell.showWarningMessage('The Python Language Server is not supported on your platform.');


Might be worth opening https://github.com/dotnet/core/blob/master/release-notes/2.1/2.1-supported-os.md

Actually, it would be error. OR we should tell user that we are switching to Jedi explicitly.

MikhailArkhipov · 2018-08-09T18:55:16Z

src/client/activation/activationService.ts

 import { IConfigurationService, IDisposableRegistry, IOutputChannel, IPythonSettings } from '../common/types';
 import { IServiceContainer } from '../ioc/types';
 import { ExtensionActivators, IExtensionActivationService, IExtensionActivator } from './types';

 const jediEnabledSetting: keyof IPythonSettings = 'jediEnabled';
+const LS_MIN_OS_VERSIONS: Map<OSType, string> = new Map([
+    [OSType.OSX, '10.12.0']  // Sierra or higher


On MacOS you can simply check os.release() which gives you Darwin version and .NET supports Darwin 16+ (10.12+)

MikhailArkhipov · 2018-08-16T16:45:25Z

src/client/activation/activationService.ts

+    const platform = services.get<IPlatformService>(IPlatformService);
+    const minVer = LS_MIN_OS_VERSIONS[platform.osType];
+    if (minVer === undefined || minVer === null) {
+        return true;


Show something in the output that platform was not recognized? Or log telemetry?

MikhailArkhipov · 2018-08-16T16:45:57Z

src/client/activation/activationService.ts

-        const jedi = this.useJedi();
+        let jedi = this.useJedi();
+        if (!jedi && !isLSSupported(this.serviceContainer)) {
+            this.appShell.showWarningMessage('The Python Language Server is not supported on your platform.');


This also needs telemetry so we know how many platforms were not supported.

MikhailArkhipov · 2018-08-16T16:47:19Z

src/test/activation/activationService.unit.test.ts

+                    activationService.dispose();
+                    activator.verifyAll();
+                });
+            };
            test('Activatory must be activated', async () => {


What is acivatory?

No idea. That was already there.

DonJayamanne

Please remove stubs file, we should use what we have or other libraries, also we need to be consistent in how we do things (writing tests, coding styles, etc).
Makes it easier for others as well, including those trying to contribute, use standard/popular libraries unless there's a very good reason not to.

DonJayamanne · 2018-08-21T05:25:02Z

src/client/common/platform/fileSystem.ts

+     * @memberof FileSystem
+     */
+    public readFileSync(filePath: string): string {
+        return fs.readFileSync(filePath, 'utf8');


Please remove sync version of file io. Where possible we must use async code.

DonJayamanne · 2018-08-21T05:30:13Z

src/client/common/platform/platformService.ts

+        @inject(IFileSystem) filesystem?: IFileSystem
+    ) {
+        if (info) {
+            this.os = info;


We have three ways to populate the os property, why is this? Why do we need to pass it as an argument? Why can't we always determine it using osinfo.
It's complicated and wrong (encapsulation) to have three ways to do the exact same thing!

We have more code paths to test as well.

It's not upto the calling classes to provide the is information. Completely defeats the purpose of having this class. This class should be responsible to determine/extract the os information.
Please fix this before merging.

DonJayamanne · 2018-08-21T05:33:30Z

src/client/common/platform/osinfo.ts

+
+export function getOSInfo(
+    readFile: (string) => string = (filename) => {
+        return fs.readFileSync(filename, 'utf8');


This function is not testable due to the use of fs!
You'll need to ensure you have physical files, makes it more difficult to test, these fall into system tests, which is unnecessary for such a simple function.

I'm not sure I understand. In tests you pass in readFile. fs is only used as the default.

DonJayamanne · 2018-08-21T05:35:52Z

src/client/common/platform/types.ts

+}
+
+export class OSInfo {
+    constructor(


types.js should only contain interfaces/contracts, not implementations.
Please remove the classes from here.
Please fix this before merging

DonJayamanne · 2018-08-21T05:39:40Z

src/client/common/platform/osinfo.ts

+    return new OSInfo(osType, arch, version);
+}
+
+// Inspired in part by: https://github.com/juju/os


Isn't there's an npm package we can use instead?
Reduces maintenance headaches for something that's very generic.

I was trying to avoid adding a new dependency. :) I'll look for a suitable library.

DonJayamanne · 2018-08-21T05:54:43Z

src/test/stub.ts

+ *    }
+ *
+ *    const s = new MyStub();
+ *    mod.func = s.someFunc;  // monkey-patch


What's wrong with typemocks that already allows you to create mocked classes, this is a reinvention of a wheel here.
Please remove before merging.
I'm a strong believer if using existing libraries, that are well tested, and it makes it easier for others to use as well. E.g. if we need to support async meetings, you'll need to change code in this stubs framework, i.e. slippery slope of maintaining code that's already supported by other libraries.

ericsnowcurrently · 2018-08-22T18:34:50Z

I'm going to land the change with the stub left in. IMO, doing it with mocks instead is much harder to understand (and write). @DonJayamanne, we'll have a discussion about this when you get back. If we decide that mocks are the one true way then I'll switch this code to use mocks. :)

d3r3kk

Your solution looks fine to me, and I look forward to having the conversation re:stubs vs TypeMoq when @DonJayamanne returns.

One thing to consider, open an issue to discuss this type of change and label it as P0 for Sept milestone!

Everything addressed except for stub concerns.

For #2351

ericsnowcurrently added 7 commits August 7, 2018 10:04

Check for language server first.

fae8ece

Add OSType.

09a6fd1

Add PlatformService.osVersion().

8ff0c1b

Add osType and osVersion to IPlatformService.

35ed66c

Only use the language server is the platform is supported.

b127f1e

Fix the unit tests.

84093a1

Fix the min version.

906f793

DonJayamanne requested changes Aug 7, 2018

View reviewed changes

MikhailArkhipov reviewed Aug 9, 2018

View reviewed changes

ericsnowcurrently added 4 commits August 15, 2018 17:28

Add OSInfo.

c7d335a

Add other supported platforms.

1f6c0f5

Finish the linux support.

8e7ed9f

Unsupported -> Unknown

6ec1713

MikhailArkhipov reviewed Aug 16, 2018

View reviewed changes

ericsnowcurrently added 11 commits August 16, 2018 12:10

Add IFileSystem.readFileSync().

a982317

Move the OSInfo code to its own file.

5bc8d26

Pass getArch() to getOSInfo().

70d8ccf

LINUX_INFO_FILE -> LINUX_OS_RELEASE_FILE

e08ea46

Pass readFile() to linuxInfoFromFile().

655d1d5

Factor our readOSReleaseFile().

0483926

Use DI for the filesystem.

45e2ecd

Optionally pass in the OSInfo.

3c49da5

Add helpers to replace the PlatformService getters.

8d993ed

Use isWindows() directly where appropriate.

dd502f8

Make all function names verbs.

04d4e83

ericsnowcurrently added 3 commits August 20, 2018 13:37

Add Stub class.

e6f5554

Add tests for OSInfo.

ab4bb2a

Add tests for PlatformService.

cbb3d2d

ericsnowcurrently force-pushed the fix-2245-ls-unsupported-os branch from a837ae2 to cbb3d2d Compare August 20, 2018 21:27

ericsnowcurrently added 2 commits August 20, 2018 16:18

Fix Ubuntu versions.

3668f16

Add tests for OS version checking.

82d40a2

ericsnowcurrently changed the title ~~[WIP] Ignore the language server if the platform is not supported.~~ Ignore the language server if the platform is not supported. Aug 20, 2018

DonJayamanne previously requested changes Aug 21, 2018

View reviewed changes

ericsnowcurrently added 7 commits August 21, 2018 15:02

Add telemtry.

6651547

Test PlatformService only against local info.

cc2fb67

Move OSInfo out of types.ts.

72d663a

Initialize PlatformService.info lazily.

eb7e75c

Do no use Filesystem.readFileSync().

71983f7

Use a third-party package for linux release info.

a12b85c

Add a NEWS entry.

ab4e333

d3r3kk approved these changes Aug 22, 2018

View reviewed changes

ericsnowcurrently merged commit d031657 into microsoft:master Aug 22, 2018

ericsnowcurrently deleted the fix-2245-ls-unsupported-os branch August 22, 2018 22:27

DonJayamanne mentioned this pull request Sep 27, 2018

Changed logic used to determine validity of debugger executable #2708

Merged

3 tasks

DonJayamanne added a commit that referenced this pull request Sep 28, 2018

Changed logic used to determine validity of debugger executable (#2708)

3bceb5d

For #2351

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Ignore the language server if the platform is not supported. #2351

Ignore the language server if the platform is not supported. #2351

Conversation

ericsnowcurrently commented Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 7, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsnowcurrently Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

ericsnowcurrently commented Aug 22, 2018

d3r3kk left a comment

Choose a reason for hiding this comment

ericsnowcurrently commented Aug 7, 2018 •

edited

Loading

codecov bot commented Aug 7, 2018 •

edited

Loading

DonJayamanne left a comment •

edited

Loading

ericsnowcurrently Aug 21, 2018 •

edited

Loading

DonJayamanne Aug 21, 2018 •

edited

Loading