[Bug]: Building by some frameworks using esbuild internaly can be non-hermetic and broken

pddg commented 1 year ago

What happened?

I am trying to use the remix framework. Referring to the example of Next.js, I was able to run the remix build but without success.

I have tried to analyze this problem in the sandbox using --sandbox_debug. As a result, I found that the problem is caused by a difference between the path returned by esbuild, which is used internally by the remix, and the path seen by the remix itself.

remix sees: /private/var/tmp/_bazel_pddg/319ec592a1cf9c9e7aab0272fcef4948/sandbox/darwin-sandbox/163/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/*
esbuild returns: /private/var/tmp/_bazel_pddg/319ec592a1cf9c9e7aab0272fcef4948/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/*

esbuild is written in Go, so it is not affected by patch_node_fs = True. On the other hand, remix cannot resolve symlinks since the patch prevent to it, so these paths that actually point to the same file will appear as if they are different files to the remix.

This is a problem that can occur not only with remix, but also with frameworks that use esbuild internally or compilers that run on runtimes other than nodejs.

Version

Development (host) and target OS/architectures:

macOS 13.1/arm64
Ubuntu 22.04/amd64

Output of bazel --version:

❯ bazel --version                             
bazel 6.0.0

Version of the Aspect rules, or other relevant rules from your WORKSPACE or MODULE.bazel file: rules_js v1.13.0

Language(s) and/or frameworks involved:

nodejs
esbuild 0.16.14
remix 1.9.0

How to reproduce

Minimum repro repository is here: https://github.com/pddg/rules_js_repro/tree/esbuild_non_hermetic

An example of reproducing a failing remix build is here: https://github.com/pddg/rules_js_repro/tree/remix_fails

Any other information?

I set patch_node_fs = False and applied a few small patches to make sure the remix build would succeed.

However, this is undesirable because it results in a non-hermetic build.

Fund our work

[ ] Sponsor our open source work by donating a bug bounty

mattem commented 1 year ago

Thanks for the report. This sounds like the same issue that's being tracked in the rules_esbuild repo

https://github.com/aspect-build/rules_esbuild/issues/58

juanpmarin commented 1 year ago

Hi @pddg , I'm experiencing the sames issues

Could you share the patches that you applied please? That would be very helpful

Thanks!

pddg commented 1 year ago

@juanpmarin I am building for cloudflare-pages, so builds for other targets may have different issues. My patch is as follows. Even though the paths are different, they point to the same file, exploiting the fact that it's dev and inode are the same. This worked with patch_node_fs = True.

--- dist/compiler/assets.js 2022-12-10 10:45:45.551670539 +0900
+++ dist/compiler/assets.js 2022-12-10 15:11:02.721211338 +0900
@@ -12,6 +12,7 @@ Object.defineProperty(exports, '__esModule', { value: 

 Object.defineProperty(exports, '__esModule', { value: true });

+var fs = require('fs');
 var path = require('path');
 var invariant = require('../invariant.js');
 var routeExports = require('./routeExports.js');
@@ -36,6 +37,7 @@ function _interopNamespace(e) {
   return Object.freeze(n);
 }

+var fs__namespace = /*#__PURE__*/_interopNamespace(fs);
 var path__namespace = /*#__PURE__*/_interopNamespace(path);

 async function createAssetsManifest(config, metafile) {
@@ -46,6 +48,15 @@ async function createAssetsManifest(config, metafile) 
     return imports.filter(im => im.kind === "import-statement").map(im => resolveUrl(im.path));
   }
   let entryClientFile = path__namespace.resolve(config.appDirectory, config.entryClientFile);
+  let entryClientFileStat = fs__namespace.statSync(entryClientFile);
+  function isEntryClientFile(path) {
+    try {
+      let givenStat = fs__namespace.statSync(path);
+      return entryClientFileStat.dev === givenStat.dev && entryClientFileStat.ino === givenStat.ino;
+    } catch(e) {
+      return false;
+    }
+  }
   let routesByFile = Object.keys(config.routes).reduce((map, key) => {
     let route = config.routes[key];
     map.set(route.file, map.has(route.file) ? [...map.get(route.file), route] : [route]);
@@ -56,7 +67,7 @@ async function createAssetsManifest(config, metafile) 
   for (let key of Object.keys(metafile.outputs).sort()) {
     let output = metafile.outputs[key];
     if (!output.entryPoint) continue;
-    if (path__namespace.resolve(output.entryPoint) === entryClientFile) {
+    if (isEntryClientFile(output.entryPoint)) {
       entry = {
         module: resolveUrl(key),
         imports: resolveImports(output.imports)

This is patching transpiled javascript, which can be easily broken. Save it as @remix-run_dev_1.11.1.patch and put it into patches directory, then apply the patch in npm_translate_lock.

npm_translate_lock(
    name = "npm",
    npmrc = "//:.npmrc",
    pnpm_lock = "//:pnpm-lock.yaml",
    verify_node_modules_ignored = "//:.bazelignore",
    bins = {
        "@remix-run/dev": {
            "remix": "dist/cli.js",
        },
    },
    patches = {
        "@remix-run/dev@1.11.1": ["//patches:@remix-run_dev_1.11.1.patch"],
    },
}

If there is a better way to do this, please let me know.

juanpmarin commented 1 year ago

Awesome, thanks @pddg !

juanpmarin commented 1 year ago

@pddg are you using pnpm workspaces for your usecase? I can only make this work if I install remix and it's dependencies in the root project, what about you?

pddg commented 1 year ago

@juanpmarin

install remix and it's dependencies in the root project

I have adopted this approach. I have not yet found a way to install and run it in individual workspaces :(

pddg commented 1 year ago

@juanpmarin I have successfully made it work using pnpm workspace. https://github.com/pddg/rules_js_repro/tree/remix_in_workspace

https://github.com/aspect-build/rules_js/pull/773 helped me.

juanpmarin commented 1 year ago

@pddg thank you for letting me know!

aspect-build / rules_js