Different strategy for installing platform-specific binaries #789
Comments
eduardoboucas
changed the title
I have considered implementing the install script this way but I didn't go down that route because if I recall correctly it makes the installation output really noisy with all of the errors about unmet optional dependencies, which is a poor user experience. I'm also not totally sure what the guarantees around optional dependencies are (since I need them to be required, not optional) and if there are any situations where that could backfire and leave you with a broken package. Especially with all of the various alternate package managers other than npm that people like to use. Having my own install script gives me more control over these things. I'd really rather not change the whole approach of the install script at this point because it's undergone a lot of evolution and has accumulated a lot of special cases that solve real problems. It's relatively stable at this point and changing it would risk breaking people, and would likely be a ton of work for me since I'd expect dozens of support requests from the fallout. It's at least not something I should do without a breaking change in the version number. Depending on what you're doing, one possibility could be for you to just make your own package with all of esbuild's platform-specific packages as optional dependencies as you suggested. That should let you do this without any changes to esbuild itself. Doing this would at least be a less disruptive way of trying out the approach than involving esbuild's whole user base in the experiment. |
|
Thanks for the additional context, @evanw. I completely understand your position. If we do decide to explore this experiment, we'll make sure to report back with our findings, so that you can decide whether this is something you want to revisit in the future. Thanks for your work. 🙌🏻 |
After looking into this for a bit, I think the main challenge would be to ensure that our package would benefit from any updates made upstream. We love esbuild and are excited to get the upcoming updates, so we were hoping to avoid anything that feels like a fork. The Would you be open to having a separate npm module that published from this repository, alongside the others? It would essentially be a copy of https://github.com/evanw/esbuild/blob/master/npm/esbuild/package.json, but with the If done from this repo, we would ensure that this module would include the latest changes, and it should be just a matter of adding an additional item to the Makefile. Thanks! |
|
I think it's worth getting an experimental package up with an example of this technique to try out. That should make it possible to test it with various package managers, operating systems, and different setting configurations and see what breaks. |
|
I wanted to record my own experience with this approach. I've authored sorvor with platform specific packages as This setup is mostly working on Mac and Linux, except few instances:
I still did not test this with yarn berry or windows (I don't have windows machine unfortunately). PS: |
|
Thanks very much for the report. It's good to have this additional data.
This seems like a pretty significant shortcoming. It seems like this means the approach just won't work after all. Is that correct? |
It is indeed an issue with npm 7. I have filed an issue and there's already a pull request for it, so it should be fixed with the next release of the npm CLI. Regardless, as per the release notes of npm 7, «scripts are always run with the effective uid and gid of the working directory owner», which means that the permissions errors I reported should disappear. We've had users confirming that the errors are gone when installing with npm 7. |
This is too early to call. As @eduardoboucas mentioned, the linked issue with NPM V7 is marked as closed and the fix will be available in next release. NPM V7 makes this even more better, because user's do not see optional dependency warnings as observed with NPM V6 client as confirmed here |
|
I came here from |
|
Note to self from #1434: Another thing to investigate here is to see how various JS package managers handle optional dependencies when there are multiple versions of esbuild installed simultaneously. It would be very bad for two different esbuild packages to be installed but for the optional dependencies to either only be installed once or not be installed at all. |
|
I just published a package called I have installed these packages with npm v7, pnpm, and yarn and they all appear to work (at least on macOS, there may be problems on Windows as usual). I also had to add hacks for Yarn's PnP mode which has a virtual file system that messes with stuff but it also appears to work under Yarn PnP after the hacks. These new packages even work with The major drawback appears to be that everything in What do people on this thread think of this experiment? Are people with installation troubles (alternative registries, complex proxies, offline installations, etc.) able to install these experimental packages? |
|
We haven't tried option to guard the download with a lightweight shim package, which could solve the bandwidth problem but not the noise problem. Ideal solution would be for the package managers to not attempt to download optionalDependencies if they are platform specific (as declared with the |
|
Interesting. Thanks for the pointers. I tried making a package that uses shims ( |
Latest updateAfter looking again it looks like Click to expand the install log for
|
|
This is an excellent news! Thanks for sharing the notes @evanw! @arcanis I looked through the issue tracker and the closest thing I found was yarnpkg/berry#2751 - this does seem to be related, but rather than reusing optional dependencies is proposing a new way to declare platform specific deps. Would yarn consider using optional deps instead in a way similar to how npm and pmp does it now? |
|
Optional dependencies matter for the build (as, make their build allowed to fail) but are still installed. This is the behavior we observed quite a long time ago, and while it never got spec'd I'm not thrilled by the idea of changing its semantic. (edit: more on that in yarnpkg/berry#3317 (comment)) It also doesn't work well for multi-system caching purposes (think osx in dev and Linux in ci), since each system would then only download their copy without being aware of the other that would still need to be in the local cache. We can keep discussing on the open issue on our repo, but I'll move it from a bug into a feature request. |
I assumed that Yarn has to at least support optional dependencies even if it introduces the package variants feature since many packages in the npm ecosystem already use optional dependencies, and Yarn wants to be compatible with the npm ecosystem. And Yarn does appear to support them, but in a seemingly sub-optimal way that downloads more data than necessary.
That did occur to me too. I was wondering how that works. It looks like npm only downloads the package for the current platform so I assumed npm's cross-platform caching use case had to work out somehow. But maybe npm's caching is actually broken in this area? Isn't that something that yarnpkg/berry#2751 would have to solve as well though? Or does that proposal still always download all packages for all platforms? If so, then the package variants feature wouldn't be helpful for esbuild's use case after all since I'm trying to avoid always downloading all packages for all platforms. I'm mentioning this since you asked about that proposal in relation to esbuild in #1154.
Sounds good, makes sense 👍 |
|
I just ran some more thorough tests on
Some thoughts:
To reiterate for clarity: The benefits of this approach vs. the current install script approach are:
The drawbacks of this approach vs. the current install script approach are:
Click to expand the code for the script that generated this table (warning: macOS only, deletes cache directories in your home folder, run at your own risk)// This is meant to be run on macOS
const child_process = require('child_process')
const fs = require('fs')
function run(command) {
console.log('$ ' + command)
child_process.execFileSync('sh', ['-c', command], {
stdio: 'inherit',
cwd: __dirname,
})
}
function runAndCheck(command) {
console.log('$ ' + command)
const stdout = child_process.execFileSync('sh', ['-c', command], {
stdio: ['inherit', 'pipe', 'inherit'],
cwd: __dirname,
})
return stdout.toString()
}
function cleanProject() {
run('rm -fr package-lock.json node_modules yarn.lock .yarn .yarnrc.yml pnpm-lock.yaml .pnp.cjs .pnpm-debug.log yarn-error.log')
}
function cleanPackageManager() {
run('rm -fr ~/.pnpm-store ~/Library/Caches/Yarn/v6 ~/.yarn/berry/cache')
run('npm cache clean --force')
}
function runPackageManager(commands, extras) {
console.log('-'.repeat(80))
cleanProject()
cleanPackageManager()
fs.writeFileSync(__dirname + '/entry.js',
`console.log(require('esbuild-experimental-optdeps').transformSync('1+2').code.trim())`)
fs.writeFileSync(__dirname + '/package.json', JSON.stringify({
scripts: { 'check': 'esbuild --version' },
dependencies: { 'esbuild-experimental-optdeps': '0.12.3' },
}, null, 2))
const runCommand = commands.pop()
const installCommand = commands.pop()
for (const cmd of commands) run(cmd)
const commandName = installCommand.split(' ')[0]
const version = commandName + '@' + runAndCheck(commandName + ' --version').trim()
const beforeColdInstall = Date.now()
run(installCommand)
const coldInstallTime = Date.now() - beforeColdInstall
let warmInstallTime = 0
for (let i = 0; i < 3; i++) {
cleanProject()
const beforeWarmInstall = Date.now()
run(installCommand)
warmInstallTime += Date.now() - beforeWarmInstall
}
warmInstallTime /= 3
const output = runAndCheck(runCommand)
const jsApiWorks = output === '1 + 2;\n'
let binPathWorks = false
try {
run('npm run check')
binPathWorks = true
} catch (e) {
}
return {
version,
coldInstallTime,
warmInstallTime,
jsApiWorks,
binPathWorks,
downloadAll: false,
logSpam: false,
...extras,
}
}
const packageManagerRunners = {
npm6: () => runPackageManager([
'npm i -g npm@6.14.14',
'npm install',
'node entry.js',
], {
downloadAll: true,
logSpam: true,
}),
npm7: () => runPackageManager([
'npm i -g npm@7.20.3',
'npm install',
'node entry.js',
], {
}),
pnpmLatest: () => runPackageManager([
'npm i -g pnpm@6.13.0',
'pnpm install',
'node entry.js',
], {
downloadAll: true,
}),
pnpmNext: () => runPackageManager([
'npm i -g pnpm@6.14.0-0',
'pnpm install',
'node entry.js',
], {
}),
yarn1: () => runPackageManager([
'yarn',
'node entry.js',
], {
downloadAll: true,
logSpam: true,
}),
yarn2: () => runPackageManager([
'yarn set version berry',
'yarn set version 2',
'yarn',
'yarn node entry.js',
], {
downloadAll: true,
}),
yarn3: () => runPackageManager([
'yarn set version berry',
'yarn set version 3',
'yarn',
'yarn node entry.js',
], {
downloadAll: true,
}),
}
const columns = {
version: 'Package manager',
coldInstallTime: 'Cold install time',
warmInstallTime: 'Warm install time',
jsApiWorks: 'JS API works',
binPathWorks: 'CLI works',
downloadAll: 'Downloads extra data',
logSpam: 'Spams irrelevant logs',
}
const table = []
function addRow(row) {
console.log(row)
table.push(row)
}
addRow('| ' + Object.values(columns).join(' | ') + ' |')
addRow('|' + Object.values(columns).map(() => '---').join('|') + '|')
for (const name in packageManagerRunners) {
const runner = packageManagerRunners[name]
const result = runner()
result.version = '`' + result.version + '`'
result.coldInstallTime = `${(result.coldInstallTime / 1000).toFixed(1)}s`
result.warmInstallTime = `${(result.warmInstallTime / 1000).toFixed(1)}s`
result.jsApiWorks = result.jsApiWorks ? '✅' : '🚫'
result.binPathWorks = result.binPathWorks ? '✅' : '🚫'
result.downloadAll = result.downloadAll ? '⚠️' : '✅'
result.logSpam = result.logSpam ? '⚠️' : '✅'
addRow('| ' + Object.keys(columns).map(k => result[k]).join(' | ') + ' |')
}
cleanProject()
console.log(table.join('\n')) |
|
Hi, just want to chime in, we're evaluating vite on a Jenkins CI/CD server, that has no access to the internet directly, only via a "npm-registry-proxy". So naturally npm installing vite gives an error: 14:37:57 Trying to install "esbuild-windows-64" using npm But testing with esbuild-experimental-optdeps@0.12.2 and 0.12.3 this works (installs without errors on our Jenkins CI/CD server, so that way of 'packaging' works) Any ideas on how to make this work with vite ? Thanks in advance! |
|
Hi, @evanw. |
Yes. Edit: The code for this is here: #1621. |
|
The PR #1621 implements this installation strategy and I believe I have fixed all issues. A test package has been published as Here are the latest results of my tests across various package managers:
Obviously yarn 2+ is doing something very inefficient but I have filed yarnpkg/berry#3317 about this and this is something yarn needs to fix. There isn't anything I can do about this on my end. At least warm install times (i.e. when that version of esbuild has already been installed once) aren't too bad. Click to expand the code for the script that generated this table (warning: macOS only, deletes cache directories in your home folder, run at your own risk)// This is meant to be run on macOS
const child_process = require('child_process')
const fs = require('fs')
function run(command) {
console.log('$ ' + command)
child_process.execFileSync('sh', ['-c', command], {
stdio: 'inherit',
cwd: __dirname,
})
}
function runAndCheck(command) {
console.log('$ ' + command)
const stdout = child_process.execFileSync('sh', ['-c', command], {
stdio: ['inherit', 'pipe', 'inherit'],
cwd: __dirname,
})
return stdout.toString()
}
function cleanProject() {
run('rm -fr package-lock.json node_modules yarn.lock .yarn .yarnrc.yml pnpm-lock.yaml .pnp.cjs .pnpm-debug.log yarn-error.log')
}
function cleanPackageManager() {
run('rm -fr ~/.pnpm-store ~/Library/Caches/Yarn/v6 ~/.yarn/berry/cache')
run('npm cache clean --force')
}
function runPackageManager(options) {
console.log('-'.repeat(80))
cleanProject()
cleanPackageManager()
fs.writeFileSync(__dirname + '/entry.js',
`console.log(require('esbuild-experimental-optdeps').transformSync('1+2').code.trim())`)
fs.writeFileSync(__dirname + '/package.json', JSON.stringify({
scripts: { 'check': 'esbuild --version' },
dependencies: { 'esbuild-experimental-optdeps': '0.12.10' },
}, null, 2))
for (const cmd of options.before) run(cmd)
const commandName = options.install.split(' ')[0]
const version = commandName + '@' + runAndCheck(commandName + ' --version').trim()
const beforeColdInstall = Date.now()
run(options.install)
const coldInstallTime = Date.now() - beforeColdInstall
let warmInstallTime = 0
for (let i = 0; i < 3; i++) {
cleanProject()
for (const cmd of options.before) run(cmd)
const beforeWarmInstall = Date.now()
run(options.install)
warmInstallTime += Date.now() - beforeWarmInstall
}
warmInstallTime /= 3
const output = runAndCheck(options.run)
const jsApiWorks = output === '1 + 2;\n'
let binPathWorks = false
try {
run(options.check)
binPathWorks = true
} catch (e) {
}
return {
version,
coldInstallTime,
warmInstallTime,
jsApiWorks,
binPathWorks,
downloadAll: false,
logSpam: false,
...options,
}
}
const packageManagerRunners = {
npm6: () => runPackageManager({
before: [
'npm i -g npm@6.14.15',
],
install: 'npm install',
run: 'node entry.js',
check: 'npm run check',
downloadAll: true,
logSpam: true,
}),
npm7: () => runPackageManager({
before: [
'npm i -g npm@7.24.0',
],
install: 'npm install',
run: 'node entry.js',
check: 'npm run check',
}),
pnpmLatest: () => runPackageManager({
before: [
'npm i -g pnpm@6.13.0',
],
install: 'pnpm install',
run: 'node entry.js',
check: 'pnpm run check',
downloadAll: true,
}),
pnpmNext: () => runPackageManager({
before: [
'npm i -g pnpm@6.15.1',
],
install: 'pnpm install',
run: 'node entry.js',
check: 'pnpm run check',
}),
yarn1: () => runPackageManager({
before: [
],
install: 'yarn',
run: 'node entry.js',
check: 'yarn run check',
downloadAll: true,
logSpam: true,
}),
yarn2: () => runPackageManager({
before: [
'yarn set version berry',
'yarn set version 2',
],
install: 'yarn',
run: 'yarn node entry.js',
check: 'yarn run check',
downloadAll: true,
}),
yarn3: () => runPackageManager({
before: [
'yarn set version berry',
'yarn set version 3',
],
install: 'yarn',
run: 'yarn node entry.js',
check: 'yarn run check',
downloadAll: true,
}),
}
const columns = {
version: 'Package manager',
coldInstallTime: 'Cold install time',
warmInstallTime: 'Warm install time',
jsApiWorks: 'JS API works',
binPathWorks: 'CLI works',
downloadAll: 'Downloads extra data',
logSpam: 'Spams irrelevant logs',
}
const table = []
function addRow(row) {
console.log(row)
table.push(row)
}
addRow('| ' + Object.values(columns).join(' | ') + ' |')
addRow('|' + Object.values(columns).map(() => '---').join('|') + '|')
for (const name in packageManagerRunners) {
const runner = packageManagerRunners[name]
const result = runner()
result.version = '`' + result.version + '`'
result.coldInstallTime = `${(result.coldInstallTime / 1000).toFixed(1)}s`
result.warmInstallTime = `${(result.warmInstallTime / 1000).toFixed(1)}s`
result.jsApiWorks = result.jsApiWorks ? '✅' : '🚫'
result.binPathWorks = result.binPathWorks ? '✅' : '🚫'
result.downloadAll = result.downloadAll ? '⚠️' : '✅'
result.logSpam = result.logSpam ? '⚠️' : '✅'
addRow('| ' + Object.keys(columns).map(k => result[k]).join(' | ') + ' |')
}
cleanProject()
console.log(table.join('\n')) |
|
Starting from cnpm@7.1.0, no additional data will be downloaded, and there will be no warnings 🎉. I added cnpm data:
How to install cnpm? npm install -g cnpm --registry=https://registry.npmmirror.com
cnpm i esbuild
du -sh node_modules/
7.4M node_modules/ (before: 118M)Thanks to my colleagues for their efforts, see also:
2021-10-26
|
|
|
|
Just wanted to mention that for those of you getting here and using pnpm you can simply run |
Hi! 👋🏻
We're looking to integrate esbuild with the Netlify CLI and some users are reporting permission-related errors when installing the application, which trace back to #369.
I understand that this is a result of the UID/GID switching that npm does when running scripts, which is problematic when the package is installed as root.
Rather than manually fetching the appropriate binary as part of the
postinstallscript, could theesbuildpackage list the various platform-specific packages asoptionalDependencies, and shift to npm itself the responsibility of figuring out which packages to installed based on thecpuandosproperties? We're doing this for one of our packages that also includes a Go binary and it works well.I'm happy to contribute with a pull request, but I wanted to check whether this is something you'd be interested in, or even if it's something you've already considered and discarded for a particular reason.
Thanks in advance!
The text was updated successfully, but these errors were encountered: