Fix inconsistent trailing slash in remappings

[Troublor]

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]

gm @Troublor are you still working on this?

[Troublor]

@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]

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]

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]

hmm this results in a bunch of failing tests

foundry-rs/foundry#7623

can't proceed with this until sorted out

[BlazeWasHere]

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]

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]

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

[Troublor]

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

[Troublor]

@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]

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

[Troublor]

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

[Troublor]

@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]

@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]

@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]

@klkvr Yes, problem solved. Thx.