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

feat: allow single file remappings #6706

Closed
2 tasks done
arthurneuron opened this issue Jan 4, 2024 · 9 comments · Fixed by #9604
Closed
2 tasks done

feat: allow single file remappings #6706

arthurneuron opened this issue Jan 4, 2024 · 9 comments · Fixed by #9604
Labels
A-dependencies Area: dependencies C-forge Command: forge T-feature Type: feature

Comments

@arthurneuron
Copy link

arthurneuron commented Jan 4, 2024

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 (9e3ab9b 2024-01-04T00:18:01.892563000Z)

What command(s) is the bug in?

forge test

Operating System

macOS (Apple Silicon)

Describe the bug

  • Correct path: lib/openzeppelin-contracts/contracts/token/ERC721/extensions/IERC721Metadata.sol
  • Required Import: @openzeppelin/contracts/token/ERC721/IERC721Metadata.sol
  • This is not possible, since this exception is only for one file: @openzeppelin/contracts/token/ERC721/=lib/openzeppelin-contracts/contracts/token/ERC721/extensions/

remappings.txt

  • @openzeppelin/contracts/token/ERC721/IERC721Metadata.sol=lib/openzeppelin-contracts/contracts/token/ERC721/extensions/IERC721Metadata.sol

Error:

  • failed to resolve file: "/Users/foundry/Developer/foundry/packages/contracts/lib/openzeppelin-contracts/contracts/token/ERC721/extensions/IERC721Metadata.sol/": Not a directory (os error 20); check configured remappings.

remappings.txt

  • @openzeppelin/contracts/token/ERC721/=lib/openzeppelin-contracts/contracts/token/ERC721/extensions/IERC721Metadata.sol

Error:

  • failed to resolve file: "/Users/foundry/Developer/foundry/packages/contracts/lib/openzeppelin-contracts/contracts/token/ERC721/extensions/IERC721Metadata.sol/IERC721Metadata.sol": Not a directory (os error 20); check configured remappings.

remappings.txt

  • @openzeppelin/contracts/token/ERC721/IERC721Metadata.sol=lib/openzeppelin-contracts/contracts/token/ERC721/extensions/

Error:

  • failed to resolve file: "/Users/foundry/Developer/foundry/packages/contracts/lib/openzeppelin-contracts/contracts/token/ERC721/extensions/": Is a directory (os error 21); check configured remappings.
@arthurneuron arthurneuron added the T-bug Type: bug label Jan 4, 2024
@gakonst gakonst added this to Foundry Jan 4, 2024
@github-project-automation github-project-automation bot moved this to Todo in Foundry Jan 4, 2024
@mattsse
Copy link
Member

mattsse commented Jan 5, 2024

could you please provide a repro for this?

@mattsse
Copy link
Member

mattsse commented Jan 5, 2024

@tash-2s I wonder if this is related to the recent path changes?

@tash-2s
Copy link

tash-2s commented Jan 5, 2024

I'll look into this.

@arthurneuron Did the same setup and command work in previous versions of forge? (Is this an issue that occurred after updating forge?) It would be really helpful if you could provide a detailed reproduction of the error.

@tash-2s
Copy link

tash-2s commented Jan 5, 2024

I believe this is the reproduction of the issue:

$ mkdir issues6706 && cd issues6706
$ mkdir src my-lib

$ cat << EOF > src/A.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.23;
import "@my-lib/B.sol";
contract A is B {}
EOF

$ cat << EOF > my-lib/B.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.23;
contract B {}
EOF

$ cat << EOF > remappings.txt
@my-lib/B.sol=my-lib/B.sol
EOF

$ forge build
[⠊] Compiling...
Error: 
failed to resolve file: "/Users/t/bench/issues6706/my-lib/B.sol/": Not a directory (os error 20); check configured remappings.
    --> "/Users/t/bench/issues6706/src/A.sol"
        "@my-lib/B.sol"

This change makes it work:

diff --git a/remappings.txt b/remappings.txt
index 134e3a5..ceb4244 100644
--- a/remappings.txt
+++ b/remappings.txt
@@ -1 +1 @@
-@my-lib/B.sol=my-lib/B.sol
+@my-lib/=my-lib/

This issue is not related to the recent changes (foundry-rs/compilers#35, foundry-rs/compilers#36). I can confirm the same behavior occurs with an older version of forge. (foundryup --commit 67ab8704476d55e47545cf6217e236553c427a80)

The current implementation of remapping resolution does not support this case. See:
https://github.com/foundry-rs/compilers/blob/1cde7fe2559815f40255f7338c7998df4f55ca7d/src/config.rs#L344-L345

To simulate that code:

fn main() {
    use std::path::Path;

    let p1 = Path::new("@my-lib/A.sol").strip_prefix(Path::new("@my-lib/A.sol"));
    let p2 = Path::new("A.sol").join(Path::new(""));

    println!("{:?}, {:?}", p1, p2); // => Ok(""), "A.sol/"
}

Copy link
Collaborator

onbjerg commented Jan 8, 2024

This is correct - you cannot remap individual files, only directories. Not sure if we should count this as a bug or as a feature request, in any case, it might make sense to support this

@drinkius
Copy link

I've faced similar issue with dependency that relies on IERC721Metadata.sol. Definitely need some way to account for possible path changes

@heeween
Copy link

heeween commented Jul 1, 2024

The reason is that from 3.4.2 openzepplin has change the path of ERC72Metadata.sol.
so i solve it by install a low version of openzepplin.
forge install github.com/OpenZeppelin/[email protected] --no-commit.

@zerosnacks
Copy link
Member

zerosnacks commented Jul 9, 2024

The reason is that from 3.4.2 openzepplin has change the path of ERC72Metadata.sol.

so i solve it by install a low version of openzepplin.

forge install github.com/OpenZeppelin/[email protected] --no-commit.

Not directly related but it is not recommended to install older versions of OpenZeppelin as they may have (known) bugs and vulnerabilities.

@zerosnacks zerosnacks added T-feature Type: feature and removed T-bug Type: bug labels Jul 9, 2024
@zerosnacks zerosnacks changed the title Remappings does not allow use for 1 file feat: allow single file remappings Jul 9, 2024
@zerosnacks zerosnacks added C-forge Command: forge A-dependencies Area: dependencies labels Jul 9, 2024
@zerosnacks
Copy link
Member

zerosnacks commented Jul 9, 2024

This is correct - you cannot remap individual files, only directories. Not sure if we should count this as a bug or as a feature request, in any case, it might make sense to support this

Updated the ticket to reflect that this is a feature request, not a bug

Related: #7527

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: dependencies C-forge Command: forge T-feature Type: feature
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

8 participants