fix: remapping of relative-path imports marked `external` that don't correspond to a filesystem path in sandbox

[Strum355]

We have a ts_project target part of an npm_package that includes the following snippet:

const postcssConfig = process.env.BAZEL_BINDIR
    ? require('../../../../../../../../postcss.config') // if in bazel
    : require('../../../../postcss.config') // if outside bazel

When processed as part of an esbuild target, it will perform static analysis (not running the script) in order to resolve the imports, so it will check for both of them. The sandbox layout in the esbuild target is as follows for the file that imports psotcss.config.js and postcss.config.js itself:

sh-5.2$ find . -name 'stylePlugin.js'
./bazel-out/k8-fastbuild/bin/node_modules/.aspect_rules_js/@[email protected]/node_modules/@sourcegraph/build-config/src/esbuild/stylePlugin.js # hence the 'true' side of the ternary when running inside bazel
./bazel-out/k8-fastbuild/bin/client/build-config/src/esbuild/stylePlugin.js # hence the 'false' side of the ternary when running outside of bazel
sh-5.2$ find . -name 'postcss.config.js'
./bazel-out/k8-fastbuild/bin/postcss.config.js

As the latter side of the ternary can't be resolved (due to the sandbox layout), we have ../../../../postcss.config marked as external in the esbuild target. The bazel sandbox plugin introduced in ~0.17.x will incorrectly reject this import as being outside BAZEL_BINDIR. This doesn't make sense, given that ../../../../postcss.config should be even less outside of the sandbox vs ../../../../../../../../postcss.config which doesnt get rejected. Looking further at the sandbox plugin, we can see that paths get rejected if they don't contain the BAZEL_BINDIR string. Adding some extra logging statements (see patch below) will show us whats going wrong.

diff --git a/esbuild/private/plugins/bazel-sandbox.js b/esbuild/private/plugins/bazel-sandbox.js
index 84f0921..f523f98 100644
--- a/esbuild/private/plugins/bazel-sandbox.js
+++ b/esbuild/private/plugins/bazel-sandbox.js
@@ -64,6 +64,9 @@ async function resolveInExecroot(build, importPath, otherOptions) {

   // If esbuild attempts to leave the execroot, map the path back into the execroot.
   if (!result.path.startsWith(execroot)) {
+    console.error(
+      `DEBUG: [bazel-sandbox] path ${result.path} is outside execroot ${execroot}`
+    )
     // If it tried to leave bazel-bin, error out completely.
     if (!result.path.includes(bindir)) {
       throw new Error(

With this log, we get two log statements for postcss.config (one for each side of the ternary:

For the accepted path: DEBUG: [bazel-sandbox] path /home/noah/.cache/bazel/_bazel_noah/8fd1d20666a46767e7f29541678514a0/execroot/__main__/bazel-out/k8-fastbuild/bin/postcss.config.js is outside execroot /home/noah/.cache/bazel/_bazel_noah/8fd1d20666a46767e7f29541678514a0/sandbox/linux-sandbox/9481/execroot/__main__ For the rejected path: DEBUG: [bazel-sandbox] path ../../../../postcss.config is outside execroot /home/noah/.cache/bazel/_bazel_noah/8fd1d20666a46767e7f29541678514a0/sandbox/linux-sandbox/9470/execroot/__main__

So the rejected path doesn't become an absolute path, so it can never pass the check for containing the BAZEL_BINDIR string even though it would be within the execroot if we actually had an absolute path. If this path wasnt marked external it would be rejected at the point of calling build.resolve.

To address this, we assume that if we don't have an absolute path, then we're dealing with a path that 1) couldn't be resolved by esbuild to an actual path but 2) didn't error due to being marked external. Therefore, we build an absolute path for it by resolving it relative to the file that attempts to import this path (which can be gotten with otherOptions.importer), and then re-attempt to perform correction of the path to remap it within the sandbox

Changes are visible to end-users: no

Test plan

• Manual testing; please provide instructions so we can reproduce:

The following commit contains this patch to rules_esbuild that you can comment out to reproduce the issue: https://github.com/sourcegraph/sourcegraph/commit/cf2a1c8856f The target to build is //client/web/dev:esbuild-config-production_bundle

[CLAassistant]

All committers have signed the CLA.

[jbedard]

Would it be possible to add a test for this?