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

fix: only copy in-project symlinks #66785

Closed
wants to merge 1 commit into from
Closed

Conversation

jun-sheaf
Copy link

What?

Instead of copying symlinks in standalone builds, we resolve them to their realpath and copy the actual file.

Why?

By their nature, symlinks are not meant for standalone builds. If they resolve to a path somewhere else, that path needs to exist at runtime which may not be the case. E.g. we use Bazel where node_module dependencies are sandboxed and symlinked during build. This implies Next.js will copy over these symlinks built for the sandbox which will break once the sandbox is destroyed.

How?

Instead of using reallink, we use realpath.

@ijjk
Copy link
Member

ijjk commented Jun 12, 2024

Allow CI Workflow Run

  • approve CI run for commit: cc21d5e

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@jun-sheaf
Copy link
Author

@ijjk Gentle ping. Could you please take a look?

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Hi, this doesn't seem correct as it is valid to have a relative symlink like with pnpm's node_module's handling which we want to handle. Instead we should be only copying the file if it is outside of the detected project root.

If we aren't copying when we detect outside of the project root could we add a test case and ensure the above described behavior is occurring?

@jun-sheaf
Copy link
Author

If we aren't copying when we detect outside of the project root could we add a test case and ensure the above described behavior is occurring?

Could you point to where I should add the test?

this doesn't seem correct as it is valid to have a relative symlink like with pnpm's node_module's handling which we want to handle.

In a mono-repo, this will fail because pnpm will link outside the package root.

Copy link

vercel bot commented Jun 18, 2024

@jun-sheaf is attempting to deploy a commit to the Vercel Partnerships Team on Vercel.

A member of the Team first needs to authorize it.

@jun-sheaf jun-sheaf changed the title fix: use realpath instead of reallink fix: only copy in-project symlinks Jun 18, 2024
@jun-sheaf jun-sheaf force-pushed the patch-2 branch 3 times, most recently from 6dd5090 to ebea1a9 Compare June 19, 2024 00:16
@jun-sheaf jun-sheaf requested a review from ijjk June 19, 2024 00:16
@jun-sheaf
Copy link
Author

jun-sheaf commented Jun 20, 2024

It looks like there are bigger problems than just the symlinks when trying to build this through Bazel. More specifically, because the .pnpm folder is outside the project root, even if you copy the direct dependencies, the indirect dependencies won't exist. I don't expect next.js to reconstruct the folder so I'll close this as out of scope.

For Bazel users, the solution we used was to wrap the CLI and run pnpm install in the standalone directory after it's constructed.

@jun-sheaf jun-sheaf closed this Jun 20, 2024
@jun-sheaf jun-sheaf deleted the patch-2 branch June 20, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants