Skip to content
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.

Sign up for GitHub

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

Jump to bottom

win,install: only download target_arch node.lib #2857

Merged
merged 1 commit into from
Jun 9, 2023
Merged

win,install: only download target_arch node.lib #2857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 93 additions & 76 deletions lib/install.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ const streamPipeline = util.promisify(stream.pipeline)

async function install (fs, gyp, argv) {
const release = processRelease(argv, gyp, process.version, process.release)
// Detecting target_arch based on logic from create-cnfig-gyp.js. Used on Windows only.
StefanStojanovic marked this conversation as resolved.
Show resolved Hide resolved
const arch = win ? (gyp.opts.target_arch || gyp.opts.arch || process.arch || 'ia32') : ''
// Used to prevent downloading tarball if only new node.lib is required on Windows.
let shouldDownloadTarball = true

// Determine which node dev files version we are installing
log.verbose('install', 'input version string %j', release.version)
Expand Down Expand Up @@ -92,6 +96,26 @@ async function install (fs, gyp, argv) {
}
}
log.verbose('install', 'version is good')
if (win) {
log.verbose('on Windows; need to check node.lib')
const nodeLibPath = path.resolve(devDir, arch, 'node.lib')
try {
await fs.promises.stat(nodeLibPath)
} catch (err) {
if (err.code === 'ENOENT') {
log.verbose('install', `version not already installed for ${arch}, continuing with install`, release.version)
StefanStojanovic marked this conversation as resolved.
Show resolved Hide resolved
try {
shouldDownloadTarball = false
return await go()
} catch (err) {
return rollback(err)
}
} else if (err.code === 'EACCES') {
return eaccesFallback(err)
}
throw err
}
}
} else {
try {
return await go()
Expand Down Expand Up @@ -179,66 +203,69 @@ async function install (fs, gyp, argv) {
}

// download the tarball and extract!
// Ommited on Windows if only new node.lib is required

// on Windows there can be file errors from tar if parallel installs
// are happening (not uncommon with multiple native modules) so
// extract the tarball to a temp directory first and then copy over
const tarExtractDir = win ? await fs.promises.mkdtemp(path.join(os.tmpdir(), 'node-gyp-tmp-')) : devDir

try {
if (tarPath) {
await tar.extract({
file: tarPath,
strip: 1,
filter: isValid,
onwarn,
cwd: tarExtractDir
})
} else {
try {
const res = await download(gyp, release.tarballUrl)
if (shouldDownloadTarball) {
if (tarPath) {
await tar.extract({
file: tarPath,
strip: 1,
filter: isValid,
onwarn,
cwd: tarExtractDir
})
} else {
try {
const res = await download(gyp, release.tarballUrl)

if (res.status !== 200) {
throw new Error(`${res.status} response downloading ${release.tarballUrl}`)
}
if (res.status !== 200) {
throw new Error(`${res.status} response downloading ${release.tarballUrl}`)
}

await streamPipeline(
res.body,
// content checksum
new ShaSum((_, checksum) => {
const filename = path.basename(release.tarballUrl).trim()
contentShasums[filename] = checksum
log.verbose('content checksum', filename, checksum)
}),
tar.extract({
strip: 1,
cwd: tarExtractDir,
filter: isValid,
onwarn
})
)
} catch (err) {
await streamPipeline(
res.body,
// content checksum
new ShaSum((_, checksum) => {
const filename = path.basename(release.tarballUrl).trim()
contentShasums[filename] = checksum
log.verbose('content checksum', filename, checksum)
}),
tar.extract({
strip: 1,
cwd: tarExtractDir,
filter: isValid,
onwarn
})
)
} catch (err) {
// something went wrong downloading the tarball?
if (err.code === 'ENOTFOUND') {
throw new Error('This is most likely not a problem with node-gyp or the package itself and\n' +
if (err.code === 'ENOTFOUND') {
throw new Error('This is most likely not a problem with node-gyp or the package itself and\n' +
'is related to network connectivity. In most cases you are behind a proxy or have bad \n' +
'network settings.')
}
throw err
}
throw err
}
}

// invoked after the tarball has finished being extracted
if (extractErrors || extractCount === 0) {
throw new Error('There was a fatal problem while downloading/extracting the tarball')
}
// invoked after the tarball has finished being extracted
if (extractErrors || extractCount === 0) {
throw new Error('There was a fatal problem while downloading/extracting the tarball')
}

log.verbose('tarball', 'done parsing tarball')
log.verbose('tarball', 'done parsing tarball')
}

const installVersionPath = path.resolve(tarExtractDir, 'installVersion')
await Promise.all([
// need to download node.lib
...(win ? downloadNodeLib() : []),
// need to download node.lib
...(win ? [downloadNodeLib()] : []),
// write the "installVersion" file
fs.promises.writeFile(installVersionPath, gyp.package.installVersion + '\n'),
// Only download SHASUMS.txt if we downloaded something in need of SHA verification
Expand Down Expand Up @@ -293,43 +320,33 @@ async function install (fs, gyp, argv) {
log.verbose('checksum data', JSON.stringify(expectShasums))
}

function downloadNodeLib () {
async function downloadNodeLib () {
log.verbose('on Windows; need to download `' + release.name + '.lib`...')
const archs = ['ia32', 'x64', 'arm64']
return archs.map(async (arch) => {
const dir = path.resolve(tarExtractDir, arch)
const targetLibPath = path.resolve(dir, release.name + '.lib')
const { libUrl, libPath } = release[arch]
const name = `${arch} ${release.name}.lib`
log.verbose(name, 'dir', dir)
log.verbose(name, 'url', libUrl)

await fs.promises.mkdir(dir, { recursive: true })
log.verbose('streaming', name, 'to:', targetLibPath)

const res = await download(gyp, libUrl)

if (res.status === 403 || res.status === 404) {
if (arch === 'arm64') {
// Arm64 is a newer platform on Windows and not all node distributions provide it.
log.verbose(`${name} was not found in ${libUrl}`)
} else {
log.warn(`${name} was not found in ${libUrl}`)
}
return
} else if (res.status !== 200) {
throw new Error(`${res.status} status code downloading ${name}`)
}
const dir = path.resolve(tarExtractDir, arch)
const targetLibPath = path.resolve(dir, release.name + '.lib')
const { libUrl, libPath } = release[arch]
const name = `${arch} ${release.name}.lib`
log.verbose(name, 'dir', dir)
log.verbose(name, 'url', libUrl)

await fs.promises.mkdir(dir, { recursive: true })
log.verbose('streaming', name, 'to:', targetLibPath)

const res = await download(gyp, libUrl)

// Since only required node.lib is downloaded throw error if it is not fetched
if (res.status !== 200) {
throw new Error(`${res.status} status code downloading ${name}`)
}

return streamPipeline(
res.body,
new ShaSum((_, checksum) => {
contentShasums[libPath] = checksum
log.verbose('content checksum', libPath, checksum)
}),
fs.createWriteStream(targetLibPath)
)
})
return streamPipeline(
res.body,
new ShaSum((_, checksum) => {
contentShasums[libPath] = checksum
log.verbose('content checksum', libPath, checksum)
}),
fs.createWriteStream(targetLibPath)
)
} // downloadNodeLib()
} // go()

Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"gyp"
],
"version": "9.3.1",
"installVersion": 10,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A question for people with more experience in node-gyp: Since a PR that changed installVersion from 9 to 10 landed yesterday in aaa117c do I need to increase it to 11 now, or not, because there was no release with installVersion: 10?

Copy link
Contributor

Choose a reason for hiding this comment

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

A question for people with more experience in node-gyp

I'm not more experienced, but my 2 cents: it's probably unnecessary for end users (since as you stated, no release went out), but AFAIK there's no harm in bumping it again, and it will prevent a situation where anyone who tested main (so folks working on node-gyp like you and I, or maybe even someone some where consuming node-gyp@main for whatever reason) and got their cache populated for installVersion: 10 won't end up in a broken situation.

So to me bumping it to 11 seems reasonable to prevent any problems.

"installVersion": 11,
"author": "Nathan Rajlich <nathan@tootallnate.net> (http://tootallnate.net)",
"repository": {
"type": "git",
Expand Down
2 changes: 1 addition & 1 deletion test/test-download.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ describe('download', function () {
await util.promisify(install)(prog, [])

const data = await fs.promises.readFile(path.join(expectedDir, 'installVersion'), 'utf8')
assert.strictEqual(data, '10\n', 'correct installVersion')
assert.strictEqual(data, '11\n', 'correct installVersion')

const list = await fs.promises.readdir(path.join(expectedDir, 'include/node'))
assert.ok(list.includes('common.gypi'))
Expand Down