Stripping base path from CLI paths (without CLI refactor) by cameel · Pull Request #11617 · argotorg/solidity

cameel · 2021-07-05T19:28:19Z

Fixes #4702.
Implements the solc part of #11408. solc-js changes will be submitted in that repo.

This is a version of #11545 that does not depend on the CLI refactor PRs (#11518, #11520, #11544) and does not include test cases for the CLI.

Things that were not implemented or were implemented differently than specified in the issue:

Extra normalization: on Windows drive letter is removed from the path if possible.
On case-sensitive case-preserving filesystems does not use the case from disk.
- I couldn't find any portable way to get the original case. I know how to detect that it differs using Boost though so I am going to propose issuing a warning for this instead (Detect platform-dependent paths in default file loader (path spec v3) #11412).

cameel · 2021-07-05T20:11:08Z

In case anyone is wondering why this is so much more complex than my initial PR, here's a non-exhaustive list of path-related weirdness it has to account for:

/a/b/c on Windows is a relatve path. Only C:/a/b/c (or C:\a\b\c) is absolute.
A path consisting only of a drive letter (C:) is relative and not equivalent to the path to the root directory (C:\, \ or /).
If the path to the working directory contains symlinks, absolute() creates an absolute path that may or may not have these links resolved.
- On macOS they are resolved and /var/ (which contains the tmp dir in my tests) is a symlink to /private/var/.
- On Windows they are not resolved.
On Windows it is possible for / and \ to represent two different directory trees.
- This is actually the case on CircleCI Windows machines. They have C:\Program Files\Git\ mounted at /. See also Where is the Git Bash / directory?
Windows does support symbolic links. But:
- There is a special type of symlink for directories. A non-directory symlink can still point at a directory, but it is not recognized as a directory (you can't use it as a part of a path for example).
- If the symlink target is a relative path, only \ works as a path separator. If it's absolute, both / and \ work.
Boost's canonical() and weakly_canonical() both resolve symlinks. There's no direct equivalent that does not.
Boost's lexically_normal() does not convert /../../a/b/c into /a/b/c. It has to be done manually.
Boost's normalization always includes drive letter in Windows paths. This makes canonical normalized paths different on different between Windows and other platforms. Drive letters need to be stripped manually.
UNC paths (e.g. //host/b/c or \\host\b\c on Windows) are relative despite having a root name.
On Windows, when the current directory is an UNC path, \ is not a valid path (at least in the shell) and does not refer to the root directory.
Making an UNC path absolute results in the current path being appended at the end rather than at the beginning like with normal paths.
Even though duplicate path separators (a//b/c.sol) and . at the end of a path (a/b/c/.) are ignored when Boost compares paths, they are still stored in the path object.

chriseth · 2021-07-06T09:00:45Z

libsolidity/interface/FileReader.h

 		return [this](std::string const& _kind, std::string const& _path) { return readFile(_kind, _path); };
 	}

+	/// Normalizes a filesystem path in a way that removes small, inconsequential differences. Specifically:


Maybe just sommarize in the header and put the full description in the cpp file?

Sure. I thought the docstring would be a better place for this. It is a bit detailed but on the other hand it still describes the effects visible to the user of the function rather than implementation details.

docs/path-resolution.rst

chriseth · 2021-07-06T09:11:06Z

Did not review the actual code.

Jessejames1180

Approved

aarlt · 2021-07-09T22:53:11Z

libsolidity/interface/FileReader.cpp

+		);
+
+		string rootName = normalizedPath.root_name().string();
+		boost::filesystem::path normalizedRootPath =


Is it really correct to define normalizedRootPath here again?

Good catch. It should not be redeclared. Weird that it did not break any tests though.

Fixed. The normalization from \\host and //host for UNC paths was broken because of this. Tests did not break because boost ignores slashes is comparisons. Slashes do not matter as long as we use generic_string() on the path before using it but better have it normalized in case someone uses native() instead.

leonardoalt

Some minor comments, looks good in general

libsolidity/interface/FileReader.h

leonardoalt · 2021-07-15T09:39:45Z

libsolidity/interface/FileReader.h

+	/// Paths are treated as case-sensitive. Does not require the path to actually exist in the
+	/// filesystem and does not follow symlinks. Only considers whole segments, e.g. /abc/d is not
+	/// considered a prefix of /abc/def. Both paths must be non-empty.
+	static bool isPathPrefix(boost::filesystem::path _prefix, boost::filesystem::path const& _path);


Any particular reason _prefix is not const&?

I'm modifying it in place inside the function. I could add a local variable but it was just more convenient to modify the parameter.

libsolidity/interface/FileReader.h

leonardoalt · 2021-07-15T09:40:20Z

libsolidity/interface/FileReader.h

+	/// If @a _prefix is actually a prefix of @p _path, removes it from @a _path to make it relative.
+	/// Otherwise returns @a _path unchanged.
+	/// Returns '.' if @a _path and @_prefix are identical.
+	static boost::filesystem::path stripPrefixIfPresent(boost::filesystem::path _prefix, boost::filesystem::path const& _path);


Same here re const&

#11617 (comment)

leonardoalt · 2021-07-15T10:02:58Z

libsolidity/interface/FileReader.cpp

+		_prefix.remove_filename();
+
+	boost::filesystem::path strippedPath = _path.lexically_relative(_prefix);
+	solAssert(strippedPath.empty() || *strippedPath.begin() != "..", "");


So each element of strippedPath is a string?

It's actually path but strings are implicitly convertible to that type.

Boost let you iterate over path segments while skipping separators. E.g. in /a/bc/def.sol you can iterate over a, bc and def.sol.

leonardoalt · 2021-07-15T10:04:23Z

libsolidity/interface/FileReader.cpp

+		rootName.size() == 2 ||
+		(rootName.size() > 2 && rootName[2] != rootName[1])
+	) && (
+		(rootName[0] == '/' && rootName[1] == '/')


You mean "why both characters should be /" or "why both // and \\"?

If it's the former - this is meant to detect UNC paths of the form //host/a/bc/def.sol.
If the latter: Windows accepts both //host and \\host as UNC path root name. Outside of windows only the one starting with // is treated specially.

leonardoalt · 2021-07-15T10:06:28Z

test/libsolidity/interface/FileReader.cpp

+	BOOST_TEST(FileReader::normalizeCLIPathForVFS("/a/b/c/../") == "/a/b/");
+
+	BOOST_TEST(FileReader::normalizeCLIPathForVFS("/a/b/c/../../..") == "/");
+	BOOST_TEST(FileReader::normalizeCLIPathForVFS("/a/b/c/../../../") == "/");


How about some error cases? Like /a/b/c/../../../../

The function has no error cases :) The function is supposed to be able to normalize every possible path.

/a/b/c/../../../../ will reduce to /. I do have a test for that. In fact lots of tests - see normalizeCLIPathForVFS_path_beyond_root below. I grouped them together not to have monster test cases spanning half of the file :)

test/libsolidity/interface/FileReader.cpp

leonardoalt · 2021-07-15T10:07:48Z

test/libsolidity/interface/FileReader.cpp

+
+	// NOTE: If path to work dir contains symlinks (of then the case on macOS), boost might resolve
+	// them, making the path different from tempDirPath.
+	boost::filesystem::path expectedWorkDir = boost::filesystem::current_path().parent_path().parent_path().parent_path();


Why parent 3 times?

Because the working directory is set to tempDir.path() / "x/y/z" and the paths I'm testing below sometimes go up a few levels so they do not always preserve the x/y/z part. So I basically want tempDir.path() but normalized the way a working directory would be (e.g. on MacOS the temp dir I get usually has a symlink in it that I need to resolve; on Windows it has a drive letter).

But I just realized that now it's no longer working dir so the name is a bit misleading so maybe that's what you meant. Renamed to expectedPrefix.

…links used on Windows

…aths

…ource paths

cameel · 2021-07-24T03:32:24Z

Similar to #11544 (comment), I'm adding some refactors and small changes from work on #11688 as fixups (previous fixups squashed). Again, functionality did not change and almost all the changes were just in tests:

Docstring tweaks: document that isPathPrefix() ignores the trailing slash and that normalizeCLIPathForVFS() always adds a slash after . + test cases for this.
dotDotPrefix moved moved to a better spot in in normalizeCLIPathForVFS() (does not affect functionality).
Unused code removed from case-sensitivity test.
Root name stripping test now is not completely skipped outside of Windows (helps to make sure it at least compiles).
TEST_CASE_NAME.
BOOST_TEST() replaced with BOOST_CHECK_EQUAL() when comparing paths. The former for some reason does not print boost::filesystem::path values when a test fails, which makes debugging CI failures much more tedious.
- Extra parentheses removed from BOOST_TEST() in a few cases where the << operator actually exists (these parentheses were a way to make the the macro not try to print stuff).

cameel · 2021-07-27T14:42:30Z

I'm closing this in favor of #11545. The only point of this PR was to be able to merge it without having to review all the dependencies first. Now that the dependencies are already merged, there's no point in keeping it around.

I kept these PRs in sync at all times so if you already reviewed this, the only extra thing to review in #11545 is test/solc/CommandLineInterface.cpp.

cameel requested review from chriseth and christianparpart July 5, 2021 19:28

cameel self-assigned this Jul 5, 2021

This was referenced Jul 5, 2021

Stripping base path from CLI paths #11545

Merged

Stripping --base-path from CLI paths + CLI path normalization (path spec v3) #11408

Closed