Warn when files could override Python modules #10787

DonJayamanne · 2022-07-13T01:41:07Z

DonJayamanne · 2022-07-13T01:46:12Z

src/extension.node.ts

@@ -169,7 +170,9 @@ async function activateUnsafe(

        const [serviceManager, serviceContainer] = initializeGlobals(context);
        activatedServiceContainer = serviceContainer;
-        initializeTelemetryGlobals(serviceContainer);
+        initializeTelemetryGlobals((interpreter) =>


Just pass a callback instead of passing the entire interface, as we dont want the telemetry layer to have any dependencies on interpreter code.

DonJayamanne · 2022-07-13T01:50:06Z

src/platform/telemetry/telemetry.ts

-let globalContainer: IServiceContainer | undefined = undefined;
-
-export function initializeGlobals(serviceContainer: IServiceContainer) {
-    globalContainer = serviceContainer;


We no longer need to inject the entire service container, just a callback is sufficient.

codecov-commenter · 2022-07-13T02:38:50Z

Codecov Report

Merging #10787 (12d9f03) into main (dd892b5) will increase coverage by 0%.
The diff coverage is 75%.

@@           Coverage Diff           @@
##            main   #10787    +/-   ##
=======================================
  Coverage     63%      63%            
=======================================
  Files        476      481     +5     
  Lines      33058    33371   +313     
  Branches    5391     5437    +46     
=======================================
+ Hits       20875    21087   +212     
- Misses     10185    10255    +70     
- Partials    1998     2029    +31

Impacted Files	Coverage Δ
src/kernels/errors/types.ts	`100% <ø> (ø)`
.../notebooks/controllers/vscodeNotebookController.ts	`79% <ø> (ø)`
src/notebooks/notebookCommandListener.ts	`52% <ø> (ø)`
src/telemetry.ts	`100% <ø> (ø)`
src/platform/errors/errorUtils.ts	`72% <48%> (-4%)`	⬇️
src/kernels/errors/kernelErrorHandler.node.ts	`50% <50%> (ø)`
src/kernels/errors/kernelErrorHandler.ts	`61% <60%> (-2%)`	⬇️
src/platform/common/utils/localize.ts	`75% <64%> (-1%)`	⬇️
...ne/diagnostics/reservedFileNameDiagnostics.node.ts	`71% <71%> (ø)`
...c/platform/interpreter/interpreterPackages.node.ts	`80% <75%> (ø)`
... and 35 more

DonJayamanne · 2022-07-14T04:01:08Z

src/extension.node.ts

@@ -169,7 +170,9 @@ async function activateUnsafe(

        const [serviceManager, serviceContainer] = initializeGlobals(context);
        activatedServiceContainer = serviceContainer;
-        initializeTelemetryGlobals(serviceContainer);
+        initializeTelemetryGlobals((interpreter) =>
+            serviceContainer.get<IInterpreterPackages>(IInterpreterPackages).getPackageVersions(interpreter)


Moved IInterpreterPackages from telemetry folder into Interprter folder, and passed just a callback into interpreters layer, we don't need everything, a callback is sufficient.

DonJayamanne · 2022-07-14T04:02:39Z

src/kernels/installer/productInstaller.node.ts

@@ -29,13 +29,13 @@ import {
 import { noop } from '../../platform/common/utils/misc';
 import { IServiceContainer } from '../../platform/ioc/types';
 import { sendTelemetryEvent, Telemetry } from '../../telemetry';
-import { InterpreterPackages } from '../../platform/telemetry/interpreterPackages.node';
+import { InterpreterPackages } from '../../platform/interpreter/interpreterPackages.node';


Fixed dependency issues, installation code does not need to depend on telemetry for things like getting packages

DonJayamanne · 2022-07-14T04:05:24Z

src/platform/interpreter/interpreterPackages.web.ts

 import { PythonEnvironment } from '../pythonEnvironments/info';
-import { IInterpreterPackages } from '../../telemetry';
+import { IInterpreterPackages } from './types';


Moved out of telemetry into interpreter.

DonJayamanne · 2022-07-14T04:05:43Z

src/platform/interpreter/types.ts

+}
+
+export const IInterpreterPackages = Symbol('IInterpreterPackages');
+export interface IInterpreterPackages {


Moved from telemetry folder

I need to reference this from the new code, hence moved this from telemtry into Interrpeter & ensured telemetry doesnt depend on this.

DonJayamanne · 2022-07-14T04:11:21Z

src/platform/serviceRegistry.web.ts

@@ -50,6 +54,15 @@ export function registerTypes(serviceManager: IServiceManager) {
        KernelProgressReporter
    );

+    serviceManager.addSingleton<IInterpreterPackages>(IInterpreterPackages, InterpreterPackages);
+    serviceManager.addSingleton<IExtensionSyncActivationService>(


These things that have to do with interpeters is now in interpeters folder and no longer in telemetry folder.

DonJayamanne · 2022-07-14T04:11:55Z

src/platform/telemetry/serviceRegistry.node.ts

@@ -1,23 +0,0 @@
-// Copyright (c) Microsoft Corporation. All rights reserved.


Yay (i think), service registrations in telemetry folder anymore

DonJayamanne · 2022-07-14T04:13:09Z

src/telemetry.ts

@@ -106,13 +105,6 @@ export type ResourceSpecificTelemetryProperties = Partial<{
    kernelLiveCount: number;
 }>;

-export const IInterpreterPackages = Symbol('IInterpreterPackages');
-export interface IInterpreterPackages {


Moved out of here

src/platform/errors/errorUtils.ts

src/platform/interpreter/reservedNamedProvider.node.ts

src/standalone/diagnostics/reservedFileNameDiagnostics.node.ts

IanMatthewHuff

Cool this will be great for customers (still want Python to own this, but that can always come later). One minor question was on the listPackages possibly guarding against python extension install internally.

src/kernels/errors/kernelErrorHandler.web.ts

src/platform/interpreter/reservedNamedProvider.node.ts

DonJayamanne commented Jul 13, 2022

View reviewed changes

DonJayamanne commented Jul 14, 2022

View reviewed changes

DonJayamanne marked this pull request as ready for review July 14, 2022 04:15

DonJayamanne requested a review from a team as a code owner July 14, 2022 04:15

DonJayamanne force-pushed the issue7538Updated branch from 50c2319 to 12d9f03 Compare July 14, 2022 04:40

IanMatthewHuff reviewed Jul 14, 2022

View reviewed changes

src/platform/errors/errorUtils.ts Outdated Show resolved Hide resolved

IanMatthewHuff reviewed Jul 14, 2022

View reviewed changes

src/platform/interpreter/reservedNamedProvider.node.ts Outdated Show resolved Hide resolved

IanMatthewHuff reviewed Jul 14, 2022

View reviewed changes

src/standalone/diagnostics/reservedFileNameDiagnostics.node.ts Show resolved Hide resolved

IanMatthewHuff reviewed Jul 14, 2022

View reviewed changes

src/standalone/diagnostics/reservedFileNameDiagnostics.node.ts Show resolved Hide resolved

IanMatthewHuff approved these changes Jul 14, 2022

View reviewed changes

DonJayamanne force-pushed the issue7538Updated branch from 12d9f03 to 0a1a16d Compare July 14, 2022 16:36

rchiodo reviewed Jul 14, 2022

View reviewed changes

src/kernels/errors/kernelErrorHandler.web.ts Show resolved Hide resolved

rchiodo reviewed Jul 14, 2022

View reviewed changes

src/platform/interpreter/reservedNamedProvider.node.ts Show resolved Hide resolved

rchiodo approved these changes Jul 14, 2022

View reviewed changes

DonJayamanne added 8 commits July 15, 2022 03:28

Misc

3e030fd

Fixes

8b63a24

Changes

873a200

misc

ea4d303

Add tests

bb02afc

Fixes

5b0db64

Misc

bff8c98

Fix tests

38df452

DonJayamanne added 9 commits July 15, 2022 03:28

Misc

7b989cd

Remove console logging

22d60bd

More fixes

267fc95

Added tests

c3c9a4a

Misc

641afe1

Move files

e861778

More tests

3e2bdc4

Misc

ec23f76

Address code review comments

1d1360a

DonJayamanne force-pushed the issue7538Updated branch from 0a1a16d to 1d1360a Compare July 14, 2022 17:29

DonJayamanne merged commit 4d1a5c7 into main Jul 14, 2022

DonJayamanne deleted the issue7538Updated branch July 14, 2022 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when files could override Python modules #10787

Warn when files could override Python modules #10787

DonJayamanne commented Jul 13, 2022

DonJayamanne Jul 13, 2022

DonJayamanne Jul 13, 2022

codecov-commenter commented Jul 13, 2022 •

edited

Loading

DonJayamanne Jul 14, 2022

DonJayamanne Jul 14, 2022

DonJayamanne Jul 14, 2022

DonJayamanne Jul 14, 2022

DonJayamanne Jul 14, 2022

DonJayamanne Jul 14, 2022

DonJayamanne Jul 14, 2022

DonJayamanne Jul 14, 2022

IanMatthewHuff left a comment

		@@ -1,23 +0,0 @@
		// Copyright (c) Microsoft Corporation. All rights reserved.

Warn when files could override Python modules #10787

Warn when files could override Python modules #10787

Conversation

DonJayamanne commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 13, 2022 • 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

IanMatthewHuff left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 13, 2022 •

edited

Loading