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.

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

Add ESBUILD_BIN_PATH #881

Merged
merged 1 commit into from
Feb 25, 2021
Merged

Add ESBUILD_BIN_PATH #881

merged 1 commit into from
Feb 25, 2021

Conversation

sushain97
Copy link
Contributor

Installation already appears to support a couple modes:

  • ESBUILD_BIN_PATH_FOR_TESTS which symlinks an esbuild binary into node_modules
  • installBinaryFromPackage which either installs from a local cache or downloads the binary from npm (directly or via npm)

However, this doesn't match our needs exactly. In particular, we run yarn install in --offline mode inside a micro VM without network access. Thus, we need a way to tell the esbuild installer about a binary that we've already downloaded. Since we need the node_modules to be standalone, ESBUILD_BIN_PATH_FOR_TESTS doesn't work since it creates a symlink.

We handle a similar issue with node-sass by using npm_config_sass_binary_cache. However, the local caching logic in esbuild is a little more involved and I'm loathe to attempt replicating it our end (especially since we support both darwin and linux -- see the hack mentioned later).

So, there are two potential solutions:

  • Support ESBUILD_CACHE_PATH which overrides the cache directly
  • Support ESBUILD_BIN_PATH which acts just like ESBUILD_BIN_PATH_FOR_TESTS but copies the file instead

I decided on the latter since that resolves the problem directly (although I do need to test this change with our build system... opening the PR early to see if there's any appetite for this tweak).

For completeness, my current hack is:

mkdir -p ~/.cache/esbuild/bin/
cp external/esbuild/bin/esbuild ~/.cache/esbuild/bin/[email protected]

(within the yarn install Bazel action)

@evanw
Copy link
Owner

evanw commented Feb 24, 2021

I'm open to doing something like this but I'd like to reserve the right to break hacks like this this in the future. This is a super specific request and the implementation details of the installer isn't something I intended to be part of esbuild's public API. For example, in the future the installer may be changed to an entirely different approach such as #789, at which point anything relying on this will break.

@sushain97
Copy link
Contributor Author

That's fair! I'm sure we can adjust our wrappers accordingly if it breaks.

While this use case is certainly rare, it's definitely something which folks have invested in: https://classic.yarnpkg.com/blog/2016/11/24/offline-mirror/. The current installation setup doesn't allow for offline installs unless you guess the cache path.

My skimming of #789 suggests that if esbuild switched to an approach where the package manager itself downloads the deps, the downloads would be no different from any other npm module and everything would Just Work.

@evanw
Copy link
Owner

evanw commented Feb 25, 2021

FYI I'm going to change ESBUILD_BIN_PATH to ESBUILD_BINARY_PATH since that is already supported for overriding the binary path of the JavaScript API code at run-time (see #597).

@evanw evanw merged commit f88eec1 into evanw:master Feb 25, 2021
@sushain97
Copy link
Contributor Author

FYI I'm going to change ESBUILD_BIN_PATH to ESBUILD_BINARY_PATH since that is already supported for overriding the binary path of the JavaScript API code at run-time (see #597).

👍 Thanks for merging!

jtojnar added a commit to jtojnar/nixfiles that referenced this pull request Sep 8, 2021
It now requires esbuild, which wants to download stuff as part of install script.
Fortunately, it can be bypassed by an environment variable.

evanw/esbuild#881

Also, annoyingly, Nixpkgs’s Go infrastructure does not support overriding attributes of buildGoPackage:

NixOS/nixpkgs#86349
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.

2 participants