Fix inconsistent trailing slash in remappings #49

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?

src/remappings.rs

Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>

src/remappings.rs

Co-authored-by: Blaze <blaze@dorime.org>

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 } 
        
               } 
        
           }

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

fix: do not blindly add trailing slash in remapping serialization

93c5f46

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 mattsse and klkvr April 2, 2024 15:20

mattsse reviewed Apr 10, 2024

View reviewed changes

BlazeWasHere added a commit to BlazeWasHere/foundry that referenced this pull request Apr 10, 2024

patch for foundry-rs/compilers#49

067ff3b

BlazeWasHere mentioned this pull request Apr 10, 2024

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

Open

BlazeWasHere reviewed Apr 10, 2024

View reviewed changes

src/remappings.rs Show resolved Hide resolved

fix: preserve trailing slash on Windows when converting slash

1b4306f

DaniPopes reviewed Apr 11, 2024

View reviewed changes

src/remappings.rs Outdated Show resolved Hide resolved

src/remappings.rs Outdated Show resolved Hide resolved

src/remappings.rs Outdated Show resolved Hide resolved

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

Troublor and others added 2 commits April 11, 2024 14:42

Apply suggestions from code review

feccc5b

Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>

chore: fix clippy errors

73c8ccd

BlazeWasHere reviewed Apr 13, 2024

View reviewed changes

src/remappings.rs Outdated Show resolved Hide resolved

Update remappings.rs

66d2677

Co-authored-by: Blaze <blaze@dorime.org>

DaniPopes approved these changes Apr 13, 2024

View reviewed changes

mattsse approved these changes Apr 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inconsistent trailing slash in remappings #49

Fix inconsistent trailing slash in remappings #49

Troublor commented Jan 21, 2024 •

edited

BlazeWasHere commented Mar 30, 2024

Troublor commented Mar 30, 2024

mattsse left a comment

BlazeWasHere commented Apr 10, 2024 •

edited

Troublor commented Apr 10, 2024

DaniPopes left a comment

mattsse left a comment

mattsse commented Apr 13, 2024

BlazeWasHere commented Apr 13, 2024 •

edited

Fix inconsistent trailing slash in remappings #49

Are you sure you want to change the base?

Fix inconsistent trailing slash in remappings #49

Conversation

Troublor commented Jan 21, 2024 • edited

Fix strategy

Rationale

BlazeWasHere commented Mar 30, 2024

Troublor commented Mar 30, 2024

mattsse left a comment

Choose a reason for hiding this comment

BlazeWasHere commented Apr 10, 2024 • edited

Troublor commented Apr 10, 2024

DaniPopes left a comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

mattsse commented Apr 13, 2024

BlazeWasHere commented Apr 13, 2024 • edited

Troublor commented Jan 21, 2024 •

edited

BlazeWasHere commented Apr 10, 2024 •

edited

BlazeWasHere commented Apr 13, 2024 •

edited