Skip to content

Conversation

int19h
Copy link

@int19h int19h commented Nov 9, 2020

Fix #14674: Enable overriding "pythonPath" in the launcher

Fix #12462: Update launch.json schema to add "python" and remove "pythonPath"

Split the "pythonPath" debug property into "python", "debugAdapterPython", and "debugLauncherPython".

Properly register the python.interpreterPath command to expand the ${command:interpreterPath} debug configuration variable.

Do most debug config validation on fully expanded property values via resolveDebugConfigurationWithSubstitutedVariables().

Add fixups for legacy launch.json with "pythonPath" and/or "${command:python.interpreterPath}".

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • The wiki is updated with any design decisions/details.

@int19h
Copy link
Author

int19h commented Nov 9, 2020

The corresponding PR on debugpy side is coming soon.

debugOptions?: DebugOptions[];
env?: Record<string, string | undefined>;
envFile: string;
envFile?: string;
Copy link
Author

Choose a reason for hiding this comment

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

So far as I can tell, these properties have always been improperly typed - test code used casts and any in places to work around that.

@codecov-io
Copy link

codecov-io commented Nov 9, 2020

Codecov Report

Merging #14676 (0386fed) into main (98f5b49) will increase coverage by 5.08%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #14676      +/-   ##
==========================================
+ Coverage   59.77%   64.85%   +5.08%     
==========================================
  Files         670      547     -123     
  Lines       37212    25792   -11420     
  Branches     5270     3679    -1591     
==========================================
- Hits        22243    16728    -5515     
+ Misses      13841     8373    -5468     
+ Partials     1128      691     -437     
Impacted Files Coverage Δ
...tension/configuration/debugConfigurationService.ts 71.42% <0.00%> (-7.52%) ⬇️
...c/client/debugger/extension/configuration/types.ts 100.00% <ø> (ø)
src/client/debugger/types.ts 100.00% <ø> (ø)
src/client/testing/common/debugLauncher.ts 90.82% <60.00%> (-1.63%) ⬇️
...bugger/extension/configuration/resolvers/launch.ts 91.57% <95.83%> (+0.66%) ⬆️
...on/diagnostics/checks/invalidLaunchJsonDebugger.ts 94.18% <100.00%> (+0.06%) ⬆️
src/client/debugger/extension/adapter/factory.ts 94.11% <100.00%> (+0.24%) ⬆️
...bugger/extension/configuration/resolvers/attach.ts 80.00% <100.00%> (ø)
...debugger/extension/configuration/resolvers/base.ts 91.25% <100.00%> (+0.22%) ⬆️
src/client/common/installer/productPath.ts 25.00% <0.00%> (-75.00%) ⬇️
... and 341 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 98f5b49...0386fed. Read the comment docs.


return config as LaunchRequestArguments;
}

Copy link
Author

Choose a reason for hiding this comment

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

This is, essentially, the sequence of calls that VSCode itself performs:

  1. resolveDebugConfiguration()
  2. substitute ${variables}
  3. resolveDebugConfigurationWithSubstitutedVariables()

@int19h int19h force-pushed the py branch 2 times, most recently from eb85b48 to bb1067d Compare November 9, 2020 23:29
Copy link

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for working on this. I left a few comments mostly to clarify my understanding but I'm fine with the PR as-is.

// The following are all internal properties that are not publicly documented or
// exposed in launch.json schema for the extension.

// Python interpreter used by the extension to spawn the debug adapter.

Choose a reason for hiding this comment

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

FWIW, I personally try to call it "Python executable" (or sometimes "Python runtime") instead of "Python interpreter". The former is more correct while still being clear.

Then again, a lot of folks call it "interpreter" too, so it isn't a big deal. I suppose at this point my brain has sufficiently associated "interpreter" with that exact portion of the runtime such that hearing the word applied to the executable is now a bit uncomfortable for me. 😄 (Plus "interpreter" will probably be a little more confusing once subinterpreters are a bigger part of the Python experience.)

Just to be clear, this is not a request that you change anything (though I'd personally like that). I'm just pointing out something to think about that most of the time isn't obvious.

Copy link
Author

Choose a reason for hiding this comment

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

I've been struggling with a good term for that for a very long time - this is partly why the new setting was called "python" back when debugpy added it; that way I can just say, "it's whatever goes in place of python in the command line you'd typically use to start".

The main reason why it's "interpreter" here is because we have command:interpreterPath. Also, generally speaking, more recent code in the extension seems to be using "interpreter" more often. It would be good to standardize on something across the codebase, whatever that ends up being.

Choose a reason for hiding this comment

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

Hmm, we made a decision a while back to not use the term "interpreter" so much, favoring "executable" or "environment" instead (depending on the context). Apparently we haven't been doing very well at that. 😞

Fix microsoft#12462: Update launch.json schema to add "python" and remove "pythonPath"

Split the "pythonPath" debug property into "python", "debugAdapterPython", and "debugLauncherPython".

Do most debug config validation on fully expanded property values via resolveDebugConfigurationWithSubstitutedVariables().

Add fixups for legacy launch.json with "pythonPath".
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@brettcannon brettcannon changed the title Fix #12462, #14674 Fix #12462, #14674 for debugging Nov 13, 2020
@int19h int19h merged commit 4e45a4d into microsoft:main Nov 16, 2020
int19h pushed a commit to int19h/vscode-python that referenced this pull request Nov 16, 2020
…icrosoft#14676)

Fix microsoft#12462: Update launch.json schema to add "python" and remove "pythonPath"

Split the "pythonPath" debug property into "python", "debugAdapterPython", and "debugLauncherPython".

Do most debug config validation on fully expanded property values via resolveDebugConfigurationWithSubstitutedVariables().

Add fixups for legacy launch.json with "pythonPath".
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 83.33333% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 64%. Comparing base (98f5b49) to head (0386fed).
⚠️ Report is 2996 commits behind head on main.

Files with missing lines Patch % Lines
...tension/configuration/debugConfigurationService.ts 0% 4 Missing ⚠️
src/client/testing/common/debugLauncher.ts 60% 1 Missing and 1 partial ⚠️
...bugger/extension/configuration/resolvers/launch.ts 95% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##            main   #14676      +/-   ##
=========================================
+ Coverage     59%      64%      +5%     
=========================================
  Files        670      547     -123     
  Lines      37212    25792   -11420     
  Branches    5270     3679    -1591     
=========================================
- Hits       22243    16728    -5515     
+ Misses     13841     8373    -5468     
+ Partials    1128      691     -437     
Files with missing lines Coverage Δ
...on/diagnostics/checks/invalidLaunchJsonDebugger.ts 94% <100%> (+<1%) ⬆️
src/client/debugger/extension/adapter/factory.ts 94% <100%> (+<1%) ⬆️
...bugger/extension/configuration/resolvers/attach.ts 80% <100%> (ø)
...debugger/extension/configuration/resolvers/base.ts 91% <100%> (+<1%) ⬆️
...c/client/debugger/extension/configuration/types.ts 100% <ø> (ø)
src/client/debugger/types.ts 100% <ø> (ø)
...bugger/extension/configuration/resolvers/launch.ts 91% <95%> (+<1%) ⬆️
src/client/testing/common/debugLauncher.ts 90% <60%> (-2%) ⬇️
...tension/configuration/debugConfigurationService.ts 71% <0%> (-8%) ⬇️

... and 342 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable overriding "pythonPath" in the launcher Update launch.json schema to add "python" and remove "pythonPath"

6 participants