ds-test not found when installing forge-std as an npm package.

[maurelian]

The package.json file added in #176 (despite coming from our team), doesn't seem to work.

You can see that this PR, which updates the commit from one prior to adding it (f18682b) to the 1.0.0 commit (17656a2), breaks the build.
The error is due to being unable to find the ds-test dependency, which doesn't seem to be included with the current package.

A package.json was also recently added to ds-test, so it could be added as a dependency to the forge-std package, however I'm not sure how to handle the different remappings that would be required if ds-test is installed to node_modules rather than as a submodule.

[mds1]

Hmm, interesting. So once change is that ds-test is now imported using relative paths, which I'd think should help any remappings issues when installed in node_modules As for ds-test not being included, I'm far from an npm/yarn expert but perhaps there is someway to specify to include submodules during the install, analogous to git clone --recurse-submodules? How did ds-test get installed before? Is ds-test present and just not found, or not at all installed?

[maurelian]

How did ds-test get installed before? Is ds-test present and just not found, or not at all installed?

It's also (and was previously) installed using yarn as well (see both here).

After some debugging, I'm able to fix this by manually editing the import paths in Test.sol and StdAssertions.sol like so:

- import "../lib/ds-test/src/test.sol";
+ import "ds-test/test.sol";

However without changing those, I'm unable to make it work by modifying the remappings in the config file. It seems as though forge is ignoring the remapping, and just looking at the location relative to the source files. Do remapping not work on relative paths?

[mds1]

cc @mattsse for more path questions 😅 perhaps our rationale in #198 and #200 for using relative paths for ds-test was wrong?

[maurelian]

I looked in to using a postinstall script

https://github.com/maurelian/forge-std/blob/6bb1e59a05a8170cd9c80f302f65dc8228224b6e/package.json?rgh-link-date=2022-11-03T23%3A40%3A52Z#L17

But that is apparently very buggy in yarn.

[mattsse]

I think what's happening here

https://github.com/ethereum-optimism/optimism/blob/1d7c53e444e29aee8db6018c7ee32539d4524041/packages/contracts-bedrock/package.json#L68-L70

is that both are installed at the same level, right?

but forge-std expects ./forge-std/lib/ds-test but ds-test will be installed under ./ds-test.

so relative paths into submodules are an issue.

I consider using this via yarn a valid use case, perhaps we should revert the relative import @mds1 ?
I think having issues with duplicate ds-test versions is less likely, and I would consider this a code smell.

[mds1]

@mattsse I agree with your assessment, opened #210 to resolve. @maurelian can you test out installing forge-std at that commit?

[maurelian]

Yep, that commit (5bafa16) works thanks!

Should we use that commit, or the current tip of master (on which CI seems to be failing)?

[mattsse]

is unrelated, afaik.

forge-std is back on remappings. so I think we can close this?

[PaulRBerg]

So, is it an acknowledged limitation of Forge Std that it cannot be installed as an npm package?

I've installed Forge Std and DSTest like this:

"dependencies": {
  "ds-test": "github:dapphub/ds-test",
  "forge-std": "github:foundry-rs/forge-std"
},

But couldn't manage to use Forge Std:

https://app.warp.dev/block/ngdqKqj4NZXwAPd0oZ20dj