Skip to content

Add extension dependencies at build time #14636

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 1 commit into from
Nov 5, 2020
Merged

Add extension dependencies at build time #14636

merged 1 commit into from
Nov 5, 2020

Conversation

joyceerhl
Copy link

For #14635

  • 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.

@auto-assign auto-assign bot requested review from karrtikr and kimadeline November 5, 2020 21:17
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 5, 2020

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
No Duplication information No Duplication information

@codecov-io
Copy link

Codecov Report

Merging #14636 into main will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #14636      +/-   ##
==========================================
+ Coverage   65.08%   65.10%   +0.01%     
==========================================
  Files         541      541              
  Lines       25377    25377              
  Branches     3587     3587              
==========================================
+ Hits        16516    16521       +5     
+ Misses       8179     8178       -1     
+ Partials      682      678       -4     
Impacted Files Coverage Δ
src/client/activation/languageClientMiddleware.ts 32.46% <0.00%> (ø)
...tascience/languageserver/notebookConcatDocument.ts
...nt/datascience/languageserver/notebookConverter.ts
...ascience/languageserver/notebookMiddlewareAddon.ts
...client/jupyter/languageserver/notebookConverter.ts 1.67% <0.00%> (ø)
.../jupyter/languageserver/notebookMiddlewareAddon.ts 3.84% <0.00%> (ø)
...t/jupyter/languageserver/notebookConcatDocument.ts 7.81% <0.00%> (ø)
src/client/common/process/proc.ts 15.21% <0.00%> (+0.72%) ⬆️
.../locators/services/virtualEnvironmentIdentifier.ts 91.78% <0.00%> (+1.36%) ⬆️
.../pythonEnvironments/common/externalDependencies.ts 74.46% <0.00%> (+2.12%) ⬆️
... and 2 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 598d849...c3ec59e. Read the comment docs.

@joyceerhl joyceerhl merged commit 7887500 into main Nov 5, 2020
@joyceerhl joyceerhl deleted the fix-gha branch November 5, 2020 22:33
@joyceerhl
Copy link
Author

I imagine this also needs to go into the release branch? @karthiknadig

@karthiknadig
Copy link
Member

This and few other things... I will pull them all to the release branch

@karrtikr
Copy link

karrtikr commented Nov 5, 2020

@joyceerhl GHA builds corresponding to this PR are failing: https://github.com/microsoft/vscode-python/actions/runs/348408071 can you please check?

@brettcannon
Copy link
Member

@joyceerhl
Copy link
Author

Sorry guys, investigating. I initially suspected a problem with smoke tests running with the VSIX, but it looks like the Build VSIX step itself is failing.

The GHA runs for the commit before this were fine. The difference is here:

> node-pre-gyp install --fallback-to-build

node-pre-gyp WARN Using request for node-pre-gyp https download 
[canvas] Success: "/home/runner/work/vscode-python/vscode-python/node_modules/canvas/build/Release/canvas.node" is installed via remote

versus after this PR went in:

> node-pre-gyp install --fallback-to-build

node-pre-gyp WARN Using request for node-pre-gyp https download 
node-pre-gyp WARN Tried to download(404): https://github.com/node-gfx/node-canvas-prebuilt/releases/download/v2.6.0/canvas-v2.6.0-node-v83-linux-glibc-x64.tar.gz 
node-pre-gyp WARN Pre-built binaries not found for [email protected] and [email protected] (node-v83 ABI, glibc) (falling back to source compile with node-gyp) 

I do not see an asset matching https://github.com/node-gfx/node-canvas-prebuilt/releases/download/v2.6.0/canvas-v2.6.0-node-v83-linux-glibc-x64.tar.gz at https://github.com/node-gfx/node-canvas-prebuilt/releases/tag/v2.6.0. Still investigating but wanted to quickly mention that I am looking into it and it's not yet clear to me how this PR could be causing this problem.

@kimadeline
Copy link

Is this related to the rollout of Node 14.X: actions/runner-images#1953 (@brettcannon mentioned it offline)?

The Ubuntu image changed between the last working commit and this one:

@joyceerhl
Copy link
Author

Yes that seems likely. IIRC our pipelines have used Node 12.15.x in the past. We'd want to use https://github.com/actions/setup-node to configure GHA with Node 12.X

@joyceerhl
Copy link
Author

joyceerhl commented Nov 6, 2020

Edit: Oops, was looking at the wrong repo's configs

There's a setup-node step defined in insiders.yml and release.yml, but it's being skipped for some reason:

      - name: Use Node ${{env.NODE_VERSION}}
        uses: actions/[email protected]
        with:
          node-version: ${{env.NODE_VERSION}}~

@kimadeline
Copy link

I believe we only use it for tests at the moment, and even then we set it up only if the previous step was successful (which wasn't the case). It should probably be moved to be the first step of the test job yeah.

@joyceerhl
Copy link
Author

Opened this PR to address the Node version issue: #14641

@joyceerhl
Copy link
Author

I believe the failures have been resolved: https://github.com/microsoft/vscode-python/actions/runs/348786875 Thanks @kimadeline for the pointer to the Node 14.x rollout, that was indeed the problem!

karthiknadig pushed a commit to karthiknadig/vscode-python that referenced this pull request Nov 6, 2020
karthiknadig added a commit that referenced this pull request Nov 6, 2020
* Update shipped wheels version (#14615)

* Update shipped wheels version

* News item

* Remove redundant files (#14620)

* Add extension dependencies at build time (#14636)

* Use Node 12.15 in Insiders and Release GitHub Actions (#14641)

* Use Node 12.15 on all Insiders and Release GitHub Actions jobs (#14642)

Co-authored-by: Joyce Er <[email protected]>
karthiknadig added a commit that referenced this pull request Nov 26, 2020
* Cherry pick from main (#14644)

* Update shipped wheels version (#14615)

* Update shipped wheels version

* News item

* Remove redundant files (#14620)

* Add extension dependencies at build time (#14636)

* Use Node 12.15 in Insiders and Release GitHub Actions (#14641)

* Use Node 12.15 on all Insiders and Release GitHub Actions jobs (#14642)

Co-authored-by: Joyce Er <[email protected]>

* Cherry pic fixes into release for tests. (#14673)

* Added FSWatching base class and made related changes (#14605)

* Add FSWatching locator base class

* Correct glob pattern to not match python3.2whoa

* Add documentation of python binary watcher

* Fix lint errors

* Update ignore list

* Add disposable registry

* Modify FSWatching Locator

* Code reviews

* Use string[]

* Remove list disposable getter

* Fix failing global virtual env watcher tests (#14633)

Co-authored-by: Kartik Raj <[email protected]>

* Version, change log and cherrypicks for nov release (#14696)

* change log updates

* Update gifs

* Fix for interpreter selection (#14693)

* Fix for interpreter selection

* Fix linting errors

* Minor tweak to property removal

* Cherry pick "Bind function to correct this for workspace syms" (#14743)

* 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".

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

Add fixups for legacy launch.json with "pythonPath".

* Point release change log and version update (#14750)

* Point release change log and version update

* Fix process picker (#14700)

* Workaround VSCode bug for process picker

* Fix how we pass in icons to VSCode

* update change log with cherry pick

Co-authored-by: Kartik Raj <[email protected]>

Co-authored-by: Joyce Er <[email protected]>
Co-authored-by: Kartik Raj <[email protected]>
Co-authored-by: Jake Bailey <[email protected]>
Co-authored-by: Pavel Minaev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog No news entry required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants