-
Notifications
You must be signed in to change notification settings - Fork 85
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
Ensure NPM Windows Installs Work Correctly #2059
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -104,11 +104,17 @@ executors: | |
environment: | ||
XTASK_TARGET: "x86_64-unknown-linux-gnu" | ||
|
||
node_js: | ||
node_js_nix: | ||
docker: | ||
- image: node:lts | ||
- image: node:20.16.0 | ||
resource_class: medium | ||
|
||
node_js_windows: | ||
machine: | ||
image: "windows-server-2019-vs2019:2022.08.1" | ||
shell: powershell.exe -ExecutionPolicy Bypass | ||
resource_class: windows.medium | ||
|
||
|
||
tag_matches_prerelease: &tag_matches_prerelease | ||
matches: | ||
|
@@ -171,10 +177,11 @@ workflows: | |
rust_channel: [stable] | ||
command: [integration-test] | ||
- install_js: | ||
name: Test installation for Javascript Package Managers (<< matrix.package_manager >>) | ||
name: Test installation for Javascript Package Managers (<< matrix.package_manager >> on << matrix.platform >>) | ||
matrix: | ||
parameters: | ||
package_manager: [npm, npm_global, pnpm] | ||
platform: [windows, nix] | ||
- node/test: | ||
name: Test NPM Installer Scripts | ||
app-dir: "~/project/installers/npm" | ||
|
@@ -216,10 +223,11 @@ workflows: | |
<<: *run_release | ||
|
||
- install_js: | ||
name: Test installation for Javascript Package Managers (<< matrix.package_manager >>) | ||
name: Test installation for Javascript Package Managers (<< matrix.package_manager >> on << matrix.platform >>) | ||
matrix: | ||
parameters: | ||
package_manager: [ npm, npm_global, pnpm ] | ||
platform: [windows, nix] | ||
<<: *run_release | ||
|
||
- node/test: | ||
|
@@ -244,9 +252,12 @@ workflows: | |
- "Run cargo tests + studio integration tests (stable rust on amd_windows)" | ||
- "Run studio integration tests in GitHub Actions (amd_macos)" | ||
- "Run supergraph-demo tests (stable rust on amd_ubuntu)" | ||
- "Test installation for Javascript Package Managers (npm)" | ||
- "Test installation for Javascript Package Managers (npm_global)" | ||
- "Test installation for Javascript Package Managers (pnpm)" | ||
- "Test installation for Javascript Package Managers (npm on nix)" | ||
- "Test installation for Javascript Package Managers (npm_global on nix)" | ||
- "Test installation for Javascript Package Managers (pnpm on nix)" | ||
- "Test installation for Javascript Package Managers (npm on windows)" | ||
- "Test installation for Javascript Package Managers (npm_global on windows)" | ||
- "Test installation for Javascript Package Managers (pnpm on windows)" | ||
- "Test NPM Installer Scripts" | ||
<<: *run_release | ||
|
||
|
@@ -348,15 +359,42 @@ jobs: | |
package_manager: | ||
type: enum | ||
enum: ["npm", "npm_global", "pnpm"] | ||
executor: node_js | ||
platform: | ||
type: enum | ||
enum: ["nix", "windows"] | ||
executor: node_js_<<parameters.platform>> | ||
steps: | ||
- checkout: | ||
path: "rover" | ||
- run: | ||
name: "Invoke Install Scripts" | ||
command: | | ||
cd rover/.circleci/scripts | ||
./install_<< parameters.package_manager >>.sh | ||
- when: | ||
condition: | ||
equal: ["nix", <<parameters.platform>>] | ||
steps: | ||
- run: | ||
name: "Invoke Install Scripts (Unix)" | ||
command: | | ||
cd rover/.circleci/scripts/<<parameters.platform>> | ||
./install_<< parameters.package_manager >>.sh | ||
- when: | ||
condition: | ||
equal: ["windows", <<parameters.platform>>] | ||
steps: | ||
- run: | ||
name: "Invoke Install Scripts (Windows)" | ||
command: | | ||
Write-Output "Installing Volta" | ||
choco install volta | ||
refreshenv | ||
Write-Output "Installing Node & NPM" | ||
volta install [email protected] | ||
Write-Output "Checking Node & NPM version" | ||
node --version | ||
npm --version | ||
Comment on lines
+385
to
+392
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Annoyingly the Windows Executors come bundled with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well that is fun! |
||
|
||
$script_location=Join-Path rover\.circleci\scripts << parameters.platform >> | ||
Set-Location $script_location | ||
.\install_<< parameters.package_manager >>.ps1 | ||
|
||
|
||
# reusable command snippets can be referred to in any `steps` object | ||
commands: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The three |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
$ErrorActionPreference = "Stop" | ||
|
||
Function New-TemporaryFolder { | ||
# Make a new folder based upon a TempFileName | ||
$TEMP_PATH=[System.IO.Path]::GetTempPath() | ||
$T= Join-Path $TEMP_PATH tmp$([convert]::tostring((get-random 65535),16).padleft(4,'0')) | ||
New-Item -ItemType Directory -Path $T | ||
} | ||
|
||
# Find the installers directory | ||
$installer_dir = [System.IO.Path]::Combine($PSScriptRoot, "..", "..", "..", "installers", "npm") | ||
Write-Output "Found installers directory at $installer_dir" | ||
|
||
# Create a temporary folder for the test | ||
$test_dir = New-TemporaryFolder | ||
Write-Output "Created test directory at $test_dir" | ||
Set-Location $test_dir | ||
# Initialise an empty NPM package | ||
npm init -y | ||
Write-Output "Initialised new npm package" | ||
|
||
# The choice of version here is arbitrary (we just need something we know exists) so that we can test if the | ||
# installer works, given an existing version. This way we're not at the mercy of whether the binary that corresponds | ||
# to the latest commit exists. | ||
npm --prefix "$installer_dir" version --allow-same-version 0.23.0 | ||
Write-Output "Temporarily patched package.json to fixed stable binary" | ||
|
||
# Install all the dependencies, including `rover` | ||
npm install --install-links=true "$installer_dir" | ||
Write-Output "Installed rover as local npm package" | ||
|
||
# Move to the installed location | ||
$node_modules_path=[System.IO.Path]::Combine($test_dir, "node_modules", ".bin") | ||
Set-Location $node_modules_path | ||
|
||
# Check the version | ||
Write-Output "Checking version" | ||
$dir_sep=[IO.Path]::DirectorySeparatorChar | ||
$rover_command=".${dir_sep}rover --version" | ||
Invoke-Expression $rover_command | ||
Write-Output "Checked version, all ok!" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
$ErrorActionPreference = "Stop" | ||
|
||
Function New-TemporaryFolder { | ||
# Make a new folder based upon a TempFileName | ||
$TEMP_PATH=[System.IO.Path]::GetTempPath() | ||
$T= Join-Path $TEMP_PATH tmp$([convert]::tostring((get-random 65535),16).padleft(4,'0')) | ||
New-Item -ItemType Directory -Path $T | ||
} | ||
|
||
# Find the installers directory | ||
$installer_dir = [System.IO.Path]::Combine($PSScriptRoot, "..", "..", "..", "installers", "npm") | ||
Write-Output "Found installers directory at $installer_dir" | ||
|
||
# Create a temporary folder for the test | ||
$test_dir = New-TemporaryFolder | ||
Write-Output "Created test directory at $test_dir" | ||
Set-Location $test_dir | ||
|
||
# The choice of version here is arbitrary (we just need something we know exists) so that we can test if the | ||
# installer works, given an existing version. This way we're not at the mercy of whether the binary that corresponds | ||
# to the latest commit exists. | ||
npm --prefix "$installer_dir" version --allow-same-version 0.23.0 | ||
Write-Output "Temporarily patched package.json to fixed stable binary" | ||
|
||
# Install all the dependencies, including `rover` | ||
npm install --install-links=true -g "$installer_dir" | ||
Write-Output "Installed rover as global npm package" | ||
|
||
# Check the version | ||
Write-Output "Checking version" | ||
rover --version | ||
Write-Output "Checked version, all ok!" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
$ErrorActionPreference = "Stop" | ||
|
||
Function New-TemporaryFolder { | ||
# Make a new folder based upon a TempFileName | ||
$TEMP_PATH=[System.IO.Path]::GetTempPath() | ||
$T= Join-Path $TEMP_PATH tmp$([convert]::tostring((get-random 65535),16).padleft(4,'0')) | ||
New-Item -ItemType Directory -Path $T | ||
} | ||
|
||
# Find the installers directory | ||
$installer_dir = [System.IO.Path]::Combine($PSScriptRoot, "..", "..", "..", "installers", "npm") | ||
Write-Output "Found installers directory at $installer_dir" | ||
|
||
# Create a temporary folder for the test | ||
$test_dir = New-TemporaryFolder | ||
Write-Output "Created test directory at $test_dir" | ||
Set-Location $test_dir | ||
# Install pnpm | ||
npm install -g [email protected] | ||
|
||
# The choice of version here is arbitrary (we just need something we know exists) so that we can test if the | ||
# installer works, given an existing version. This way we're not at the mercy of whether the binary that corresponds | ||
# to the latest commit exists. | ||
npm --prefix "$installer_dir" version --allow-same-version 0.23.0 | ||
Write-Output "Temporarily patched package.json to fixed stable binary" | ||
|
||
# Install all the dependencies, including `rover` | ||
pnpm init | ||
pnpm add "file:$installer_dir" | ||
Write-Output "Installed rover as local npm package" | ||
|
||
# Move to the installed location | ||
$node_modules_path=[System.IO.Path]::Combine($test_dir, "node_modules", ".bin") | ||
Set-Location $node_modules_path | ||
|
||
# Check the version | ||
Write-Output "Checking version" | ||
$dir_sep=[IO.Path]::DirectorySeparatorChar | ||
$rover_command=".${dir_sep}rover --version" | ||
Invoke-Expression $rover_command | ||
Write-Output "Checked version, all ok!" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,37 +25,39 @@ const supportedPlatforms = [ | |
ARCHITECTURE: "x64", | ||
RUST_TARGET: "x86_64-pc-windows-msvc", | ||
BINARY_NAME: `${name}-${version}.exe`, | ||
RAW_NAME: `${name}.exe` | ||
}, | ||
{ | ||
TYPE: "Linux", | ||
ARCHITECTURE: "x64", | ||
RUST_TARGET: "x86_64-unknown-linux-gnu", | ||
BINARY_NAME: `${name}-${version}`, | ||
RAW_NAME: `${name}` | ||
}, | ||
{ | ||
TYPE: "Linux", | ||
ARCHITECTURE: "arm64", | ||
RUST_TARGET: "aarch64-unknown-linux-gnu", | ||
BINARY_NAME: `${name}-${version}`, | ||
RAW_NAME: `${name}` | ||
}, | ||
{ | ||
TYPE: "Darwin", | ||
ARCHITECTURE: "x64", | ||
RUST_TARGET: "x86_64-apple-darwin", | ||
BINARY_NAME: `${name}-${version}`, | ||
RAW_NAME: `${name}` | ||
}, | ||
{ | ||
TYPE: "Darwin", | ||
ARCHITECTURE: "arm64", | ||
RUST_TARGET: "aarch64-apple-darwin", | ||
BINARY_NAME: `${name}-${version}`, | ||
RAW_NAME: `${name}` | ||
}, | ||
]; | ||
|
||
const getPlatform = () => { | ||
const type = os.type(); | ||
const architecture = os.arch(); | ||
|
||
const getPlatform = (type = os.type(), architecture = os.arch()) => { | ||
for (let supportedPlatform of supportedPlatforms) { | ||
if ( | ||
type === supportedPlatform.TYPE && | ||
|
@@ -102,7 +104,7 @@ const getPlatform = () => { | |
|
||
/*! Copyright (c) 2019 Avery Harnish - MIT License */ | ||
class Binary { | ||
constructor(name, url, installDirectory) { | ||
constructor(name, raw_name, url, installDirectory) { | ||
let errors = []; | ||
if (typeof url !== "string") { | ||
errors.push("url must be a string"); | ||
|
@@ -132,6 +134,7 @@ class Binary { | |
} | ||
this.url = url; | ||
this.name = name; | ||
this.raw_name = raw_name; | ||
this.installDirectory = installDirectory; | ||
|
||
if (!existsSync(this.installDirectory)) { | ||
|
@@ -176,7 +179,7 @@ class Binary { | |
}); | ||
}) | ||
.then(() => { | ||
fs.renameSync(join(this.installDirectory, name), this.binaryPath); | ||
fs.renameSync(join(this.installDirectory, this.raw_name), this.binaryPath); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the actual bug-fix here, make sure we use the raw_name when renaming (so |
||
if (!suppressLogs) { | ||
console.error(`${this.name} has been installed!`); | ||
} | ||
|
@@ -212,8 +215,7 @@ class Binary { | |
} | ||
} | ||
|
||
const getBinary = (overrideInstallDirectory) => { | ||
const platform = getPlatform(); | ||
const getBinary = (overrideInstallDirectory, platform = getPlatform()) => { | ||
const download_host = process.env.npm_config_apollo_rover_download_host || process.env.APOLLO_ROVER_DOWNLOAD_HOST || 'https://rover.apollo.dev' | ||
// the url for this binary is constructed from values in `package.json` | ||
// https://rover.apollo.dev/tar/rover/x86_64-unknown-linux-gnu/v0.4.8 | ||
|
@@ -224,7 +226,7 @@ const getBinary = (overrideInstallDirectory) => { | |
if (overrideInstallDirectory != null && overrideInstallDirectory !== "") { | ||
installDirectory = overrideInstallDirectory | ||
} | ||
let binary = new Binary(platform.BINARY_NAME, url, installDirectory); | ||
let binary = new Binary(platform.BINARY_NAME, platform.RAW_NAME, url, installDirectory); | ||
|
||
// setting this allows us to extract supergraph plugins to the proper directory | ||
// the variable itself is read in Rust code | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes here are mostly plumbing, just getting CircleCI to do what we want it to