Stripping base path from CLI paths by cameel · Pull Request #11545 · argotorg/solidity

cameel · 2021-06-16T20:48:19Z

Fixes #4702.
Implements the solc part of #11408. solc-js changes will be submitted in that repo.
~~Depends on #11544.~~ Merged.

This still work in progress. I'd say It's about 90% done. There's not much implementation left (it's very small overall) but some corner cases are not yet handled or not handled properly:

Paths with a different case on case-insensitive filesystems.
Paths starting with // (UNC paths).
Paths using .. to go beyond filesystem root (boost's normalization does not squash /../ into /).
Paths with symlinks.
Removing root name so that paths always start with / (rather than drive letter on Windows) when they're on the same partition as CWD.
Different behavior on some older Boost versions.

libsolidity/interface/FileReader.cpp

cameel · 2021-06-21T10:55:15Z

Here's a short update on the current status on this PR:

I dealt with most of the corner cases listed in PR description. The last remaining ones are:
- Symlinks. On macOS in CI the path to the temporary directory contains a symlink (/var/folders/ -> /private/var/folders/) and Boost sometimes resolves the symlink when the path is relative and starts with ../. I suspect it might be a bug in its boost version but I need to investigate. This will be tedious because I can reproduce it only in CI.
- The root path is sometimes C:/ on Windows where it should be normalized to /. This might be a bug on my part.
Base path docs need to be updated.
I resolved all the CI failures in the refactor PRs it depends on (CommandLineParser #11518, Local output streams in CommandLineParser #11520, Common input file processing for CLI and Standard JSON #11544). But as requested by @chriseth I'll put this one directly on develop anyway - in a stripped-down version (without CLI tests) - once I get tests to pass.

cameel · 2021-07-05T20:17:12Z

This PR is now working correctly on all platforms. It still depends on the CLI refactor so I have created an equivalent PR without the CLI tests that does not: #11617. These tests turned out to be crucial as I suspected though. There were tons of weird cases and the FileReader unit tests did not catch many of them (see #11617 (comment)).

aarlt · 2021-08-19T13:11:23Z

libsolidity/interface/FileReader.cpp


+void FileReader::setBasePath(boost::filesystem::path const& _path)
+{
+	m_basePath = (_path.empty() ? "" : normalizeCLIPathForVFS(_path));


What about setting normalizeCLIPathForVFS(".") already here, if _path is empty? With this m_basePath would always contain a valid path.

The problem is that "" is a special case that is not equivalent to any specific path.

For example if I use the working dir as you're suggesting, it will break absolute paths - import "/x/y.sol" will try to load x.y.sol from work dir rather than the absolute /x/y.sol.

Using / would keep absolute paths working but it would relative paths - import "x/y.sol" would try to load /x.y.sol.

So these two values are out and I can't think of any other path that would keep --base-path="" working as it does now.

cameel · 2021-08-20T18:06:31Z

While working on #11409 I found a small bug introduced in this PR: standard input was not being handled correctly in combination with base path. <stdin> was converted to an absolute path as if it were a file name. I added a fixup that solves this and contains a test for this case.

I have also squashed the older fixups.

leonardoalt · 2021-08-24T08:01:07Z

solc/CommandLineInterface.cpp

@@ -451,7 +451,7 @@ bool CommandLineInterface::readInputFiles()
 			m_standardJsonInput = readUntilEnd(m_sin);


Not related but this line seems to have its indentation off. Could you fix it since you're touching the file already?

Sure, but the indent on this line and others around it look fine to me (it's all tabs). Is this the right line?

Hm it looked weird on GitHub, not sure then.

Yeah, github renders tabs in a weird way - as if they were shifted left relative to the beginning of the line. Really annoying.

leonardoalt · 2021-08-24T08:01:41Z

solc/CommandLineInterface.cpp

 		}
 		else
-			m_fileReader.setSource(g_stdinFileName, readUntilEnd(m_sin));
+			m_fileReader.setStdin(readUntilEnd(m_sin));


Is this the only place we read from stdin?

Yeah. This and the one 3 lines up. It used to be done separately for Standard JSON but I unified that in #11544 so that it's now done here for all input modes.

leonardoalt · 2021-08-24T08:02:34Z

libsolidity/interface/FileReader.cpp

+
+void FileReader::setStdin(SourceCode _source)
+{
+	m_sourceCodes["<stdin>"] = std::move(_source);


Where do we use m_sourceCodes["<stdin>"]? I'm wondering if the key "<stdin>" can get misused/misplaced somewhere

It's passed along with all the other input files to the compiler stack. You can technically import it just like a normal file with import "<stdin>" because that's its source unit name. This is a pre-existing behavior documented in https://docs.soliditylang.org/en/latest/path-resolution.html#index-3.

It's possible to have a file that gets the same source unit name:

echo 'contract A {}' > '<stdin>' echo 'contract B {}' | solc '<stdin>' - --base-path .

this line would then overwrite the content of that file in FileReader and replace it with the content of standard input. Not great but I think that <stdin> as a file name is obscure enough that it does not really matter all that much.

In any case this is how the compiler already worked before and my PR does not change that. I only added a dedicated function for adding content for this source unit name because setSource() now performs normalization and we obviously do not want that here.

Ok sounds good.

test/solc/CommandLineInterface.cpp

test/solc/Common.cpp

cameel · 2021-08-26T18:28:39Z

I just pushed an updated version. It removes defaultCommandLineOptions() and has slightly reworded documentation (backported here from my new PR for --include-path).

libsolidity/interface/FileReader.h

aarlt · 2021-08-26T21:50:55Z

test/CMakeLists.txt

    libsolidity/SyntaxTest.h
    libsolidity/ViewPureChecker.cpp
    libsolidity/analysis/FunctionCallGraph.cpp
+    libsolidity/interface/FileReader.cpp


What about libsolidity/interface/FileReader.h?

Boost tests typically do not have header files. Boost can find and run them without it.

The ones that do have headers here are actually a part of soltest/isoltest implementation and not test cases.

test/FilesystemUtils.cpp

test/FilesystemUtils.h

cameel · 2021-08-27T12:46:06Z

@aarlt Fixes applied. Ready for another round.

…links used on Windows

…aths

cameel · 2021-08-27T13:12:12Z

Thanks @leonardoalt! Could you reapprove? I just squashed the fixups.

cameel self-assigned this Jun 16, 2021

chriseth reviewed Jun 17, 2021

View reviewed changes

libsolidity/interface/FileReader.cpp Outdated Show resolved Hide resolved

chriseth reviewed Jun 17, 2021

View reviewed changes

libsolidity/interface/FileReader.cpp Outdated Show resolved Hide resolved

cameel force-pushed the standard-json-cli-common-input-file-processing branch 4 times, most recently from 58efdde to cc6e5fc Compare June 17, 2021 17:11

cameel force-pushed the stripping-base-path-from-cli branch 5 times, most recently from 362d656 to 654dc3f Compare June 18, 2021 23:55

cameel force-pushed the stripping-base-path-from-cli branch from 2a89fdc to 52c5c9e Compare June 29, 2021 20:29

cameel force-pushed the standard-json-cli-common-input-file-processing branch 3 times, most recently from c2e67e1 to c1e4cb8 Compare July 1, 2021 18:23

cameel force-pushed the stripping-base-path-from-cli branch 5 times, most recently from b5820f0 to db0029b Compare July 2, 2021 18:35

cameel force-pushed the standard-json-cli-common-input-file-processing branch from c1e4cb8 to d85b750 Compare July 2, 2021 21:02

cameel force-pushed the stripping-base-path-from-cli branch 2 times, most recently from 7e9230d to 350a950 Compare July 5, 2021 19:02

cameel force-pushed the standard-json-cli-common-input-file-processing branch from d85b750 to 992c868 Compare July 5, 2021 19:06

cameel force-pushed the stripping-base-path-from-cli branch 2 times, most recently from 0763219 to 9cd715b Compare July 5, 2021 19:14

cameel mentioned this pull request Jul 5, 2021

Stripping base path from CLI paths (without CLI refactor) #11617

Closed

cameel force-pushed the stripping-base-path-from-cli branch from b1ba2bd to 444d863 Compare August 17, 2021 10:42

aarlt reviewed Aug 19, 2021

View reviewed changes

cameel force-pushed the stripping-base-path-from-cli branch from 444d863 to fa9576f Compare August 20, 2021 18:04

leonardoalt reviewed Aug 24, 2021

View reviewed changes

test/solc/CommandLineInterface.cpp Show resolved Hide resolved

leonardoalt reviewed Aug 24, 2021

View reviewed changes

test/solc/Common.cpp Outdated Show resolved Hide resolved

cameel force-pushed the stripping-base-path-from-cli branch from fa9576f to 37fed1e Compare August 26, 2021 18:24

cameel requested review from aarlt and leonardoalt August 26, 2021 18:28

aarlt reviewed Aug 26, 2021

View reviewed changes

libsolidity/interface/FileReader.h Outdated Show resolved Hide resolved

aarlt reviewed Aug 26, 2021

View reviewed changes

test/FilesystemUtils.cpp Outdated Show resolved Hide resolved

aarlt reviewed Aug 26, 2021

View reviewed changes

test/FilesystemUtils.h Outdated Show resolved Hide resolved

cameel force-pushed the stripping-base-path-from-cli branch from 37fed1e to 83a9983 Compare August 27, 2021 12:44

cameel requested a review from aarlt August 27, 2021 12:45

leonardoalt previously approved these changes Aug 27, 2021

View reviewed changes

cameel added 3 commits August 27, 2021 15:10

createSymlinkIfSupportedByFilesystem(): Add support for directory sym…

618ba2f

…links used on Windows

CLI tests for FileReader

2d3ec69

FileReader: Normalize base path and strip it from normalized source p…

13f46eb

…aths

cameel dismissed leonardoalt’s stale review via 13f46eb August 27, 2021 13:11

cameel force-pushed the stripping-base-path-from-cli branch from 83a9983 to 13f46eb Compare August 27, 2021 13:11

leonardoalt approved these changes Aug 27, 2021

View reviewed changes

cameel merged commit 5229ace into develop Aug 27, 2021

0xalpharush mentioned this pull request Mar 22, 2024

fix: normalize windows path to handle solc >=0.8.8 filename lookup crytic/crytic-compile#554

Merged

		@@ -451,7 +451,7 @@ bool CommandLineInterface::readInputFiles()
		m_standardJsonInput = readUntilEnd(m_sin);

Conversation

cameel commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cameel commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

aarlt Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cameel commented Aug 26, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cameel commented Aug 27, 2021

Uh oh!

cameel commented Aug 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cameel commented Jun 16, 2021 •

edited

Loading

cameel commented Jun 21, 2021 •

edited

Loading

cameel commented Jul 5, 2021 •

edited

Loading

aarlt Aug 19, 2021 •

edited

Loading

cameel commented Aug 20, 2021 •

edited

Loading