-
Notifications
You must be signed in to change notification settings - Fork 6
fix: Replace simple-binary-install with custom implementation for pnpm compatibility #170
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
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 |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| /package-lock.json | ||
| /node_modules/ | ||
| /bin/ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,39 +1,112 @@ | ||
| import { Binary } from 'simple-binary-install'; | ||
| import * as os from 'os'; | ||
| import * as fs from 'fs'; | ||
| import fetch from 'node-fetch'; | ||
| import { mkdirSync, chmodSync, existsSync, readFileSync } from 'fs'; | ||
|
Contributor
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. Prefix all node native modules with |
||
| import { fileURLToPath } from 'url'; | ||
| import { dirname, join } from 'path'; | ||
| import { pipeline } from 'stream/promises'; | ||
| import * as tar from 'tar'; | ||
|
|
||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = dirname(__filename); | ||
|
|
||
| function getPlatform() { | ||
| const type = os.type(); | ||
| const arch = os.arch(); | ||
| const type = process.platform; | ||
| const arch = process.arch; | ||
|
|
||
| if (type === 'Windows_NT' && arch === 'x64') { | ||
| if (type === 'win32' && arch === 'x64') { | ||
| return 'x86_64-pc-windows-msvc'; | ||
| } | ||
|
|
||
| if (type === 'Linux' && arch === 'x64') { | ||
| if (type === 'linux' && arch === 'x64') { | ||
| return 'x86_64-unknown-linux-musl'; | ||
| } | ||
|
|
||
| if (type === 'Linux' && arch === 'arm64') { | ||
| if (type === 'linux' && arch === 'arm64') { | ||
| return 'aarch64-unknown-linux-musl'; | ||
| } | ||
|
|
||
| if (type === 'Darwin' && arch === 'x64') { | ||
| if (type === 'darwin' && arch === 'x64') { | ||
| return 'x86_64-apple-darwin'; | ||
| } | ||
|
|
||
| if (type === 'Darwin' && arch === 'arm64') { | ||
| if (type === 'darwin' && arch === 'arm64') { | ||
|
Contributor
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. I'd consider wrapping in utility functions; these things are repetitive across the code. |
||
| return 'aarch64-apple-darwin'; | ||
| } | ||
|
|
||
| throw new Error(`Unsupported platform: ${type} ${arch}. Please create an issue at https://github.com/coralogix/protofetch/issues`); | ||
| } | ||
|
|
||
| export function getBinary() { | ||
| function getVersion() { | ||
| const packageJsonPath = join(__dirname, 'package.json'); | ||
| const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8')); | ||
| return packageJson.version; | ||
| } | ||
|
|
||
| async function downloadBinary(options = {}) { | ||
| const platform = getPlatform(); | ||
| const { version } = JSON.parse(fs.readFileSync('./package.json')); | ||
| const url = `https://github.com/coralogix/protofetch/releases/download/v${version}/protofetch_${platform}.tar.gz`; | ||
| const name = 'protofetch'; | ||
| const version = getVersion(); | ||
|
|
||
| // Support custom URL for testing (not exposed via postinstall, only via direct script call) | ||
| const url = options.url || `https://github.com/coralogix/protofetch/releases/download/v${version}/protofetch_${platform}.tar.gz`; | ||
|
|
||
| const binDir = join(__dirname, 'bin'); | ||
| const isWindows = process.platform === 'win32'; | ||
| const binaryName = isWindows ? 'protofetch.exe' : 'protofetch'; | ||
| const binaryPath = join(binDir, binaryName); | ||
|
|
||
| if (!existsSync(binDir)) { | ||
| mkdirSync(binDir, { recursive: true }); | ||
| } | ||
|
|
||
| console.log(`Downloading protofetch binary from ${url}...`); | ||
|
|
||
| return new Binary(name, url) | ||
| let lastError; | ||
| for (let attempt = 1; attempt <= 3; attempt++) { | ||
| try { | ||
| const response = await fetch(url, { | ||
| redirect: 'follow', | ||
| timeout: 60000 | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to download binary (HTTP ${response.status}): ${response.statusText}`); | ||
| } | ||
|
|
||
| await pipeline( | ||
| response.body, | ||
| tar.extract({ | ||
| cwd: binDir, | ||
| strip: 1, | ||
| strict: true, | ||
| preservePaths: false, | ||
| preserveOwner: false, | ||
| filter: (path, entry) => { | ||
| const allowedFiles = ['protofetch', 'protofetch.exe']; | ||
| const fileName = path.split('/').pop(); | ||
| return entry.type === 'File' && allowedFiles.includes(fileName); | ||
| } | ||
| }) | ||
| ); | ||
|
|
||
| if (!isWindows && existsSync(binaryPath)) { | ||
| chmodSync(binaryPath, 0o755); | ||
| } | ||
|
|
||
| if (existsSync(binaryPath)) { | ||
| console.log('protofetch binary installed successfully'); | ||
| return; | ||
| } else { | ||
| throw new Error(`Binary extraction failed - ${binaryName} not found after extraction`); | ||
| } | ||
| } catch (error) { | ||
| lastError = error; | ||
| if (attempt < 3) { | ||
| console.log(`Download attempt ${attempt} failed, retrying...`); | ||
| await new Promise(resolve => setTimeout(resolve, attempt * 1000)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| throw new Error(`Failed to download protofetch after 3 attempts: ${lastError.message}`); | ||
| } | ||
|
|
||
| export { downloadBinary }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,26 @@ | ||
| #!/usr/bin/env node | ||
| import { getBinary } from './getBinary.js'; | ||
| import { spawn } from 'child_process'; | ||
|
Contributor
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. prefix with |
||
| import path from 'path'; | ||
| import { fileURLToPath } from 'url'; | ||
|
|
||
| getBinary().run(); | ||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = path.dirname(__filename); | ||
|
|
||
| const isWindows = process.platform === 'win32'; | ||
| const binaryName = isWindows ? 'protofetch.exe' : 'protofetch'; | ||
| const binaryPath = path.join(__dirname, 'bin', binaryName); | ||
|
|
||
| const child = spawn(binaryPath, process.argv.slice(2), { stdio: 'inherit' }); | ||
|
|
||
ocombe marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| child.on('error', (error) => { | ||
| console.error(`Failed to start protofetch: ${error.message}`); | ||
| console.error('The binary may be missing or corrupted. Try reinstalling the package:'); | ||
| console.error(' npm install --force'); | ||
| console.error(' or'); | ||
| console.error(' pnpm install --force'); | ||
| process.exit(1); | ||
| }); | ||
|
|
||
| child.on('close', (code) => { | ||
| process.exit(code); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,34 @@ | ||
| import { getBinary } from './getBinary.js'; | ||
| import { downloadBinary } from './getBinary.js'; | ||
|
|
||
| if (process.argv.includes('install')) { | ||
| getBinary().install(); | ||
| // Check for --url argument for testing (only localhost allowed for security) | ||
| const urlArg = process.argv.find(arg => arg.startsWith('--url=')); | ||
|
Contributor
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. parseArgs? |
||
| let url = null; | ||
|
|
||
| if (urlArg) { | ||
| const providedUrl = urlArg.split('=')[1]; | ||
| try { | ||
| const parsedUrl = new URL(providedUrl); | ||
| // Only allow localhost URLs for testing | ||
| if (parsedUrl.hostname === 'localhost' || parsedUrl.hostname === '127.0.0.1') { | ||
|
Contributor
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. isLocalhost() |
||
| url = providedUrl; | ||
| } else { | ||
| console.error('Error: --url parameter only allows localhost URLs for security reasons'); | ||
| process.exit(1); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error: Invalid URL provided to --url parameter'); | ||
| process.exit(1); | ||
| } | ||
| } | ||
|
|
||
| downloadBinary({ url }) | ||
| .then(() => { | ||
| console.log('Installation complete'); | ||
| process.exit(0); | ||
| }) | ||
| .catch((error) => { | ||
| console.error('Installation failed:', error.message); | ||
| process.exit(1); | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -157,10 +157,110 @@ jobs: | |
| name: package-${{ matrix.target.rust }} | ||
| path: protofetch_${{ matrix.target.rust }}.tar.gz | ||
|
|
||
| test-npm-package: | ||
| needs: [ package ] | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: | ||
| - runner: ubuntu-latest | ||
| platform: x86_64-unknown-linux-musl | ||
| - runner: macos-14 | ||
| platform: aarch64-apple-darwin | ||
| - runner: macos-13 | ||
| platform: x86_64-apple-darwin | ||
| - runner: windows-latest | ||
| platform: x86_64-pc-windows-msvc | ||
| runs-on: ${{ matrix.os.runner }} | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20' | ||
|
Contributor
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. node 20 is maintenance, use 22 |
||
|
|
||
| - name: Install pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: 8 | ||
|
Contributor
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. latest is 10, why 8? |
||
|
|
||
| - name: Download artifacts | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: package-${{ matrix.os.platform }} | ||
| path: artifacts | ||
|
|
||
| - name: Setup test environment | ||
| shell: bash | ||
| run: | | ||
| cd .github/npm | ||
| # Replace version placeholder with test version | ||
ocombe marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| sed 's/VERSION#TO#REPLACE/0.0.0-test/g' package.json > package.test.json | ||
| mv package.test.json package.json | ||
|
|
||
| - name: Start HTTP server | ||
| shell: bash | ||
| run: | | ||
| cd artifacts | ||
| # Start HTTP server in background | ||
| python3 -m http.server 8000 & | ||
|
Contributor
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. Any reason not to continue with Node here? |
||
| echo $! > /tmp/http_server.pid | ||
| sleep 2 | ||
| echo "HTTP server started on port 8000" | ||
|
|
||
| - name: Test npm installation | ||
| shell: bash | ||
| run: | | ||
| cd .github/npm | ||
|
|
||
| # Install dependencies | ||
| npm install --ignore-scripts | ||
|
|
||
| # Run installation script with custom URL pointing to local HTTP server | ||
| node scripts.js install --url=http://localhost:8000/protofetch_${{ matrix.os.platform }}.tar.gz | ||
|
|
||
| # Verify binary was extracted and works | ||
| if [ "${{ runner.os }}" = "Windows" ]; then | ||
| ./bin/protofetch.exe --version | ||
| else | ||
| ./bin/protofetch --version | ||
| fi | ||
|
|
||
| # Clean up | ||
| rm -rf node_modules package-lock.json bin/ | ||
|
|
||
| - name: Test pnpm installation | ||
| shell: bash | ||
| run: | | ||
| cd .github/npm | ||
|
|
||
| # Install dependencies | ||
| pnpm install --ignore-scripts | ||
|
|
||
| # Run installation script with custom URL pointing to local HTTP server | ||
| node scripts.js install --url=http://localhost:8000/protofetch_${{ matrix.os.platform }}.tar.gz | ||
|
|
||
| # Verify binary was extracted and works | ||
| if [ "${{ runner.os }}" = "Windows" ]; then | ||
| ./bin/protofetch.exe --version | ||
| else | ||
| ./bin/protofetch --version | ||
| fi | ||
|
|
||
| - name: Stop HTTP server | ||
| if: always() | ||
| shell: bash | ||
| run: | | ||
| if [ -f /tmp/http_server.pid ]; then | ||
| kill $(cat /tmp/http_server.pid) || true | ||
| fi | ||
|
|
||
| release: | ||
| runs-on: ubuntu-latest | ||
| if: github.repository == 'coralogix/protofetch' && startsWith(github.ref, 'refs/tags/') | ||
| needs: [ package ] | ||
| needs: [ package, test-npm-package ] | ||
| env: | ||
| CRATES_IO_TOKEN: ${{ secrets.CRATES_IO_TOKEN }} | ||
| NPM_TOKEN: ${{ secrets.NPM_ACCESS_TOKEN }} | ||
|
|
||
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.
please use Node's built in.