fix(builtin): fix npm_install & yarn_install post_install_patches whe…

…n symlink_node_modules is enabled

WORKSPACE

@@ -18,6 +18,8 @@ workspace(
         "@angular_deps": ["packages/angular/node_modules"],
         # cypress_deps must be a managed directory to ensure it is downloaded before cypress_repositories is run.
         "@cypress_deps": ["packages/cypress/test/node_modules"],
+        "@internal_npm_install_test_patches_npm_symlinked": ["internal/npm_install/test/patches_npm_symlinked/node_modules"],
+        "@internal_npm_install_test_patches_yarn_symlinked": ["internal/npm_install/test/patches_yarn_symlinked/node_modules"],
         "@internal_test_multi_linker_sub_deps": ["internal/linker/test/multi_linker/sub/node_modules"],
         "@npm": ["node_modules"],
         "@npm_node_patches": ["packages/node-patches/node_modules"],

internal/npm_install/generate_build_file.ts

@@ -62,7 +62,7 @@ let config: any = {
   package_path: '',
   rule_type: 'yarn_install',
   strict_visibility: true,
-  workspace_root_prefix: '',
+  workspace_rerooted_path: '',
   workspace: '',
 };
 
@@ -113,7 +113,6 @@ function createFileSymlinkSync(target: string, p: string) {
  */
 export function main() {
   config = require('./generate_config.json')
-  config.workspace_root_base = config.workspace_root_prefix?.split('/')[0];
   config.limited_visibility = `@${config.workspace}//:__subpackages__`;
 
   if (config.exports_directories_only) {
@@ -136,7 +135,7 @@ export function main() {
   generateBuildFiles(pkgs)
 
   // write a .bazelignore file
-  writeFileSync('.bazelignore', `node_modules\n${config.workspace_root_base}`);
+  writeFileSync('.bazelignore', `node_modules\n${config.workspace_rerooted_path}`);
 }
 
 /**

internal/npm_install/index.js

@@ -20,7 +20,7 @@ let config = {
     package_path: '',
     rule_type: 'yarn_install',
     strict_visibility: true,
-    workspace_root_prefix: '',
+    workspace_rerooted_path: '',
     workspace: '',
 };
 function generateBuildFileHeader(visibility = PUBLIC_VISIBILITY) {
@@ -49,9 +49,7 @@ function createFileSymlinkSync(target, p) {
     fs.symlinkSync(target, p, 'file');
 }
 function main() {
-    var _a;
     config = require('./generate_config.json');
-    config.workspace_root_base = (_a = config.workspace_root_prefix) === null || _a === void 0 ? void 0 : _a.split('/')[0];
     config.limited_visibility = `@${config.workspace}//:__subpackages__`;
     if (config.exports_directories_only) {
         NODE_MODULES_PACKAGE_NAME = '$node_modules_dir$';
@@ -61,7 +59,7 @@ function main() {
     flattenDependencies(pkgs);
     generateBazelWorkspaces(pkgs);
     generateBuildFiles(pkgs);
-    writeFileSync('.bazelignore', `node_modules\n${config.workspace_root_base}`);
+    writeFileSync('.bazelignore', `node_modules\n${config.workspace_rerooted_path}`);
 }
 exports.main = main;
 function generateBuildFiles(pkgs) {

internal/npm_install/npm_install.bzl

@@ -252,15 +252,27 @@ directory. This is planned for 4.0: https://github.com/bazelbuild/rules_nodejs/i
 
 This can be used to make changes to installed packages after the package manager runs.
 
-File paths in patches should be relative to workspace root.""",
+File paths in patches should be relative to workspace root.
+
+Use with caution when `symlink_node_modules` enabled as the patches will run in your workspace and
+will modify files in your workspace.
+
+NB: If `symlink_node_modules` is enabled, the node_modules folder is re-used between executions of the
+    repository rule. Patches may be re-applied to files in this case and fail to apply. A marker file
+    `node_modules/.bazel-post-install-patches` is left in this mode when patches are applied. When the 
+    marker file is detected, patch file failures are treated as WARNINGS. For this reason, it is recommended
+    to patch npm packages with an npm tool such as https://www.npmjs.com/package/patch-package when
+    `symlink_node_modules` is enabled which handles re-apply patching logic more robustly.""",
     ),
     "pre_install_patches": attr.label_list(
         doc = """Patch files to apply before running package manager.
 
 This can be used to make changes to package.json or other data files passed in before running the
 package manager.
 
-File paths in patches should be relative to workspace root.""",
+File paths in patches should be relative to workspace root.
+
+Not supported with `symlink_node_modules` enabled.""",
     ),
     "quiet": attr.bool(
         default = True,
@@ -298,33 +310,81 @@ data attribute.
     ),
 })
 
-def _apply_patches(repository_ctx, patches):
+def _apply_pre_install_patches(repository_ctx):
+    if len(repository_ctx.attr.pre_install_patches) == 0:
+        return
+    if repository_ctx.attr.symlink_node_modules:
+        fail("pre_install_patches cannot be used with symlink_node_modules enabled")
+    _apply_patches(repository_ctx, _WORKSPACE_REROOTED_PATH, repository_ctx.attr.pre_install_patches)
+
+def _apply_post_install_patches(repository_ctx):
+    if len(repository_ctx.attr.post_install_patches) == 0:
+        return
+    if repository_ctx.attr.symlink_node_modules:
+        print("\nWARNING: @%s post_install_patches with symlink_node_modules enabled will run in your workspace and potentially modify source files" % repository_ctx.name)
+    working_directory = _user_workspace_root(repository_ctx) if repository_ctx.attr.symlink_node_modules else _WORKSPACE_REROOTED_PATH
+    marker_file = None
+    if repository_ctx.attr.symlink_node_modules:
+        marker_file = "%s/node_modules/.bazel-post-install-patches" % repository_ctx.path(repository_ctx.attr.package_json).dirname
+    _apply_patches(repository_ctx, working_directory, repository_ctx.attr.post_install_patches, marker_file)
+
+def _apply_patches(repository_ctx, working_directory, patches, marker_file = None):
     bash_exe = repository_ctx.os.environ["BAZEL_SH"] if "BAZEL_SH" in repository_ctx.os.environ else "bash"
 
     patch_tool = repository_ctx.attr.patch_tool
     if not patch_tool:
         patch_tool = "patch"
     patch_args = repository_ctx.attr.patch_args
 
-    for patchfile in patches:
-        command = "{patchtool} {patch_args} < {patchfile}".format(
-            patchtool = patch_tool,
-            patchfile = repository_ctx.path(patchfile),
-            patch_args = " ".join([
-                "'%s'" % arg
-                for arg in patch_args
-            ]),
-        )
+    for patch_file in patches:
+        if marker_file:
+            command = """{patch_tool} {patch_args} < {patch_file}
+CODE=$?
+if [ $CODE -ne 0 ]; then
+    CODE=1
+    if [ -f \"{marker_file}\" ]; then
+        CODE=2
+    fi
+fi
+echo '1' > \"{marker_file}\"
+exit $CODE""".format(
+                patch_tool = patch_tool,
+                patch_file = repository_ctx.path(patch_file),
+                patch_args = " ".join([
+                    "'%s'" % arg
+                    for arg in patch_args
+                ]),
+                marker_file = marker_file,
+            )
+        else:
+            command = "{patch_tool} {patch_args} < {patch_file}".format(
+                patch_tool = patch_tool,
+                patch_file = repository_ctx.path(patch_file),
+                patch_args = " ".join([
+                    "'%s'" % arg
+                    for arg in patch_args
+                ]),
+            )
+
+        if not repository_ctx.attr.quiet:
+            print("@%s appling patch file %s in %s" % (repository_ctx.name, patch_file, working_directory))
+            if marker_file:
+                print("@%s leaving patches marker file %s" % (repository_ctx.name, marker_file))
         st = repository_ctx.execute(
             [bash_exe, "-c", command],
             quiet = repository_ctx.attr.quiet,
             # Working directory is _ which is where all files are copied to and
             # where the install is run; patches should be relative to workspace root.
-            working_directory = "_",
+            working_directory = working_directory,
         )
         if st.return_code:
-            fail("Error applying patch %s:\n%s%s" %
-                 (str(patchfile), st.stderr, st.stdout))
+            # If return code is 2 (see bash snippet above) that means a marker file was found before applying patches;
+            # Treat patch failure as a warning in this case
+            if st.return_code == 2:
+                print("""\nWARNING: @%s failed to apply patch file %s in %s:\n%s%s
+This can happen with symlink_node_modules enabled since your workspace node_modules is re-used between executions of the repository rule.""" % (repository_ctx.name, patch_file, working_directory, st.stderr, st.stdout))
+            else:
+                fail("Error applying patch %s in %s:\n%s%s" % (str(patch_file), working_directory, st.stderr, st.stdout))
 
 def _create_build_files(repository_ctx, rule_type, node, lock_file, generate_local_modules_build_files):
     repository_ctx.report_progress("Processing node_modules: installing Bazel packages and generating BUILD files")
@@ -351,7 +411,7 @@ def _create_build_files(repository_ctx, rule_type, node, lock_file, generate_loc
             rule_type = rule_type,
             strict_visibility = repository_ctx.attr.strict_visibility,
             workspace = repository_ctx.attr.name,
-            workspace_root_prefix = _workspace_root_prefix(repository_ctx),
+            workspace_rerooted_path = _WORKSPACE_REROOTED_PATH,
         ),
     )
     repository_ctx.file("generate_config.json", generate_config_json)
@@ -377,24 +437,42 @@ def _add_scripts(repository_ctx):
         {},
     )
 
-def _workspace_root_path(repository_ctx, f):
-    segments = ["_"]
-    if f.package:
-        segments.append(f.package)
-    segments.append(f.name)
-    return "/".join(segments)
+# The directory in the external repository where we re-root the workspace by copying
+# package.json, lock file & data files to and running the package manager in the
+# folder of the package.json file.
+_WORKSPACE_REROOTED_PATH = "_"
 
-def _workspace_root_prefix(repository_ctx):
+# Returns the root of the user workspace. No built-in way to get
+# this but we can derive it from the path of the package.json file
+# in the user workspace sources.
+def _user_workspace_root(repository_ctx):
     package_json = repository_ctx.attr.package_json
-    segments = ["_"]
+    segments = []
     if package_json.package:
-        segments.append(package_json.package)
+        segments.extend(package_json.package.split("/"))
     segments.extend(package_json.name.split("/"))
     segments.pop()
-    return "/".join(segments) + "/"
+    user_workspace_root = repository_ctx.path(package_json).dirname
+    for i in segments:
+        user_workspace_root = user_workspace_root.dirname
+    return str(user_workspace_root)
+
+# Returns the path to a file within the re-rooted user workspace
+# under _WORKSPACE_REROOTED_PATH in this repo rule's external workspace
+def _rerooted_workspace_path(repository_ctx, f):
+    segments = [_WORKSPACE_REROOTED_PATH]
+    if f.package:
+        segments.append(f.package)
+    segments.append(f.name)
+    return "/".join(segments)
+
+# Returns the path to the package.json directory within the re-rooted user workspace
+# under _WORKSPACE_REROOTED_PATH in this repo rule's external workspace
+def _rerooted_workspace_package_json_dir(repository_ctx):
+    return str(repository_ctx.path(_rerooted_workspace_path(repository_ctx, repository_ctx.attr.package_json)).dirname)
 
 def _copy_file(repository_ctx, f):
-    to = _workspace_root_path(repository_ctx, f)
+    to = _rerooted_workspace_path(repository_ctx, f)
 
     # ensure the destination directory exists
     to_segments = to.split("/")
@@ -419,7 +497,7 @@ def _copy_file(repository_ctx, f):
         fail("cp -f {} {} failed: \nSTDOUT:\n{}\nSTDERR:\n{}".format(repository_ctx.path(f), to, result.stdout, result.stderr))
 
 def _symlink_file(repository_ctx, f):
-    repository_ctx.symlink(f, _workspace_root_path(repository_ctx, f))
+    repository_ctx.symlink(f, _rerooted_workspace_path(repository_ctx, f))
 
 def _copy_data_dependencies(repository_ctx):
     """Add data dependencies to the repository."""
@@ -450,7 +528,7 @@ def _symlink_node_modules(repository_ctx):
         )
     else:
         repository_ctx.symlink(
-            repository_ctx.path(_workspace_root_prefix(repository_ctx) + "node_modules"),
+            repository_ctx.path(_rerooted_workspace_package_json_dir(repository_ctx) + "/node_modules"),
             repository_ctx.path("node_modules"),
         )
 
@@ -489,7 +567,7 @@ def _npm_install_impl(repository_ctx):
     if repository_ctx.attr.symlink_node_modules:
         root = str(repository_ctx.path(repository_ctx.attr.package_json).dirname)
     else:
-        root = str(repository_ctx.path(_workspace_root_prefix(repository_ctx)))
+        root = str(repository_ctx.path(_rerooted_workspace_package_json_dir(repository_ctx)))
 
     # The entry points for npm install for osx/linux and windows
     if not is_windows_host:
@@ -525,7 +603,7 @@ cd /D "{root}" && "{npm}" {npm_args}
     _copy_data_dependencies(repository_ctx)
     _add_scripts(repository_ctx)
     _add_node_repositories_info_deps(repository_ctx)
-    _apply_patches(repository_ctx, repository_ctx.attr.pre_install_patches)
+    _apply_pre_install_patches(repository_ctx)
 
     result = repository_ctx.execute(
         [node, "pre_process_package_json.js", repository_ctx.path(repository_ctx.attr.package_json), "npm"],
@@ -571,7 +649,7 @@ cd /D "{root}" && "{npm}" {npm_args}
         fail("remove_npm_absolute_paths failed: %s (%s)" % (result.stdout, result.stderr))
 
     _symlink_node_modules(repository_ctx)
-    _apply_patches(repository_ctx, repository_ctx.attr.post_install_patches)
+    _apply_post_install_patches(repository_ctx)
 
     _create_build_files(repository_ctx, "npm_install", node, repository_ctx.attr.package_lock_json, repository_ctx.attr.generate_local_modules_build_files)
 
@@ -641,7 +719,7 @@ def _yarn_install_impl(repository_ctx):
     if repository_ctx.attr.symlink_node_modules:
         root = str(repository_ctx.path(repository_ctx.attr.package_json).dirname)
     else:
-        root = str(repository_ctx.path(_workspace_root_prefix(repository_ctx)))
+        root = str(repository_ctx.path(_rerooted_workspace_package_json_dir(repository_ctx)))
 
     # The entry points for npm install for osx/linux and windows
     if not is_windows_host:
@@ -684,7 +762,7 @@ cd /D "{root}" && "{yarn}" {yarn_args}
     _copy_data_dependencies(repository_ctx)
     _add_scripts(repository_ctx)
     _add_node_repositories_info_deps(repository_ctx)
-    _apply_patches(repository_ctx, repository_ctx.attr.pre_install_patches)
+    _apply_pre_install_patches(repository_ctx)
 
     result = repository_ctx.execute(
         [node, "pre_process_package_json.js", repository_ctx.path(repository_ctx.attr.package_json), "yarn"],
@@ -710,7 +788,7 @@ cd /D "{root}" && "{yarn}" {yarn_args}
         fail("yarn_install failed: %s (%s)" % (result.stdout, result.stderr))
 
     _symlink_node_modules(repository_ctx)
-    _apply_patches(repository_ctx, repository_ctx.attr.post_install_patches)
+    _apply_post_install_patches(repository_ctx)
 
     _create_build_files(repository_ctx, "yarn_install", node, repository_ctx.attr.yarn_lock, repository_ctx.attr.generate_local_modules_build_files)
 

internal/npm_install/test/patches_npm_symlinked/BUILD.bazel

@@ -0,0 +1,9 @@
+load("//:index.bzl", "nodejs_test")
+
+nodejs_test(
+    name = "test",
+    data = [
+        "@internal_npm_install_test_patches_npm_symlinked//semver",
+    ],
+    entry_point = "test.js",
+)

internal/npm_install/test/patches_npm_symlinked/package.json

@@ -0,0 +1,5 @@
+{
+  "dependencies": {
+    "semver": "1.0.0"
+  }
+}

internal/npm_install/test/patches_npm_symlinked/semver+1.0.0.patch

@@ -0,0 +1,10 @@
+--- internal/npm_install/test/patches_npm_symlinked/node_modules/semver/semver.js
++++ internal/npm_install/test/patches_npm_symlinked/node_modules/semver/semver.js
+@@ -34,6 +34,7 @@ exports.valid = valid
+ exports.validPackage = validPackage
+ exports.validRange = validRange
+ exports.maxSatisfying = maxSatisfying
++exports.patched = true
+
+ function clean (ver) {
+   v = exports.parse(ver)

internal/npm_install/test/patches_npm_symlinked/test.js

@@ -0,0 +1,5 @@
+const semver = require('semver')
+if (!semver.patched) {
+  console.error('Expected semver to be patched');
+  process.exitCode = 1;
+}

internal/npm_install/test/patches_yarn_symlinked/BUILD.bazel

@@ -0,0 +1,9 @@
+load("//:index.bzl", "nodejs_test")
+
+nodejs_test(
+    name = "test",
+    data = [
+        "@internal_npm_install_test_patches_yarn_symlinked//semver",
+    ],
+    entry_point = "test.js",
+)

internal/npm_install/test/patches_yarn_symlinked/package.json

@@ -0,0 +1,5 @@
+{
+  "dependencies": {
+    "semver": "1.0.0"
+  }
+}

internal/npm_install/test/patches_yarn_symlinked/semver+1.0.0.patch

@@ -0,0 +1,10 @@
+--- internal/npm_install/test/patches_yarn_symlinked/node_modules/semver/semver.js
++++ internal/npm_install/test/patches_yarn_symlinked/node_modules/semver/semver.js
+@@ -34,6 +34,7 @@ exports.valid = valid
+ exports.validPackage = validPackage
+ exports.validRange = validRange
+ exports.maxSatisfying = maxSatisfying
++exports.patched = true
+
+ function clean (ver) {
+   v = exports.parse(ver)