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

node_modules linker refactor #21

Closed
wants to merge 6 commits into from

Conversation

Silic0nS0ldier
Copy link
Contributor

The existing linker has the following issues;

  1. Not thread-safe
    Symlinks are created in shared spaces that point to action specific runfiles symlink forests and
    leading to race conditions (linkers for different executables writing conflicting symlinks). The
    latter results in actions running wihtout access to their node_modules inputs.
  2. Non-portable symlink paths
    Symlink targets are computed in a separate action which may not be using the same execution
    strategy or will simply never match if sandboxing is enabled, resulting in invalid symlinks.
  3. Leaking inputs
    The node_modules created closest to cwd points to the originally generated node_modules
    external repository. This contains all packages for the owning package.json/yarn.lock pair
    and in conjunction with managed_directories allows these packages to use files in the user
    workspace.

This PR refactors the linker so symlinks are written under [name].runfiles/ in a thread-safe manner.

Compared to the previous implementation;

  • node_modules symlinks are written inside the runfiles directory of the current executable.
  • Symlink destinations are rooted in the runfiles directory, preventing certain classes of input leaks via the managed_directories Bazel feature.

    Before:

    • /__cwd__/web/node_modules
      /__output_base__/external/web_node_modules/node_modules
      (with managed_directories, this is a symlink into the user workspace)
    • /__output_base__/execroot/com_canva_canva/bazel-out/_/bin/___/_.runfiles/com_canva_canva/web/node_modules
      /__output_base__/execroot/com_canva_canva/web/node_modules
    • /__cwd__/bazel-out/_/bin/web/node_modules
      /__output_base__/execroot/com_canva_canva/web/node_modules

    After

    • /__output_base__/execroot/com_canva_canva/bazel-out/_/bin/___/_.runfiles/com_canva_canva/web/node_modules
      /__output_base__/execroot/com_canva_canva/bazel-out/_/bin/___/_.runfiles/web_node_modules/node_modules
  • Elimination of cross-contamination issues due to writing symlinks in directories visible to different executables.
  • Sandboxing support (--worker_sandboxing and sandboxed spawn strategies).
  • A big step towards remote execution support.

@Silic0nS0ldier Silic0nS0ldier marked this pull request as ready for review November 16, 2023 01:03
Copy link
Member

@christianscott christianscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work

internal/linker/link_node_modules.ts Outdated Show resolved Hide resolved
internal/linker/link_node_modules.ts Outdated Show resolved Hide resolved
@Silic0nS0ldier
Copy link
Contributor Author

Remaining issue is due to flaws around how Rollup is used. The same issue that had to be solved internally when this change was first rolled out via a .patch file.

I'll need to rework existing Rollup usage to work with this.

@Silic0nS0ldier
Copy link
Contributor Author

More issues were discovered. e.g. TypeScript rules (ts_project and the tsc generated rule) are unable to resolve node_modules, so types from packages like @types/node cannot be found.

Given sufficient time these issues can be overcome, however the goal here was to simplify patch management. To that end, repo source will be replaced with v3.8.0 release artefact contents.

Closing PR.

@Silic0nS0ldier Silic0nS0ldier deleted the jordan-mele_nm-linker_take-2 branch December 6, 2023 23:35
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