Skip to content

Resolve pythonPath before comparing it to shebang #4403

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

Merged
merged 4 commits into from
Mar 5, 2019
Merged

Resolve pythonPath before comparing it to shebang #4403

merged 4 commits into from
Mar 5, 2019

Conversation

fnesveda
Copy link

@fnesveda fnesveda commented Feb 14, 2019

For #4601

When comparing if the path to the Python interpreter from the shebang in a file and the pythonPath setting point to one and the same interpreter, resolve pythonPath to the path to the actual Python interpreter executable before the comparison.

This solves the problem of having python.pythonPath set to a symlink to the actual Python interpreter and having a /usr/bin/env shebang in a file, where both would actually point to the same interpreter, but the Set as interpreter CodeLens would display anyway, because the CodeLens provider wouldn't recognize that.

For example, on macOS, with Python installed by Homebrew,#!/usr/bin/env python3 and python.pythonPath: "/usr/local/bin/python3" both actually point to the same interpreter and give sys.executable == '/usr/local/opt/python3/bin/python3.7', but they would be treated as different interpreters.

This was originally done with 7372367 but then undone with edbd57a.

  • 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!)
  • 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)

When comparing if the path to the Python interpreter from the shebang in a file and the pythonPath setting point to one and the same interpreter, resolve pythonPath to the path to the actual Python interpreter executable before the comparison.
This fixes unit test failures introduced by commit #37cf7bb by adapting strategies from the related unit tests.
@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #4403 into master will increase coverage by 20%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #4403     +/-   ##
========================================
+ Coverage      58%     77%    +20%     
========================================
  Files         369     445     +76     
  Lines       15942   21074   +5132     
  Branches     2506    3424    +918     
========================================
+ Hits         9135   16219   +7084     
+ Misses       6185    4849   -1336     
+ Partials      622       6    -616
Flag Coverage Δ
#Linux 65% <75%> (?)
#Windows 66% <75%> (?)
#macOS 65% <75%> (?)

@fnesveda
Copy link
Author

I have updated the unit tests which were failing after the change. I believe they were failing only because ShebangCodeLensProvider.getFullyQualifiedPathToInterpreter() does not run properly during unit testing, I suspect that's why the detectShebang function is being overriden during tests to return a fixed value.

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

One simple change required, then all should be good.

@DonJayamanne
Copy link

@fnesveda Thanks for the PR.
Please could you address the requested change, once done, I can merge this.

Return early when there is no valid shebang in the document, to avoid waiting for pythonPath to be resolved when not necessary.
@fnesveda
Copy link
Author

fnesveda commented Mar 4, 2019

I did the change as requested, but now the CI times out, and for the life of me I can't see how the change could affect that. On my local machine all the tests pass, but that's a Mac, and the debugger tests time out on Linux. I hope it's not because of the force push, I did that only to push a fix for a trailing space that I didn't want to litter the commit history with.

@DonJayamanne DonJayamanne merged commit b78a3c9 into microsoft:master Mar 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants