Fix inconsistent trailing slash in remappings by Troublor · Pull Request #49 · foundry-rs/compilers

Troublor · 2024-01-21T05:08:02Z

Fix #47. One corresponding test is added.

Fix strategy

Remove the code that blindly add trailing / when serializing Remapping and RelativeRemapping.
Preserve the trailing / in the strip_prefix method of Remapping.

Rationale

There are roughly two sources where Remapping comes from:

Generated in find_many function.
Loaded from configuration like foundry.toml.

When generating Remapping in find_many function, the algorithm makes sure that the name and path of every remapping are valid folders in the file system. Then, trailing /s are added to both name and path in the impl From<RelativeRemapping> for Remapping implementation.
https://github.com/Troublor/compilers/blob/93c5f46a7dd0c0d387dd94c8ea756812e2907d92/src/remappings.rs#L360-L365

When loading Remapping from user-provided configurations, the name and path of remappings should be set as it is, and do not add any trailing /s.

Either way, the field name and path of Remapping contains intended values and should not be changed in the serialization (the implementation for Display trait).

PS: About coding style, I tried to use TempProject to write the test case but it seems the project.compile() method does not respect the remapping I give it via project.with_settings(settings_with_remapping). The remapping just does not take effect in the compilation. Not sure whether it's I don't know how to use it or TempProject has some bugs.

BlazeWasHere · 2024-03-30T23:31:39Z

gm @Troublor are you still working on this?

Troublor · 2024-03-30T23:35:27Z

@BlazeWasHere Yeah you remind me. I will finish it this weekend.

mattsse

sorry forgot about this,

could you please open a foundry pr that patches the foundry-compilers crate to this branch to check if this causes any issues

BlazeWasHere · 2024-04-10T11:37:17Z

ah im not approved for github workflow actions, but locally it doesnt seem to build not sure its because of my system (new pc -- have never used rust on this before) or rust crate incompatibility

edit: it seems like this branch is just missing latest commits from main

Troublor · 2024-04-10T18:00:25Z

The test cases of foundry-rs/compilers (in the CI) seem to indicate there are some problems with Windows. I don't have a Windows machine. Could someone offer some help to look into the issues?

DaniPopes

lgtm, pending @mattsse

mattsse

lgtm,

really sorry about the delay

mattsse · 2024-04-13T05:03:22Z

hmm this results in a bunch of failing tests

foundry-rs/foundry#7623

can't proceed with this until sorted out

BlazeWasHere · 2024-04-13T08:53:52Z

hmm this results in a bunch of failing tests

foundry-rs/foundry#7623

can't proceed with this until sorted out

i havent updated my fork yet, can you retrigger ci again now

edit:

https://github.com/foundry-rs/foundry/actions/runs/8672405018/job/23783250651?pr=7623#step:12:316
windows (the bane of oss dev) has missing trailing /

gnu/linux ditto. 😢
https://github.com/foundry-rs/foundry/actions/runs/8672405018/job/23783250255?pr=7623#step:12:153

https://github.com/foundry-rs/foundry/blob/89f0fb923773cf0f8f966290e579bae92f505077/crates/forge/tests/cli/config.rs#L233
seems to fail because of

compilers/src/remappings.rs

Lines 364 to 376 in 66d2677

    
           impl From<RelativeRemapping> for Remapping { 
        
               fn from(r: RelativeRemapping) -> Self { 
        
                   let RelativeRemapping { context, mut name, path } = r; 
        
                   let mut path = format!("{}", path.relative().display()); 
        
                   if !path.ends_with('/') { 
        
                       path.push('/'); 
        
                   } 
        
                   if !name.ends_with('/') { 
        
                       name.push('/'); 
        
                   } 
        
                   Remapping { context, name, path } 
        
               } 
        
           }

klkvr · 2024-06-30T04:45:42Z

hey @Troublor do you have plans for finishing this?

considering that CI fails I am thinking that perhaps we should take a different approach? currently in remappings.txt of a foundry project you can specify remappings in any format: any of project=src, project=src/, project/=src, etc would get updated to project/=src/ because it's the way conversion RelativeRemapping -> Remapping works:

compilers/crates/artifacts/solc/src/remappings.rs

Lines 360 to 365 in a21275d

    
           if !path.ends_with('/') { 
        
               path.push('/'); 
        
           } 
        
           if !name.ends_with('/') { 
        
               name.push('/'); 
        
           }

however, when working with i.e. etherscan projects or any sources of remappings requiring deserialization directly into Remapping we don't go through that conversion and validation and name and path are kept as-is.

Thus, I think we can either just update Display impl for Remapping to also push / to name if it's not present (currently we are only pushing it to path):

compilers/crates/artifacts/solc/src/remappings.rs

Lines 159 to 161 in a21275d

    
           if !s.ends_with('/') { 
        
               s.push('/'); 
        
           }

Also, we can consider adding this additional logic to serde::Serialize impl which should also work

Troublor · 2024-07-05T05:06:21Z

@klkvr Thanks. I restructured the patch. Please approve the CI tests.

Troublor · 2024-07-05T18:42:19Z

@klkvr I fixed some regression test oracles. Please trigger CI again. Thx.

Troublor · 2024-07-05T18:55:04Z

@klkvr There seems to be some race: the last commit (just some refactoring) is not included in the CI. But the previous CI passes, so everything should look good now. Thanks for your efforts.

klkvr · 2024-07-05T21:17:46Z

@Troublor mind updating patch in foundry-rs/foundry#7623? (or creating a new PR if you don't have access to that branch)

Troublor · 2024-07-06T01:39:50Z

@klkvr It seems @BlazeWasHere has already updated that PR.

Troublor · 2024-07-06T04:11:11Z

@klkvr I created another PR to test on foundry: foundry-rs/foundry#8377

Foundry would need a little patch as well for one integration test: can_extract_config_values: The expected value of remapping name should have a trailing slash.

Other than this, all tests pass and everything looks good.

klkvr · 2024-07-06T15:12:39Z

@Troublor nice! does this solve the initial issue you had in #47 with compiling contract from Etherscan? If so, I think this is good to merge

ZhangZhuoSJTU · 2024-07-06T18:13:08Z

@Troublor nice! does this solve the initial issue you had in #47 with compiling contract from Etherscan? If so, I think this is good to merge

I think the original issue is solved after this patch.

Troublor · 2024-07-06T19:00:15Z

@klkvr Yes, problem solved. Thx.

Troublor requested review from DaniPopes and Evalir as code owners January 21, 2024 05:08

Troublor marked this pull request as draft January 21, 2024 05:08

Troublor force-pushed the issue47 branch from d57d997 to 93c5f46 Compare March 31, 2024 02:25

Troublor marked this pull request as ready for review March 31, 2024 02:37

DaniPopes requested review from klkvr and mattsse April 2, 2024 15:20

mattsse reviewed Apr 10, 2024

View reviewed changes

BlazeWasHere mentioned this pull request Apr 10, 2024

[do not merge] patch new foundry-compilers foundry-rs/foundry#7623

Closed

BlazeWasHere reviewed Apr 10, 2024

View reviewed changes

Comment thread src/remappings.rs Outdated

DaniPopes reviewed Apr 11, 2024

View reviewed changes

Comment thread src/remappings.rs Outdated

Comment thread src/remappings.rs Outdated

Comment thread src/remappings.rs Outdated

DaniPopes requested a review from mattsse April 11, 2024 18:38

BlazeWasHere reviewed Apr 13, 2024

View reviewed changes

Comment thread src/remappings.rs Outdated

DaniPopes approved these changes Apr 13, 2024

View reviewed changes

mattsse approved these changes Apr 13, 2024

View reviewed changes

Troublor force-pushed the issue47 branch from 66d2677 to baa5018 Compare July 5, 2024 05:05

add test and fix

d53022b

Troublor force-pushed the issue47 branch from baa5018 to d53022b Compare July 5, 2024 05:11

Troublor added 2 commits July 5, 2024 14:41

fix clippy issue

c2e3cd5

fix regression test oracles

c924196

format

29bde06

remove unnecessary prints

c233542

Troublor mentioned this pull request Jul 6, 2024

[do not merge] test the patch in foundry-compilers https://github.com/foundry-rs/compilers/pull/49 foundry-rs/foundry#8377

Closed

DaniPopes approved these changes Jul 6, 2024

View reviewed changes

DaniPopes merged commit 56911f2 into foundry-rs:main Jul 6, 2024

BlazeWasHere mentioned this pull request Jul 12, 2024

implicit trailing / in remappings causes file remappings to not resolve foundry-rs/foundry#7527

Closed

2 tasks done

Conversation

Troublor commented Jan 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix strategy

Rationale

Uh oh!

BlazeWasHere commented Mar 30, 2024

Uh oh!

Troublor commented Mar 30, 2024

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

BlazeWasHere commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Troublor commented Apr 10, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaniPopes left a comment

Choose a reason for hiding this comment

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

mattsse commented Apr 13, 2024

Uh oh!

BlazeWasHere commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klkvr commented Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Troublor commented Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Troublor commented Jul 5, 2024

Uh oh!

Troublor commented Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klkvr commented Jul 5, 2024

Uh oh!

Troublor commented Jul 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Troublor commented Jul 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klkvr commented Jul 6, 2024

Uh oh!

ZhangZhuoSJTU commented Jul 6, 2024

Uh oh!

Troublor commented Jul 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Troublor commented Jan 21, 2024 •

edited

Loading

BlazeWasHere commented Apr 10, 2024 •

edited

Loading

BlazeWasHere commented Apr 13, 2024 •

edited

Loading

klkvr commented Jun 30, 2024 •

edited

Loading

Troublor commented Jul 5, 2024 •

edited

Loading

Troublor commented Jul 5, 2024 •

edited

Loading

Troublor commented Jul 6, 2024 •

edited

Loading

Troublor commented Jul 6, 2024 •

edited

Loading