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

forge struggles to handle the way pnpm links node_modules in pnpm-workspaces #2714

Closed
2 tasks done
JasoonS opened this issue Aug 11, 2022 · 8 comments
Closed
2 tasks done
Assignees
Labels
C-forge Command: forge Cmd-forge-pm Command: forge install/update/remove T-bug Type: bug

Comments

@JasoonS
Copy link

JasoonS commented Aug 11, 2022

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (6cd6618 2022-08-11T00:03:54.64793456Z)

What command(s) is the bug in?

forge build

Operating System

Linux

Describe the bug

This is very likely a duplicate of the original issue here: #1801 - creating a new issue since that issue seems to have been confused with other issues, so maybe a clean conversation is useful.

This issue is only pressent when using pnpm; yarn and npm works fine.

The reproduction of the issue is here: https://github.com/JasoonS/forge-node_module-pnpm-issue-repro

exact steps to run in terminal:

git clone git@github.com:JasoonS/forge-node_module-pnpm-issue-repro.git
cd forge-node_module-pnpm-issue-repro/contracts
pnpm i
forge build # this fails
rm -rf node_modules
yarn
rm -rf out cache & forge cache clean
forge build # this succeeds! Not an issue with yarn

I think this is happening because pnpm is placing the package outside of the contracts folder (in the root of the repo) and sym linking it.

Thank you in advance for taking a look at this issue 🙏

@JasoonS JasoonS added the T-bug Type: bug label Aug 11, 2022
@gakonst gakonst added this to Foundry Aug 11, 2022
@gakonst gakonst moved this to Todo in Foundry Aug 11, 2022
@fubhy
Copy link
Contributor

fubhy commented Aug 11, 2022

I don't think it should be foundry's responsibility to understand all the ways in which the different package managers store and manage their assets. In the case of pnpm for instance you can resolve that by configuring your pnpm workspace with an .npmrc that instructs pnpm to hoist the packages in question.

Simply create a .npmrc file with this content (or whatever fits your requirements):

public-hoist-pattern[]=@openzeppelin/contracts
public-hoist-pattern[]=@uniswap/v3-core
public-hoist-pattern[]=@uniswap/v3-periphery
...

(After that you need to run pnpm install again of course ;-)

@JasoonS
Copy link
Author

JasoonS commented Aug 11, 2022

Thanks for the reply @fubhy - preventing pnpm from hoisting to the package root wasn't working for some reason, I didn't manage to solve that (also tried hoist=false).

In the end I just made the contracts sub-folder its own pnpm workspace by adding a pnpm-workspace.yaml file into its folder.

Not the ideal solution, but it works!

@onbjerg onbjerg added C-forge Command: forge Cmd-forge-pm Command: forge install/update/remove labels Aug 11, 2022
@onbjerg onbjerg self-assigned this Aug 11, 2022
@mattsse mattsse assigned mattsse and unassigned onbjerg Aug 11, 2022
@fubhy
Copy link
Contributor

fubhy commented Aug 11, 2022

@JasoonS I see. I looked at your reproduction repo now, missed that before, sorry. I was just assuming that you are refering to the pnpm store behavior and how it resolves packages via symlinks from node_modules/.pnpm in the workspace root. This you can solve with public-hoist-pattern which causes the packages to be hoisted to node_modules/@openzeppelin/contracts/... etc. instead of node_modules/.pnpm/@openzeppelin+contracts@{version}/...

It doesn't solve the problem you are facing though. To get around it, I'd suggest to put to foundry.toml in the root and configure it accordingly. I've opened a PR against your repro repo. I personally prefer to have to foundry.toml in the root so that you can conveniently use forge from there too just like you are used to with pnpm run, pnpm turbo run etc. but I can see how that might be personal preference.

Maybe someone else has a better idea for how this might work. There are some monorepos out there using foundry already e.g. https://github.com/nomad-xyz/monorepo (yarn, granted, but still). That one also uses packages to split up the contracts for instance.

@JasoonS
Copy link
Author

JasoonS commented Aug 11, 2022

That makes sense, thanks for the help. I'll experiment around with different configurations. Main thing is that it is all working ✌️

@onbjerg
Copy link
Member

onbjerg commented Aug 12, 2022

I think people just do libs = ['../../../node_modules'] and so on from their subfolders's foundry.toml to add the node_modules in the root of the project?

@sambacha
Copy link
Contributor

sambacha commented Aug 17, 2022

https://github.com/sambacha/forge-scope if you can try running the inportly command to generate the scoped remappings it should work after you have installed your node dependencies

ie:

root_dir :- $ npx importly < package.json > forge-importmap.json

Then transfer the results to remapping.txt or your toml file

this will work if you define a resolutions field in your package manifest only

@mattsse
Copy link
Member

mattsse commented Aug 18, 2022

this is a symlink issue,
I don't understand yet how pnpm manages and links different node_modules folders but it looks like solc is able to resolve them:

error[6275]: ParserError: Source "node_modules/@openzeppelin/contracts/utils/cryptography/ECDSA.sol" not found: File outside of allowed directories. ....:

so what you need to do is to add:

allow_paths = ["../node_modules"]

in you foundry.toml

@mattsse
Copy link
Member

mattsse commented Aug 18, 2022

@onbjerg added a troubleshooting section for pnpm/symlinks:
foundry-rs/book#540

I'm hesitant to do any more symlink work in ethers-solc, since most of them can be fixed by solc + allow_paths, I vote to close this one as won't fix

@onbjerg onbjerg closed this as not planned Won't fix, can't repro, duplicate, stale Aug 19, 2022
Repository owner moved this from Todo to Done in Foundry Aug 19, 2022
kevinhalliday added a commit to omni-network/omni that referenced this issue Jan 5, 2024
Switch from yarn to pnpm for node package management.

#### Motivation

[pnpm](https://pnpm.io/) is becoming industry standard for managing node
packages. It's lightweight, and removes a lot of bloat from node_modules
- using symlinks and ensuring package versions are only installed to
disk once. Installations are much quicker. And it has better support for
monorepos (pnpm workspaces) should we decide to manage multiple pnpm
projects in our monorepo (as [optimism
does](https://github.com/ethereum-optimism/optimism/blob/develop/pnpm-workspace.yaml),
for example)

I've avoided using it in the past alongside foundry because foundry has
allegedly had [issues with handling pnpm
symlinks](foundry-rs/foundry#2714), but I've
tried it in our playground repo and imports work well, for both building
& IDE ls integrations.

task: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge Cmd-forge-pm Command: forge install/update/remove T-bug Type: bug
Projects
Archived in project
Development

No branches or pull requests

5 participants