Skip to content

CI: Simplify preparation on Windows, Use Python 3.8 on all OSes #140

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

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Sep 8, 2020

Description of the Change

  • Don't update npm twice. Once is enough.
    • Delete these redundant steps: npm install -g npm-windows-upgrade and npm-windows-upgrade.
    • Keep the npm install -g npm command.
      • The npm install -g npm command modifies the npm at C:/npm/prefix/npm.cmd. So we set the NPM_BIN_PATH environment variable to point to that path instead of C:/hostedtoolcache/windows/node/[maj.min.patch]/x64/npm.cmd, which is the install that npm-windows-upgrade modifies.
  • Set the Python version in the "Common" (all OSes) section of preparation, rather than doing this only on Windows.

Quantitative Performance Benefits

About 45 seconds is saved before bootstrapping/building on Windows, and another 45 seconds is saved in each Windows test job.

  • Installing npm-windows-upgrade takes about 20 seconds
  • Running npm-windows-upgrade takes about 24 or 25 seconds.

See the time taken for those steps in this CI run for an example: https://dev.azure.com/DeeDeeG/b/_build/results?buildId=625&view=logs&j=2985f0af-e798-5fdc-91b8-be9f0a3685c5&t=33143b02-a236-56d5-9221-c8b903d3c111

Possible Drawbacks

None.

Verification Process

CI Passes. For example: https://dev.azure.com/DeeDeeG/b/_build/results?buildId=624&view=results

Windows correctly uses npm 6.14.8 as we have configured it to. See: https://dev.azure.com/DeeDeeG/b/_build/results?buildId=624&view=logs&j=2985f0af-e798-5fdc-91b8-be9f0a3685c5&t=9ceed7b2-fba1-5410-fd8f-91cfc59f4174 to confirm.

Applicable Issues

Closes #51.

Release Notes

N/A

Copy link
Member

@aminya aminya left a comment

Choose a reason for hiding this comment

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

Python installation should be unified for all operating systems.

@DeeDeeG

This comment has been minimized.

@DeeDeeG DeeDeeG changed the title CI: Simplify preparation on Windows CI: Simplify preparation on Windows, Use Python 3.8 on all OSes Sep 8, 2020
@DeeDeeG DeeDeeG requested a review from aminya September 8, 2020 19:59
@DeeDeeG DeeDeeG force-pushed the CI-simplify-windows-preparation branch from 6028262 to 1191b31 Compare September 9, 2020 00:12
@aminya aminya merged commit a0496d4 into atom-community:master Sep 9, 2020
aminya added a commit that referenced this pull request Sep 11, 2020
We already install the npm version we want in previous steps

#140
aminya added a commit that referenced this pull request Sep 11, 2020
We already install the npm version we want in previous steps

#140
aminya added a commit that referenced this pull request Sep 11, 2020
We already install the npm version we want in previous steps

#140

Co-Authored-By: DeeDeeG <[email protected]>
aminya added a commit that referenced this pull request Sep 11, 2020
aminya added a commit that referenced this pull request Sep 28, 2020
We already install the npm version we want in previous steps

#140

Co-Authored-By: DeeDeeG <[email protected]>
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.

Updating Python 3 for all operating systems
2 participants